Mostly cross-architecture programming questions and gotchas.
http://codereview.chromium.org/3170015/diff/8001/9001 File tools/oom_dump/README (right): http://codereview.chromium.org/3170015/diff/8001/9001#newcode1 tools/oom_dump/README:1: oom_dump extracts useful information from Google Chrome OOM minidumps. Extra space in "OOM minidumps." http://codereview.chromium.org/3170015/diff/8001/9001#newcode18 tools/oom_dump/README:18: if you're on 64-bit platform, otherwise you would get link error when link error -> a link error http://codereview.chromium.org/3170015/diff/8001/9001#newcode26 tools/oom_dump/README:26: (Additionally you can control v8 working copy dir, but default---../..--- Confusing! http://codereview.chromium.org/3170015/diff/8001/9001#newcode30 tools/oom_dump/README:30: some useful information about OOM crash. about (the||an) OOM crash. http://codereview.chromium.org/3170015/diff/8001/9003 File tools/oom_dump/oom_dump.cc (right): http://codereview.chromium.org/3170015/diff/8001/9003#newcode28 tools/oom_dump/oom_dump.cc:28: #include <algorithm> The style guide says: C system headers first, then C++ system headers. <algorithm> should follow <stdlib.h> (preferably with a blank line in between). http://codereview.chromium.org/3170015/diff/8001/9003#newcode131 tools/oom_dump/oom_dump.cc:131: const MDRawContextX86* contextX86 = crash_context->GetContextX86(); Have you tried this with non-x86 dumps? Does it work with x86_64? Your README seems to indicate so, but I can't see how this would work. It would be best if this tool were written to be independent of the architecture on which it was running. For example, regardless of whether it's running on x86 or x86_64, it should be able to interpret both x86 and x86_64 dumps. The processor library supports this. You'd probably just need to provide your own code to get the stack pointer value from whatever context type is present in the dump, given architectures you care about (x86 and x86_64?) http://codereview.chromium.org/3170015/diff/8001/9003#newcode148 tools/oom_dump/oom_dump.cc:148: if (value2 == 0xdecade00) { This might be better as a symbolic constant, like kHeapStatsSignature. http://codereview.chromium.org/3170015/diff/8001/9003#newcode164 tools/oom_dump/oom_dump.cc:164: const int new_space_size = READ_FIELD(1); Is a native "int" correct? Are these fields fixed-size or do they always use the native definition of "int"? http://codereview.chromium.org/3170015/diff/8001/9003#newcode216 tools/oom_dump/oom_dump.cc:216: printf("exception thread ID: %d (%x)\n", suggest 0x%x instead of just raw %x. Actually, you should use "%" PRIu32 and "0x%" PRIx32 here. You're printing uint32_ts. http://codereview.chromium.org/3170015/diff/8001/9003#newcode218 tools/oom_dump/oom_dump.cc:218: printf("heap stats address: %p\n", (void*)heap_stats_addr); Unlike the previous one, this one's an even bigger deal. heap_stats_addr is uint64_t, but %p expects a native-sized pointer. While we can be reasonably certain that %d and %x are compatible with uint32_t (in the previous example), there's no assurance that %p has anything to do with uint64 in this case. This should use "%#" PRIx64. Getting this sort of thing wrong will cause printf to print incorrect data and possibly crash. http://codereview.chromium.org/3170015/diff/8001/9003#newcode220 tools/oom_dump/oom_dump.cc:220: printf("\t%-25s\t% 10d\n", #stat ":", stat); These are OK if a native "int" is correct (see above). http://codereview.chromium.org/3170015/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
