Allow heap dump requests with a FileDescriptor arg.
Part of a larger change to allow "am" to initiate hprof heap dumps.
The original implementation wrote data to a temp file, because we
compute the second part first while doing GC work. Now all paths
work like DDMS, writing all data to a memstream and then copying it
out to a file at the very end.
Also, fix a potential fd leak on failure in the profiling code.
Bug 2759474.
Change-Id: I0f838cbd9948a23088f08463c3008be7198d76e7
diff --git a/vm/Profile.c b/vm/Profile.c
index f2c0c0f..57c96df 100644
--- a/vm/Profile.c
+++ b/vm/Profile.c
@@ -329,7 +329,8 @@
* trace all threads).
*
* This opens the output file (if an already open fd has not been supplied,
- * and we're not going direct to DDMS) and allocates the data buffer.
+ * and we're not going direct to DDMS) and allocates the data buffer. This
+ * takes ownership of the file descriptor, closing it on completion.
*
* On failure, we throw an exception and return.
*/
@@ -377,6 +378,7 @@
goto fail;
}
}
+ traceFd = -1;
memset(state->buf, (char)FILL_PATTERN, bufferSize);
state->directToDdms = directToDdms;
@@ -425,6 +427,8 @@
free(state->buf);
state->buf = NULL;
}
+ if (traceFd >= 0)
+ close(traceFd);
dvmUnlockMutex(&state->startStopLock);
}
diff --git a/vm/SignalCatcher.c b/vm/SignalCatcher.c
index 2b994da..40c36c6 100644
--- a/vm/SignalCatcher.c
+++ b/vm/SignalCatcher.c
@@ -221,7 +221,7 @@
{
#if WITH_HPROF
LOGI("SIGUSR1 forcing GC and HPROF dump\n");
- hprofDumpHeap(NULL, false);
+ hprofDumpHeap(NULL, -1, false);
#else
LOGI("SIGUSR1 forcing GC (no HPROF)\n");
dvmCollectGarbage(false);
diff --git a/vm/alloc/Heap.c b/vm/alloc/Heap.c
index 80b5030..474818c 100644
--- a/vm/alloc/Heap.c
+++ b/vm/alloc/Heap.c
@@ -720,7 +720,7 @@
gcHeap->hprofFileName = nameBuf;
}
gcHeap->hprofContext = hprofStartup(gcHeap->hprofFileName,
- gcHeap->hprofDirectToDdms);
+ gcHeap->hprofFd, gcHeap->hprofDirectToDdms);
if (gcHeap->hprofContext != NULL) {
hprofStartHeapDump(gcHeap->hprofContext);
}
@@ -929,11 +929,18 @@
/*
* Perform garbage collection, writing heap information to the specified file.
*
+ * If "fd" is >= 0, the output will be written to that file descriptor.
+ * Otherwise, "fileName" is used to create an output file.
+ *
* If "fileName" is NULL, a suitable name will be generated automatically.
+ * (TODO: remove this when the SIGUSR1 feature goes away)
+ *
+ * If "directToDdms" is set, the other arguments are ignored, and data is
+ * sent directly to DDMS.
*
* Returns 0 on success, or an error code on failure.
*/
-int hprofDumpHeap(const char* fileName, bool directToDdms)
+int hprofDumpHeap(const char* fileName, int fd, bool directToDdms)
{
int result;
@@ -941,6 +948,7 @@
gDvm.gcHeap->hprofDumpOnGc = true;
gDvm.gcHeap->hprofFileName = fileName;
+ gDvm.gcHeap->hprofFd = fd;
gDvm.gcHeap->hprofDirectToDdms = directToDdms;
dvmCollectGarbageInternal(false, GC_HPROF_DUMP_HEAP);
result = gDvm.gcHeap->hprofResult;
diff --git a/vm/alloc/HeapInternal.h b/vm/alloc/HeapInternal.h
index 728af95..112fc91 100644
--- a/vm/alloc/HeapInternal.h
+++ b/vm/alloc/HeapInternal.h
@@ -117,6 +117,7 @@
#if WITH_HPROF
bool hprofDumpOnGc;
const char* hprofFileName;
+ int hprofFd;
hprof_context_t *hprofContext;
int hprofResult;
bool hprofDirectToDdms;
diff --git a/vm/hprof/Hprof.c b/vm/hprof/Hprof.c
index 0b95aad..bcad8b1 100644
--- a/vm/hprof/Hprof.c
+++ b/vm/hprof/Hprof.c
@@ -25,6 +25,7 @@
#include <string.h>
#include <unistd.h>
+#include <fcntl.h>
#include <errno.h>
#include <sys/time.h>
#include <time.h>
@@ -33,35 +34,8 @@
#define kHeadSuffix "-hptemp"
hprof_context_t *
-hprofStartup(const char *outputFileName, bool directToDdms)
+hprofStartup(const char *outputFileName, int fd, bool directToDdms)
{
- FILE* fp = NULL;
-
- if (!directToDdms) {
- int len = strlen(outputFileName);
- char fileName[len + sizeof(kHeadSuffix)];
-
- /* Construct the temp file name. This wasn't handed to us by the
- * application, so we need to be careful about stomping on it.
- */
- sprintf(fileName, "%s" kHeadSuffix, outputFileName);
- if (access(fileName, F_OK) == 0) {
- LOGE("hprof: temp file %s exists, bailing\n", fileName);
- return NULL;
- }
-
- fp = fopen(fileName, "w+");
- if (fp == NULL) {
- LOGE("hprof: can't open %s: %s.\n", fileName, strerror(errno));
- return NULL;
- }
- if (unlink(fileName) != 0) {
- LOGW("hprof: WARNING: unable to remove temp file %s\n", fileName);
- /* keep going */
- }
- LOGI("hprof: dumping VM heap to \"%s\".\n", fileName);
- }
-
hprofStartup_String();
hprofStartup_Class();
#if WITH_HPROF_STACK
@@ -72,70 +46,26 @@
hprof_context_t *ctx = malloc(sizeof(*ctx));
if (ctx == NULL) {
LOGE("hprof: can't allocate context.\n");
- if (fp != NULL)
- fclose(fp);
return NULL;
}
- /* pass in "fp" for the temp file, and the name of the output file */
- hprofContextInit(ctx, strdup(outputFileName), fp, false, directToDdms);
+ /* pass in name or descriptor of the output file */
+ hprofContextInit(ctx, strdup(outputFileName), fd, false, directToDdms);
- assert(ctx->fp != NULL);
+ assert(ctx->memFp != NULL);
return ctx;
}
/*
- * Copy the entire contents of "srcFp" to "dstFp".
- *
- * Returns "true" on success.
- */
-static bool
-copyFileToFile(FILE *dstFp, FILE *srcFp)
-{
- char buf[65536];
- size_t dataRead, dataWritten;
-
- while (true) {
- dataRead = fread(buf, 1, sizeof(buf), srcFp);
- if (dataRead > 0) {
- dataWritten = fwrite(buf, 1, dataRead, dstFp);
- if (dataWritten != dataRead) {
- LOGE("hprof: failed writing data (%d of %d): %s\n",
- dataWritten, dataRead, strerror(errno));
- return false;
- }
- } else {
- if (feof(srcFp))
- return true;
- LOGE("hprof: failed reading data (res=%d): %s\n",
- dataRead, strerror(errno));
- return false;
- }
- }
-}
-
-/*
* Finish up the hprof dump. Returns true on success.
*/
bool
hprofShutdown(hprof_context_t *tailCtx)
{
- FILE *fp = NULL;
-
- /* flush output to the temp file, then prepare the output file */
+ /* flush the "tail" portion of the output */
hprofFlushCurrentRecord(tailCtx);
- LOGI("hprof: dumping heap strings to \"%s\".\n", tailCtx->fileName);
- if (!tailCtx->directToDdms) {
- fp = fopen(tailCtx->fileName, "w");
- if (fp == NULL) {
- LOGE("can't open %s: %s\n", tailCtx->fileName, strerror(errno));
- hprofFreeContext(tailCtx);
- return false;
- }
- }
-
/*
* Create a new context struct for the start of the file. We
* heap-allocate it so we can share the "free" function.
@@ -143,14 +73,13 @@
hprof_context_t *headCtx = malloc(sizeof(*headCtx));
if (headCtx == NULL) {
LOGE("hprof: can't allocate context.\n");
- if (fp != NULL)
- fclose(fp);
hprofFreeContext(tailCtx);
- return NULL;
+ return false;
}
- hprofContextInit(headCtx, strdup(tailCtx->fileName), fp, true,
+ hprofContextInit(headCtx, strdup(tailCtx->fileName), tailCtx->fd, true,
tailCtx->directToDdms);
+ LOGI("hprof: dumping heap strings to \"%s\".\n", tailCtx->fileName);
hprofDumpStrings(headCtx);
hprofDumpClasses(headCtx);
@@ -176,11 +105,11 @@
hprofShutdown_StackFrame();
#endif
- if (tailCtx->directToDdms) {
- /* flush to ensure memstream pointer and size are updated */
- fflush(headCtx->fp);
- fflush(tailCtx->fp);
+ /* flush to ensure memstream pointer and size are updated */
+ fflush(headCtx->memFp);
+ fflush(tailCtx->memFp);
+ if (tailCtx->directToDdms) {
/* send the data off to DDMS */
struct iovec iov[2];
iov[0].iov_base = headCtx->fileDataPtr;
@@ -190,22 +119,50 @@
dvmDbgDdmSendChunkV(CHUNK_TYPE("HPDS"), iov, 2);
} else {
/*
- * Append the contents of the temp file to the output file. The temp
- * file was removed immediately after being opened, so it will vanish
- * when we close it.
+ * Open the output file, and copy the head and tail to it.
*/
- rewind(tailCtx->fp);
- if (!copyFileToFile(headCtx->fp, tailCtx->fp)) {
- LOGW("hprof: file copy failed, hprof data may be incomplete\n");
- /* finish up anyway */
+ assert(headCtx->fd == tailCtx->fd);
+
+ int outFd;
+ if (headCtx->fd >= 0) {
+ outFd = dup(headCtx->fd);
+ if (outFd < 0) {
+ LOGE("dup(%d) failed: %s\n", headCtx->fd, strerror(errno));
+ /* continue to fail-handler below */
+ }
+ } else {
+ outFd = open(tailCtx->fileName, O_WRONLY|O_CREAT, 0644);
+ if (outFd < 0) {
+ LOGE("can't open %s: %s\n", headCtx->fileName, strerror(errno));
+ /* continue to fail-handler below */
+ }
+ }
+ if (outFd < 0) {
+ hprofFreeContext(headCtx);
+ hprofFreeContext(tailCtx);
+ return false;
+ }
+
+ int result;
+ result = sysWriteFully(outFd, headCtx->fileDataPtr,
+ headCtx->fileDataSize, "hprof-head");
+ result |= sysWriteFully(outFd, tailCtx->fileDataPtr,
+ tailCtx->fileDataSize, "hprof-tail");
+ close(outFd);
+ if (result != 0) {
+ hprofFreeContext(headCtx);
+ hprofFreeContext(tailCtx);
+ return false;
}
}
+ /* throw out a log message for the benefit of "runhat" */
+ LOGI("hprof: heap dump completed (%dKB)\n",
+ (headCtx->fileDataSize + tailCtx->fileDataSize + 1023) / 1024);
+
hprofFreeContext(headCtx);
hprofFreeContext(tailCtx);
- /* throw out a log message for the benefit of "runhat" */
- LOGI("hprof: heap dump completed, temp file removed\n");
return true;
}
@@ -217,8 +174,10 @@
{
assert(ctx != NULL);
- if (ctx->fp != NULL)
- fclose(ctx->fp);
+ /* we don't own ctx->fd, do not close */
+
+ if (ctx->memFp != NULL)
+ fclose(ctx->memFp);
free(ctx->curRec.body);
free(ctx->fileName);
free(ctx->fileDataPtr);
diff --git a/vm/hprof/Hprof.h b/vm/hprof/Hprof.h
index fd89d06..18f4102 100644
--- a/vm/hprof/Hprof.h
+++ b/vm/hprof/Hprof.h
@@ -133,15 +133,16 @@
size_t objectsInSegment;
/*
- * If "directToDdms" is not set, "fileName" is valid, and "fileDataPtr"
- * and "fileDataSize" are not used. If "directToDdms" is not set,
- * it's the other way around.
+ * If directToDdms is set, "fileName" and "fd" will be ignored.
+ * Otherwise, "fileName" must be valid, though if "fd" >= 0 it will
+ * only be used for debug messages.
*/
bool directToDdms;
char *fileName;
char *fileDataPtr; // for open_memstream
size_t fileDataSize; // for open_memstream
- FILE *fp;
+ FILE *memFp;
+ int fd;
} hprof_context_t;
@@ -187,7 +188,7 @@
* HprofOutput.c functions
*/
-void hprofContextInit(hprof_context_t *ctx, char *fileName, FILE *fp,
+void hprofContextInit(hprof_context_t *ctx, char *fileName, int fd,
bool writeHeader, bool directToDdms);
int hprofFlushRecord(hprof_record_t *rec, FILE *fp);
@@ -244,7 +245,8 @@
* Hprof.c functions
*/
-hprof_context_t* hprofStartup(const char *outputFileName, bool directToDdms);
+hprof_context_t* hprofStartup(const char *outputFileName, int fd,
+ bool directToDdms);
bool hprofShutdown(hprof_context_t *ctx);
void hprofFreeContext(hprof_context_t *ctx);
@@ -255,7 +257,7 @@
* the heap implementation; these functions require heap knowledge,
* so they are implemented in Heap.c.
*/
-int hprofDumpHeap(const char* fileName, bool directToDdms);
+int hprofDumpHeap(const char* fileName, int fd, bool directToDdms);
void dvmHeapSetHprofGcScanState(hprof_heap_tag_t state, u4 threadSerialNumber);
#endif // _DALVIK_HPROF_HPROF
diff --git a/vm/hprof/HprofOutput.c b/vm/hprof/HprofOutput.c
index 0677c85..25512b2 100644
--- a/vm/hprof/HprofOutput.c
+++ b/vm/hprof/HprofOutput.c
@@ -59,32 +59,30 @@
/*
* Initialize an hprof context struct.
*
- * This will take ownership of "fileName" and "fp".
+ * This will take ownership of "fileName".
*/
void
-hprofContextInit(hprof_context_t *ctx, char *fileName, FILE *fp,
+hprofContextInit(hprof_context_t *ctx, char *fileName, int fd,
bool writeHeader, bool directToDdms)
{
memset(ctx, 0, sizeof (*ctx));
- if (directToDdms) {
- /*
- * Have to do this here, because it must happen after we
- * memset the struct (want to treat fileDataPtr/fileDataSize
- * as read-only while the file is open).
- */
- assert(fp == NULL);
- fp = open_memstream(&ctx->fileDataPtr, &ctx->fileDataSize);
- if (fp == NULL) {
- /* not expected */
- LOGE("hprof: open_memstream failed: %s\n", strerror(errno));
- dvmAbort();
- }
+ /*
+ * Have to do this here, because it must happen after we
+ * memset the struct (want to treat fileDataPtr/fileDataSize
+ * as read-only while the file is open).
+ */
+ FILE* fp = open_memstream(&ctx->fileDataPtr, &ctx->fileDataSize);
+ if (fp == NULL) {
+ /* not expected */
+ LOGE("hprof: open_memstream failed: %s\n", strerror(errno));
+ dvmAbort();
}
ctx->directToDdms = directToDdms;
ctx->fileName = fileName;
- ctx->fp = fp;
+ ctx->memFp = fp;
+ ctx->fd = fd;
ctx->curRec.allocLen = 128;
ctx->curRec.body = malloc(ctx->curRec.allocLen);
@@ -158,7 +156,7 @@
int
hprofFlushCurrentRecord(hprof_context_t *ctx)
{
- return hprofFlushRecord(&ctx->curRec, ctx->fp);
+ return hprofFlushRecord(&ctx->curRec, ctx->memFp);
}
int
@@ -167,7 +165,7 @@
hprof_record_t *rec = &ctx->curRec;
int err;
- err = hprofFlushRecord(rec, ctx->fp);
+ err = hprofFlushRecord(rec, ctx->memFp);
if (err != 0) {
return err;
} else if (rec->dirty) {
diff --git a/vm/native/dalvik_system_VMDebug.c b/vm/native/dalvik_system_VMDebug.c
index d27926c..2f79033 100644
--- a/vm/native/dalvik_system_VMDebug.c
+++ b/vm/native/dalvik_system_VMDebug.c
@@ -20,10 +20,40 @@
#include "Dalvik.h"
#include "native/InternalNativePriv.h"
+#include <string.h>
+#include <unistd.h>
#include <errno.h>
/*
+ * Extracts the fd from a FileDescriptor object.
+ *
+ * If an error is encountered, or the extracted descriptor is numerically
+ * invalid, this returns -1 with an exception raised.
+ */
+static int getFileDescriptor(Object* obj)
+{
+ assert(obj != NULL);
+ assert(strcmp(obj->clazz->descriptor, "Ljava/io/FileDescriptor;") == 0);
+
+ InstField* field = dvmFindInstanceField(obj->clazz, "descriptor", "I");
+ if (field == NULL) {
+ dvmThrowException("Ljava/lang/NoSuchFieldException;",
+ "No FileDescriptor.descriptor field");
+ return -1;
+ }
+
+ int fd = dvmGetFieldInt(obj, field->byteOffset);
+ if (fd < 0) {
+ dvmThrowExceptionFmt("Ljava/lang/RuntimeException;",
+ "Invalid file descriptor");
+ return -1;
+ }
+
+ return fd;
+}
+
+/*
* Convert an array of char* into a String[].
*
* Returns NULL on failure, with an exception raised.
@@ -326,7 +356,7 @@
{
#ifdef WITH_PROFILER
StringObject* traceFileStr = (StringObject*) args[0];
- DataObject* traceFd = (DataObject*) args[1];
+ Object* traceFd = (Object*) args[1];
int bufferSize = args[2];
int flags = args[3];
@@ -346,17 +376,14 @@
int fd = -1;
if (traceFd != NULL) {
- InstField* field =
- dvmFindInstanceField(traceFd->obj.clazz, "descriptor", "I");
- if (field == NULL) {
- dvmThrowException("Ljava/lang/NoSuchFieldException;",
- "No FileDescriptor.descriptor field");
+ int origFd = getFileDescriptor(traceFd);
+ if (origFd < 0)
RETURN_VOID();
- }
- fd = dup(dvmGetFieldInt(&traceFd->obj, field->byteOffset));
+
+ fd = dup(origFd);
if (fd < 0) {
dvmThrowExceptionFmt("Ljava/lang/RuntimeException;",
- "dup() failed: %s", strerror(errno));
+ "dup(%d) failed: %s", origFd, strerror(errno));
RETURN_VOID();
}
}
@@ -660,7 +687,7 @@
}
/*
- * static void dumpHprofData(String fileName)
+ * static void dumpHprofData(String fileName, FileDescriptor fd)
*
* Cause "hprof" data to be dumped. We can throw an IOException if an
* error occurs during file handling.
@@ -670,22 +697,37 @@
{
#ifdef WITH_HPROF
StringObject* fileNameStr = (StringObject*) args[0];
+ Object* fileDescriptor = (Object*) args[1];
char* fileName;
int result;
- if (fileNameStr == NULL) {
+ /*
+ * Only one of these may be NULL.
+ */
+ if (fileNameStr == NULL && fileDescriptor == NULL) {
dvmThrowException("Ljava/lang/NullPointerException;", NULL);
RETURN_VOID();
}
- fileName = dvmCreateCstrFromString(fileNameStr);
- if (fileName == NULL) {
- /* unexpected -- malloc failure? */
- dvmThrowException("Ljava/lang/RuntimeException;", "malloc failure?");
- RETURN_VOID();
+ if (fileNameStr != NULL) {
+ fileName = dvmCreateCstrFromString(fileNameStr);
+ if (fileName == NULL) {
+ /* unexpected -- malloc failure? */
+ dvmThrowException("Ljava/lang/RuntimeException;", "malloc failure?");
+ RETURN_VOID();
+ }
+ } else {
+ fileName = strdup("[fd]");
}
- result = hprofDumpHeap(fileName, false);
+ int fd = -1;
+ if (fileDescriptor != NULL) {
+ fd = getFileDescriptor(fileDescriptor);
+ if (fd < 0)
+ RETURN_VOID();
+ }
+
+ result = hprofDumpHeap(fileName, fd, false);
free(fileName);
if (result != 0) {
@@ -712,7 +754,7 @@
#ifdef WITH_HPROF
int result;
- result = hprofDumpHeap("[DDMS]", true);
+ result = hprofDumpHeap("[DDMS]", -1, true);
if (result != 0) {
/* ideally we'd throw something more specific based on actual failure */
@@ -938,7 +980,7 @@
Dalvik_dalvik_system_VMDebug_getLoadedClassCount },
{ "threadCpuTimeNanos", "()J",
Dalvik_dalvik_system_VMDebug_threadCpuTimeNanos },
- { "dumpHprofData", "(Ljava/lang/String;)V",
+ { "dumpHprofData", "(Ljava/lang/String;Ljava/io/FileDescriptor;)V",
Dalvik_dalvik_system_VMDebug_dumpHprofData },
{ "dumpHprofDataDdms", "()V",
Dalvik_dalvik_system_VMDebug_dumpHprofDataDdms },
