From 4b851a75f7712086a9fc4427f68c99b83725f37d Mon Sep 17 00:00:00 2001 From: Andy McFadden Date: Fri, 9 Jul 2010 16:50:05 -0700 Subject: [PATCH] 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 --- vm/Profile.c | 6 +- vm/SignalCatcher.c | 2 +- vm/alloc/Heap.c | 12 ++- vm/alloc/HeapInternal.h | 1 + vm/hprof/Hprof.c | 149 ++++++++++++++------------------------ vm/hprof/Hprof.h | 16 ++-- vm/hprof/HprofOutput.c | 34 ++++----- vm/native/dalvik_system_VMDebug.c | 80 +++++++++++++++----- 8 files changed, 157 insertions(+), 143 deletions(-) diff --git a/vm/Profile.c b/vm/Profile.c index f2c0c0fc4..57c96dff1 100644 --- a/vm/Profile.c +++ b/vm/Profile.c @@ -329,7 +329,8 @@ static void dumpMethodList(FILE* fp) * 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 @@ void dvmMethodTraceStart(const char* traceFileName, int traceFd, int bufferSize, goto fail; } } + traceFd = -1; memset(state->buf, (char)FILL_PATTERN, bufferSize); state->directToDdms = directToDdms; @@ -425,6 +427,8 @@ fail: 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 2b994dac2..40c36c6ea 100644 --- a/vm/SignalCatcher.c +++ b/vm/SignalCatcher.c @@ -221,7 +221,7 @@ static void handleSigUsr1(void) { #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 80b5030f8..474818cc1 100644 --- a/vm/alloc/Heap.c +++ b/vm/alloc/Heap.c @@ -720,7 +720,7 @@ void dvmCollectGarbageInternal(bool clearSoftRefs, GcReason reason) 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 @@ void dvmCollectGarbageInternal(bool clearSoftRefs, GcReason reason) /* * 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 @@ int hprofDumpHeap(const char* fileName, bool directToDdms) 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 728af9531..112fc914f 100644 --- a/vm/alloc/HeapInternal.h +++ b/vm/alloc/HeapInternal.h @@ -117,6 +117,7 @@ struct GcHeap { #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 0b95aad01..bcad8b1f9 100644 --- a/vm/hprof/Hprof.c +++ b/vm/hprof/Hprof.c @@ -25,6 +25,7 @@ #include #include +#include #include #include #include @@ -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 @@ hprofStartup(const char *outputFileName, bool directToDdms) 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 @@ hprofShutdown(hprof_context_t *tailCtx) 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(hprof_context_t *tailCtx) 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 @@ hprofShutdown(hprof_context_t *tailCtx) 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 @@ hprofFreeContext(hprof_context_t *ctx) { 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 fd89d06d4..18f410253 100644 --- a/vm/hprof/Hprof.h +++ b/vm/hprof/Hprof.h @@ -133,15 +133,16 @@ typedef struct hprof_context_t { 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 @@ int hprofDumpHeapObject(hprof_context_t *ctx, const Object *obj); * 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 @@ int hprofShutdown_StackFrame(void); * 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 @@ void hprofFreeContext(hprof_context_t *ctx); * 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 0677c85d2..25512b2fa 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 @@ hprofFlushRecord(hprof_record_t *rec, FILE *fp) int hprofFlushCurrentRecord(hprof_context_t *ctx) { - return hprofFlushRecord(&ctx->curRec, ctx->fp); + return hprofFlushRecord(&ctx->curRec, ctx->memFp); } int @@ -167,7 +165,7 @@ hprofStartNewRecord(hprof_context_t *ctx, u1 tag, u4 time) 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 d27926c58..2f7903378 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 +#include #include /* + * 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 @@ static void Dalvik_dalvik_system_VMDebug_startMethodTracingNative(const u4* args { #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 @@ static void Dalvik_dalvik_system_VMDebug_startMethodTracingNative(const u4* args 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 Dalvik_dalvik_system_VMDebug_threadCpuTimeNanos(const u4* args, } /* - * 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 @@ static void Dalvik_dalvik_system_VMDebug_dumpHprofData(const u4* args, { #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]"); + } + + int fd = -1; + if (fileDescriptor != NULL) { + fd = getFileDescriptor(fileDescriptor); + if (fd < 0) + RETURN_VOID(); } - result = hprofDumpHeap(fileName, false); + result = hprofDumpHeap(fileName, fd, false); free(fileName); if (result != 0) { @@ -712,7 +754,7 @@ static void Dalvik_dalvik_system_VMDebug_dumpHprofDataDdms(const u4* args, #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 @@ const DalvikNativeMethod dvm_dalvik_system_VMDebug[] = { 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 }, -- 2.11.0