Thanks a lot, Mark, for comments. I've started http://codereview.chromium.org/3119023/show to address them.
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. On 2010/08/16 20:56:43, Mark Mentovai wrote:
Extra space in "OOM minidumps."
Done. 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 On 2010/08/16 20:56:43, Mark Mentovai wrote:
link error -> a link error
Done. http://codereview.chromium.org/3170015/diff/8001/9001#newcode26 tools/oom_dump/README:26: (Additionally you can control v8 working copy dir, but default---../..--- On 2010/08/16 20:56:43, Mark Mentovai wrote:
Confusing!
Just simplified it. Is it fine now? http://codereview.chromium.org/3170015/diff/8001/9001#newcode30 tools/oom_dump/README:30: some useful information about OOM crash. On 2010/08/16 20:56:43, Mark Mentovai wrote:
about (the||an) OOM crash.
Done. 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> On 2010/08/16 20:56:43, Mark Mentovai wrote:
The style guide says: C system headers first, then C++ system headers. <algorithm> should follow <stdlib.h> (preferably with a blank line in
between). Done. Also removed some now unnecessary deps. http://codereview.chromium.org/3170015/diff/8001/9003#newcode131 tools/oom_dump/oom_dump.cc:131: const MDRawContextX86* contextX86 = crash_context->GetContextX86(); On 2010/08/16 20:56:43, Mark Mentovai wrote:
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?)
Mark, my bad---I didn't make it explicit what is expected quality for me. I wrote this tool just to make analysis of Windows OOM minidumps easier. I'll probably will need to support x64 later on (to analyse Linux/MacOS crash dumps), but I'd rather postpone that until it's necessary. So, unless you feel very strong against that, I would keep it as is (adding a check that x86 dump is analysed right now), ok? Still I'll try to adjust all other portability issues you spotted. http://codereview.chromium.org/3170015/diff/8001/9003#newcode148 tools/oom_dump/oom_dump.cc:148: if (value2 == 0xdecade00) { On 2010/08/16 20:56:43, Mark Mentovai wrote:
This might be better as a symbolic constant, like kHeapStatsSignature.
Done. http://codereview.chromium.org/3170015/diff/8001/9003#newcode164 tools/oom_dump/oom_dump.cc:164: const int new_space_size = READ_FIELD(1); On 2010/08/16 20:56:43, Mark Mentovai wrote:
Is a native "int" correct?
Are these fields fixed-size or do they always use the native
definition of
"int"?
They are declared as plain ints: http://www.google.com/codesearch/p?hl=en#W9JxUuHYyMg/trunk/src/heap.h&q=class%20HeapStats&sa=N&cd=1&ct=rc&l=1325 http://codereview.chromium.org/3170015/diff/8001/9003#newcode216 tools/oom_dump/oom_dump.cc:216: printf("exception thread ID: %d (%x)\n", On 2010/08/16 20:56:43, Mark Mentovai wrote:
suggest 0x%x instead of just raw %x.
Actually, you should use "%" PRIu32 and "0x%" PRIx32 here. You're
printing
uint32_ts.
I wasn't sure those a portable enough, but as GCC understands them fine, let's use it :) 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); On 2010/08/16 20:56:43, Mark Mentovai wrote:
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.
Done. http://codereview.chromium.org/3170015/diff/8001/9003#newcode220 tools/oom_dump/oom_dump.cc:220: printf("\t%-25s\t% 10d\n", #stat ":", stat); On 2010/08/16 20:56:43, Mark Mentovai wrote:
These are OK if a native "int" is correct (see above).
They should be. http://codereview.chromium.org/3170015/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
