Initial implementation of oom_dump utility.

Initial implementation of oom_dump utility.

Note: it intentionally lags behind current HeapStats (which now has os_error field)
as these new minidumps could not be produced by current versions of Chrome yet.

Committed: http://code.google.com/p/v8/source/detail?r=5272

[antonm, 2010-08-13 16:35:12]

Mads and Mark,

may you have a look?

As commit message points out it's intentionally slightly behind current
HeapStats, I would update it laster when my latest goes live.

tia and yours.

[Mads Ager (chromium), 2010-08-14 10:54:40]

LGTM

Thanks for implementing this utility Anton! This will make OOM investigates much
easier.

http://codereview.chromium.org/3170015/diff/2001/1004
File tools/oom_dump/README (right):

http://codereview.chromium.org/3170015/diff/2001/1004#newcode1
tools/oom_dump/README:1: oom_dump extracts useful information from OOM minidump.
OOM minidumps -> Google Chrome OOM minidumps.

http://codereview.chromium.org/3170015/diff/2001/1004#newcode3
tools/oom_dump/README:3: To build one needs google-breakpad checkout
needs -> needs a

http://codereview.chromium.org/3170015/diff/2001/1004#newcode15
tools/oom_dump/README:15: put a soft line into /usr/lib directory).
line -> link

http://codereview.chromium.org/3170015/diff/2001/1006
File tools/oom_dump/oom_dump.cc (right):

http://codereview.chromium.org/3170015/diff/2001/1006#newcode72
tools/oom_dump/oom_dump.cc:72: u_int64_t array_ptr, int size,
Could you put one argument per line? Here and below.

http://codereview.chromium.org/3170015/diff/2001/1006#newcode262
tools/oom_dump/oom_dump.cc:262: printf(
The first argument will fit on the line here.

http://codereview.chromium.org/3170015/diff/2001/1006#newcode267
tools/oom_dump/oom_dump.cc:267: printf(
First argument will fit on the line.

[antonm, 2010-08-16 12:35:23]

All comments addressed, thanks a lot for review, Mads, submitting.

Mark, if you have any comments, just let me know and I'll update files in a
separate CL.

http://codereview.chromium.org/3170015/diff/2001/1004
File tools/oom_dump/README (right):

http://codereview.chromium.org/3170015/diff/2001/1004#newcode1
tools/oom_dump/README:1: oom_dump extracts useful information from OOM minidump.
On 2010/08/14 10:54:41, Mads Ager wrote:
> OOM minidumps -> Google Chrome OOM minidumps.

Done.

http://codereview.chromium.org/3170015/diff/2001/1004#newcode3
tools/oom_dump/README:3: To build one needs google-breakpad checkout
On 2010/08/14 10:54:41, Mads Ager wrote:
> needs -> needs a

Done.

http://codereview.chromium.org/3170015/diff/2001/1004#newcode15
tools/oom_dump/README:15: put a soft line into /usr/lib directory).
On 2010/08/14 10:54:41, Mads Ager wrote:
> line -> link

Done.

http://codereview.chromium.org/3170015/diff/2001/1006
File tools/oom_dump/oom_dump.cc (right):

http://codereview.chromium.org/3170015/diff/2001/1006#newcode72
tools/oom_dump/oom_dump.cc:72: u_int64_t array_ptr, int size,
On 2010/08/14 10:54:41, Mads Ager wrote:
> Could you put one argument per line? Here and below.

Done.

http://codereview.chromium.org/3170015/diff/2001/1006#newcode262
tools/oom_dump/oom_dump.cc:262: printf(
On 2010/08/14 10:54:41, Mads Ager wrote:
> The first argument will fit on the line here.

Done.

http://codereview.chromium.org/3170015/diff/2001/1006#newcode267
tools/oom_dump/oom_dump.cc:267: printf(
On 2010/08/14 10:54:41, Mads Ager wrote:
> First argument will fit on the line.

Done.

[Mark Mentovai, 2010-08-16 20:56:43]

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).

[antonm, 2010-08-17 13:13:19]

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...

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.