Adding untrusted crash dump / stack trace tests.

Adding untrusted crash dump / stack trace tests.
This demonstrates using hardware exception handling support to produce
stack traces in crashed modules.

Not yet handled:
  - 64-bit
  - registers
  - upload of dumps
  - call frame variable/parameter decoding


BUG=http://code.google.com/p/nativeclient/issues/detail?id=2595
TEST=untrusted_crash_dump_test,inbrowser_untrusted_crash_dump_test
R=mseaborn@chromium.org,ncbray@chromium.org

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=7825

[Mark Seaborn, 2012-02-06 17:35:04]

I've just looked at the TCB change so far...

http://codereview.chromium.org/9316125/diff/3010/src/trusted/service_runtime/...
File src/trusted/service_runtime/build.scons (right):

http://codereview.chromium.org/9316125/diff/3010/src/trusted/service_runtime/...
src/trusted/service_runtime/build.scons:326:
env_exc.ComponentObject('sel_main_exc', 'sel_main.c')] + r_debug_objs
This sets a bad precedent.  I don't want to start building different variants of
sel_ldr that differ only in whether an option is enabled or disabled.  If we did
this for more options there would be an exponential explosion.  Can you use an
environment variable for enabling exception handling instead?

[bradn, 2012-02-06 19:26:59]

PTAL

http://codereview.chromium.org/9316125/diff/3010/src/trusted/service_runtime/...
File src/trusted/service_runtime/build.scons (right):

http://codereview.chromium.org/9316125/diff/3010/src/trusted/service_runtime/...
src/trusted/service_runtime/build.scons:326:
env_exc.ComponentObject('sel_main_exc', 'sel_main.c')] + r_debug_objs
On 2012/02/06 17:35:04, Mark Seaborn wrote:
> This sets a bad precedent.  I don't want to start building different variants
of
> sel_ldr that differ only in whether an option is enabled or disabled.  If we
did
> this for more options there would be an exponential explosion.  Can you use an
> environment variable for enabling exception handling instead?

Done.
Odd, I didn't know the environment get propagated thru like that.
Is that wise?
I had the impression is was scrubbed.

[Mark Seaborn, 2012-02-06 19:44:06]

http://codereview.chromium.org/9316125/diff/3010/SConstruct
File SConstruct (right):

http://codereview.chromium.org/9316125/diff/3010/SConstruct#newcode1489
SConstruct:1489: cmd = ('${PYTHON} ' +
Use AutoDepsCommand() instead of concatenating the string yourself, and then
dependencies will be handled properly.

http://codereview.chromium.org/9316125/diff/3010/tests/inbrowser_untrusted_cr...
File tests/inbrowser_untrusted_crash_dump_test/nacl.scons (right):

http://codereview.chromium.org/9316125/diff/3010/tests/inbrowser_untrusted_cr...
tests/inbrowser_untrusted_crash_dump_test/nacl.scons:8: # Exception handling
support is only implemented for x86-32 Windows.
Fix comment.

http://codereview.chromium.org/9316125/diff/3010/tests/inbrowser_untrusted_cr...
tests/inbrowser_untrusted_crash_dump_test/nacl.scons:16:
env.FilterOut(CCFLAGS=['-O2', '-fomit-frame-pointer'])
Shouldn't be needed if the following line really appends.  This wouldn't be
useful if we switched to '-On', n!=2.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
File tests/untrusted_crash_dump/decode_dump.py (right):

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:8: Current this produces a simple
stack trace.
'Currently'

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:29: filepart =
posixpath.split(filename)[1]
Use posixpath.dirname()?

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:31: nmf_url = nmf_entry.get('x86-32',
{}).get('url')
Add a TODO for the 'x86-32' part.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:34: if not p: continue
Split into two lines.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:38: break
Split this loop into a separate lookup function, and use 'return' instead of
'break'?

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:50: options.addr2line, '-f', '-e',
filename, ('0x%08x' % addr),
Ideally you should also use '--inlines', and check for multiple results.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:53: (p_stdout, p_stderr) =
p.communicate()
BTW, you can use
var1, var2 = foo()
without the brackets.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:64: def Decode(options, core_path,
dst_trace):
'dst' -> 'dest'

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:75: for f in core['frames']:
'f' -> 'frame'

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:80:
posixpath.split(info['filename'])[1],
posixpath.basename()?  But why do you want to drop the full path?

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
File tests/untrusted_crash_dump/untrusted_crash_dump.c (right):

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:37: uint32_t addr;
uintptr_t

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:39: } PROGRAM_TABLE_DATA;
Naming style: Just call it 'struct ProgramTableData'?

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:86: uint32_t start, end;
One var per line is our usual style, I think.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:129: (void)core;
Use UNREFERENCED_PARAMETER(), or nothing if the warning is not enabled.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:139: uint32_t
SafeRead(uint32_t a) {
SafeRead() is not safe.  Does it need a TODO?

Types should probably be uintptr_t.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:187:
core_filename=getenv("NACLCOREFILE");
spaces around '='

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:227: assert(!result);
assert(result == 0);

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:236: result =
NACL_SYSCALL(exception_handler)(g_PrevHandler, &old);
'old' is unused.  Use NULL.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:246: stack =
malloc(CRASH_STACK_SIZE);
mallocing stacks is bad because there's no guard page.  Preferably mmap and set
a guard page.  Or leave exception stacks to a later improvement?

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:248: result =
NACL_SYSCALL(exception_stack)(stack, CRASH_STACK_SIZE);
FYI, you don't really need to set an exception stack.  Comment that calling this
is optional?

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
File tests/untrusted_crash_dump/untrusted_crash_dump.h (right):

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.h:19: extern void
NaClCrashDumpInit(char *program_name);
'extern' not needed.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
File tests/untrusted_crash_dump/untrusted_crash_dump_test.c (right):

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump_test.c:44:
NaClPluginLowLevelInitializationComplete();
Please comment what this is for.  Do we actually need to sync with the trusted
plugin?

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump_test.c:53: fprintf(stderr, "**
intended_exit_status=0\n");
Surely you expect it to crash and not reach here?  So just 'return 1' here.

[bradn, 2012-02-06 22:27:48]

PTAL

http://codereview.chromium.org/9316125/diff/3010/SConstruct
File SConstruct (right):

http://codereview.chromium.org/9316125/diff/3010/SConstruct#newcode1489
SConstruct:1489: cmd = ('${PYTHON} ' +
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> Use AutoDepsCommand() instead of concatenating the string yourself, and then
> dependencies will be handled properly.

This is an action not a command, so reusing AutoDepsCommand is non-trivial. (Its
an action so it can be inserted in front of the golden file check, which is also
an action).
I briefly messed with generalizing AutoDeps, only to realize that actions can't
really have dependencies like that anyway.

I've changed it to a list for readability.

I'm game to rearrange how AutoDeps + golden files work, but am a little unsure
the desired end state, and suspect that's out of scope for this change?

http://codereview.chromium.org/9316125/diff/3010/tests/inbrowser_untrusted_cr...
File tests/inbrowser_untrusted_crash_dump_test/nacl.scons (right):

http://codereview.chromium.org/9316125/diff/3010/tests/inbrowser_untrusted_cr...
tests/inbrowser_untrusted_crash_dump_test/nacl.scons:8: # Exception handling
support is only implemented for x86-32 Windows.
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> Fix comment.

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/inbrowser_untrusted_cr...
tests/inbrowser_untrusted_crash_dump_test/nacl.scons:16:
env.FilterOut(CCFLAGS=['-O2', '-fomit-frame-pointer'])
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> Shouldn't be needed if the following line really appends.  This wouldn't be
> useful if we switched to '-On', n!=2.

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
File tests/untrusted_crash_dump/decode_dump.py (right):

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:8: Current this produces a simple
stack trace.
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> 'Currently'

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:29: filepart =
posixpath.split(filename)[1]
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> Use posixpath.dirname()?
I assume you mean basename.
Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:31: nmf_url = nmf_entry.get('x86-32',
{}).get('url')
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> Add a TODO for the 'x86-32' part.

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:34: if not p: continue
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> Split into two lines.

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:38: break
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> Split this loop into a separate lookup function, and use 'return' instead of
> 'break'?

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:50: options.addr2line, '-f', '-e',
filename, ('0x%08x' % addr),
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> Ideally you should also use '--inlines', and check for multiple results.

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:53: (p_stdout, p_stderr) =
p.communicate()
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> BTW, you can use
> var1, var2 = foo()
> without the brackets.

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:64: def Decode(options, core_path,
dst_trace):
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> 'dst' -> 'dest'

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:75: for f in core['frames']:
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> 'f' -> 'frame'

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/d...
tests/untrusted_crash_dump/decode_dump.py:80:
posixpath.split(info['filename'])[1],
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> posixpath.basename()?  But why do you want to drop the full path?
To allow golden stace traces.
I've added a verbose flag to keep the full path.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
File tests/untrusted_crash_dump/untrusted_crash_dump.c (right):

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:37: uint32_t addr;
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> uintptr_t

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:39: } PROGRAM_TABLE_DATA;
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> Naming style: Just call it 'struct ProgramTableData'?

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:86: uint32_t start, end;
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> One var per line is our usual style, I think.

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:129: (void)core;
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> Use UNREFERENCED_PARAMETER(), or nothing if the warning is not enabled.

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:139: uint32_t
SafeRead(uint32_t a) {
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> SafeRead() is not safe.  Does it need a TODO?
> 
> Types should probably be uintptr_t.

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:187:
core_filename=getenv("NACLCOREFILE");
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> spaces around '='

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:227: assert(!result);
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> assert(result == 0);

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:236: result =
NACL_SYSCALL(exception_handler)(g_PrevHandler, &old);
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> 'old' is unused.  Use NULL.

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:246: stack =
malloc(CRASH_STACK_SIZE);
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> mallocing stacks is bad because there's no guard page.  Preferably mmap and
set
> a guard page.  Or leave exception stacks to a later improvement?
Switched to mmap + guard.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.c:248: result =
NACL_SYSCALL(exception_stack)(stack, CRASH_STACK_SIZE);
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> FYI, you don't really need to set an exception stack.  Comment that calling
this
> is optional?

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
File tests/untrusted_crash_dump/untrusted_crash_dump.h (right):

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump.h:19: extern void
NaClCrashDumpInit(char *program_name);
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> 'extern' not needed.

Done.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
File tests/untrusted_crash_dump/untrusted_crash_dump_test.c (right):

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump_test.c:44:
NaClPluginLowLevelInitializationComplete();
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> Please comment what this is for.  Do we actually need to sync with the trusted
> plugin?

Ooops, cargo cult.
Dropped.

http://codereview.chromium.org/9316125/diff/3010/tests/untrusted_crash_dump/u...
tests/untrusted_crash_dump/untrusted_crash_dump_test.c:53: fprintf(stderr, "**
intended_exit_status=0\n");
On 2012/02/06 19:44:06, Mark Seaborn wrote:
> Surely you expect it to crash and not reach here?  So just 'return 1' here.
Whoops.
Leftover, thanks.
Done.

[Mark Seaborn, 2012-02-09 22:24:55]

http://codereview.chromium.org/9316125/diff/15001/tests/inbrowser_untrusted_c...
File tests/inbrowser_untrusted_crash_dump_test/nacl.scons (right):

http://codereview.chromium.org/9316125/diff/15001/tests/inbrowser_untrusted_c...
tests/inbrowser_untrusted_crash_dump_test/nacl.scons:1: # -*- python -*-
The in-browser version isn't going to work for NaCl-in-Chromium just yet, so
this would make chrome_browser_tests fail when Chromium's DEPS is updated.  I'd
suggest dropping the in-browser version of this test for now and let's focus on
the backtracing first.

http://codereview.chromium.org/9316125/diff/15001/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/nacl.scons (right):

http://codereview.chromium.org/9316125/diff/15001/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:10: is_broken = (not
(env.Bit('host_windows') or env.Bit('host_mac')) or
You can drop this because we have exception handling on all x86-32 systems now.

http://codereview.chromium.org/9316125/diff/15001/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:31:
env['ENV']['NACL_UNTRUSTED_EXCEPTION_HANDLING'] = '1'
env['ENV'] *might* be shared across all the .scons files.

http://codereview.chromium.org/9316125/diff/15001/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:47: env.AddPostAction(
Can't this be a separate Scons target that depends on dump_file?

http://codereview.chromium.org/9316125/diff/15001/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/untrusted_crash_dump_test_glibc.golden (right):

http://codereview.chromium.org/9316125/diff/15001/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump_test_glibc.golden:8:
__libc_start_main at libc-start.c:227
I don't like having glibc line numbers in golden files.  It will just create
problems for anyone updating DEPS or running the tests on different libc
versions.

My suggestion:  Have a Python test script that does this:

 * imports decode_dump.py
 * Calls a function in decode_dump to get an unformatted backtrace as a Python
list.
 * Assert that the list starts with the function names "layer5", "layer4" etc.
with the filename "untrusted_crash_dump_test.c".
 * Don't embed line numbers in the test.  But you can check a couple of line
numbers by reading the line from the file (using Python's linecache) and
asserting that the line contains a special comment.
 * Also check that you can format the backtrace without getting a Python
exception (but don't look at the formatted backtrace itself).

http://codereview.chromium.org/9316125/diff/23001/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/untrusted_crash_dump.c (right):

http://codereview.chromium.org/9316125/diff/23001/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:160: next = SafeRead(sp);
This doesn't work for crashes in non-trivial functions where %esp no longer
points to the return address.  You need to start off with the frame pointer, not
the stack pointer.  For example, see:
http://codereview.chromium.org/3462021/diff/6001/src/native_client/src/truste...

That means that currently you will need some inline assembly to get %ebp, until
we put this into the exception stack frame.

You should be able to trigger the non-trivial behaviour with something like
this:

char *var; /* might need to declare this volatile */
void crasher() {
  var = alloca(100);
  *(volatile int *) 0 = 0;
}

[bradn, 2012-02-11 01:04:14]

PTAL

http://codereview.chromium.org/9316125/diff/15001/tests/inbrowser_untrusted_c...
File tests/inbrowser_untrusted_crash_dump_test/nacl.scons (right):

http://codereview.chromium.org/9316125/diff/15001/tests/inbrowser_untrusted_c...
tests/inbrowser_untrusted_crash_dump_test/nacl.scons:1: # -*- python -*-
On 2012/02/09 22:24:55, Mark Seaborn wrote:
> The in-browser version isn't going to work for NaCl-in-Chromium just yet, so
> this would make chrome_browser_tests fail when Chromium's DEPS is updated. 
I'd
> suggest dropping the in-browser version of this test for now and let's focus
on
> the backtracing first.
I've disabled when disable_dynamic_plugin_loading is on, which should keep this
from running with sel_ldr in chrome.
This is the use case we're actually trying to get to folks so it seems kind of
important to test it?

http://codereview.chromium.org/9316125/diff/15001/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/nacl.scons (right):

http://codereview.chromium.org/9316125/diff/15001/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:10: is_broken = (not
(env.Bit('host_windows') or env.Bit('host_mac')) or
On 2012/02/09 22:24:55, Mark Seaborn wrote:
> You can drop this because we have exception handling on all x86-32 systems
now.
Already done in later patch set.

http://codereview.chromium.org/9316125/diff/15001/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:31:
env['ENV']['NACL_UNTRUSTED_EXCEPTION_HANDLING'] = '1'
On 2012/02/09 22:24:55, Mark Seaborn wrote:
> env['ENV'] *might* be shared across all the .scons files.

BuildEnvironments clones env deeply. I've also checked this behavior by
permuting things in SConscript and trying to access in a later nacl.scons
We use this idiom thoughout (ie each .scons allowed to modify env).

http://codereview.chromium.org/9316125/diff/15001/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:47: env.AddPostAction(
On 2012/02/09 22:24:55, Mark Seaborn wrote:
> Can't this be a separate Scons target that depends on dump_file?

Done.

http://codereview.chromium.org/9316125/diff/23001/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/untrusted_crash_dump.c (right):

http://codereview.chromium.org/9316125/diff/23001/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:160: next = SafeRead(sp);
On 2012/02/09 22:24:55, Mark Seaborn wrote:
> This doesn't work for crashes in non-trivial functions where %esp no longer
> points to the return address.  You need to start off with the frame pointer,
not
> the stack pointer.  For example, see:
>
http://codereview.chromium.org/3462021/diff/6001/src/native_client/src/truste...
> 
> That means that currently you will need some inline assembly to get %ebp,
until
> we put this into the exception stack frame.
> 
> You should be able to trigger the non-trivial behaviour with something like
> this:
> 
> char *var; /* might need to declare this volatile */
> void crasher() {
>   var = alloca(100);
>   *(volatile int *) 0 = 0;
> }
Now using frame_ptr you propagated thru in other CL.

[bradn, 2012-02-13 18:55:45]

Uploaded new version with lineno -> line contents via linecache (sorry forgot
that bit in the last change).

[Mark Seaborn, 2012-02-13 19:04:08]

Can you create an issue to track this feature, please?

http://codereview.chromium.org/9316125/diff/30019/SConstruct
File SConstruct (right):

http://codereview.chromium.org/9316125/diff/30019/SConstruct#newcode3286
SConstruct:3286: 'tests/untrusted_crash_dump/nacl.scons',
I noticed your change "taking out of irt tests".  Why did you do this?  This
crash dumping ought to work with the IRT too.

http://codereview.chromium.org/9316125/diff/30019/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_main.c (right):

http://codereview.chromium.org/9316125/diff/30019/src/trusted/service_runtime...
src/trusted/service_runtime/sel_main.c:222: if
(getenv("NACL_UNTRUSTED_EXCEPTION_HANDLING") != NULL) {
If you put this immediately before "if (exception_handling_requested)" it will
keep related code more together.

http://codereview.chromium.org/9316125/diff/30019/tests/exception_test/except...
File tests/exception_test/exception_test.c (right):

http://codereview.chromium.org/9316125/diff/30019/tests/exception_test/except...
tests/exception_test/exception_test.c:305: ".p2align NACLENTRYALIGN\n"
If you could split this out into a separate changeset, it would help.  You don't
have to, but it would help make the review process go quicker since there would
be fewer things for me to keep track of.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/decode_dump.py (right):

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:30: if options.main_nexe and filename
in ['NaClMain', '(null)']:
You could comment that "NaClMain" is the argv[0] value filled out by sel_main.c.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:37: if not nmf_url:
"if nmf_url is not None"

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:41: if path:
"if path is not None"

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:67: options.addr2line, '-f',
'--inlines', '-e', filename, ('0x%08x' % addr),
Nit: Don't need brackets around '0x%08x' % addr

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:69: p = subprocess.Popen(cmd,
stdout=subprocess.PIPE)
Try to avoid one-char variable names.  e.g. 'p' -> 'proc'

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:96: options.nmf_data =
json.loads(open(options.nmf, 'r').read())
You can do json.load(open(...)) instead.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:100: core = json.loads(core_text)
Ditto.  json.load()

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:119: for info in ip_mapped['scopes']:
Just "frame['ip_mapped']['scopes']"?

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:140: usage='USAGE: %prog [options]
<core.json> <trace>')
You show two args here but check for one later.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/nacl.scons (right):

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:18: # Don't build on anything other than
x86-32 for now.
Nit: This is one of those comments that just paraphrases the code.  Instead you
could say "TODO: Add x86-64 support when we can get the frame pointer there".

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:23: def ValidateCrashDump(target=None,
source=None, env=None):
Please don't embed non-trivial test cases like this into a .scons file.  This
can go in a separate .py file which is run as a command.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:47: print >>sys.stderr, '*** EXPECTED %s
but got %s' % (
Why not raise an exception?  (Because this is in a .scons file?)

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:90: env.Depends(node, run_test)
Is this necessary, if you've declared that node depends on dump_file, which is
produced by run_test?

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:99:
env_browser.Append(CPPDEFINES=['IN_BROWER=1'])
Typo means this is presumably not needed.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:148: node, ['chrome_browser_tests',
'exception_tests'],
Don't add browser tests to exception_tests, please.

I'd rather exception_tests stay fast and just include tests for the feature
itself, not the users of the feature.  So I'd prefer if
'run_untrusted_crash_dump_test' wasn't in exception_tests either.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/untrusted_crash_dump.c (right):

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:48: struct ProgramTableData
*ptd = (struct ProgramTableData*)data;
Nit: spacing style

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:79: static void
PrintSections(FILE *core) {
'Sections' -> 'Segments'

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:86: static int
PrintMappedAddressOne(
So you have two dl_iterate_phdr() loops.  One lists all objects' segments.  The
other looks up a specific address.  Why not do the latter in the Python script
using the information dumped by the former?

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:147: if (a < 0x30000000 || a >
0x3fffffff) {
I don't think this is kicking in during your test, is it?  If not, leave it out
but put in a TODO for using exception handling to recover from bad reads.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:249: fprintf(stderr, "ADDR
%x\n", (unsigned int)CrashHandlerWrapper);
Remove debugging code

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:259: result =
NACL_SYSCALL(exception_handler)(g_PrevHandler, NULL);
FWIW, I'm not sure that attempting to restore this is necessarily helpful
behaviour.  When this pattern was applied with mismatched init/shutdown on
Windows, it led to a hole in IE: http://uninformed.org/index.cgi?v=4&a=5
("Taking Another Stab at the Unhandled Exception Filter").

It might be better just to omit this function.  It does not get called during
the test because the crash happens first.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:263: g_ProgramName = 0;
0 -> NULL

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:280: assert(guard !=
MAP_FAILED);
A stricter assertion would be
  assert(guard == stack);

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/untrusted_crash_dump.h (right):

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.h:1: /*
Can this go under 'src/untrusted/crash_dump'?  We might as well put it in the
right location to start with.  It would also make it clearer what is test code
and what is software-under-test.

[bradn, 2012-02-13 23:42:03]

Added issue for this.
PTAL

http://codereview.chromium.org/9316125/diff/30019/SConstruct
File SConstruct (right):

http://codereview.chromium.org/9316125/diff/30019/SConstruct#newcode3286
SConstruct:3286: 'tests/untrusted_crash_dump/nacl.scons',
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> I noticed your change "taking out of irt tests".  Why did you do this?  This
> crash dumping ought to work with the IRT too.
Had some trouble or other with the build, left in by accident.
Fixed.

http://codereview.chromium.org/9316125/diff/30019/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_main.c (right):

http://codereview.chromium.org/9316125/diff/30019/src/trusted/service_runtime...
src/trusted/service_runtime/sel_main.c:222: if
(getenv("NACL_UNTRUSTED_EXCEPTION_HANDLING") != NULL) {
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> If you put this immediately before "if (exception_handling_requested)" it will
> keep related code more together.

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/exception_test/except...
File tests/exception_test/exception_test.c (right):

http://codereview.chromium.org/9316125/diff/30019/tests/exception_test/except...
tests/exception_test/exception_test.c:305: ".p2align NACLENTRYALIGN\n"
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> If you could split this out into a separate changeset, it would help.  You
don't
> have to, but it would help make the review process go quicker since there
would
> be fewer things for me to keep track of.

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/decode_dump.py (right):

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:30: if options.main_nexe and filename
in ['NaClMain', '(null)']:
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> You could comment that "NaClMain" is the argv[0] value filled out by
sel_main.c.

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:37: if not nmf_url:
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> "if nmf_url is not None"

Done (you mean is None).

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:41: if path:
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> "if path is not None"
Done and changed round to make correct!

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:67: options.addr2line, '-f',
'--inlines', '-e', filename, ('0x%08x' % addr),
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> Nit: Don't need brackets around '0x%08x' % addr

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:69: p = subprocess.Popen(cmd,
stdout=subprocess.PIPE)
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> Try to avoid one-char variable names.  e.g. 'p' -> 'proc'

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:96: options.nmf_data =
json.loads(open(options.nmf, 'r').read())
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> You can do json.load(open(...)) instead.

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:100: core = json.loads(core_text)
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> Ditto.  json.load()

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:119: for info in ip_mapped['scopes']:
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> Just "frame['ip_mapped']['scopes']"?

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/decode_dump.py:140: usage='USAGE: %prog [options]
<core.json> <trace>')
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> You show two args here but check for one later.

Fixed (changed at some point to one)

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/nacl.scons (right):

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:18: # Don't build on anything other than
x86-32 for now.
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> Nit: This is one of those comments that just paraphrases the code.  Instead
you
> could say "TODO: Add x86-64 support when we can get the frame pointer there".

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:23: def ValidateCrashDump(target=None,
source=None, env=None):
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> Please don't embed non-trivial test cases like this into a .scons file.  This
> can go in a separate .py file which is run as a command.

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:47: print >>sys.stderr, '*** EXPECTED %s
but got %s' % (
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> Why not raise an exception?  (Because this is in a .scons file?)

Wanted to continue on fail, but yeah moot if not in the scons.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:90: env.Depends(node, run_test)
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> Is this necessary, if you've declared that node depends on dump_file, which is
> produced by run_test?
Indeed, done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:99:
env_browser.Append(CPPDEFINES=['IN_BROWER=1'])
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> Typo means this is presumably not needed.

Ah yeah, dropped that in the code :-)

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/nacl.scons:148: node, ['chrome_browser_tests',
'exception_tests'],
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> Don't add browser tests to exception_tests, please.
> 
> I'd rather exception_tests stay fast and just include tests for the feature
> itself, not the users of the feature.  So I'd prefer if
> 'run_untrusted_crash_dump_test' wasn't in exception_tests either.

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/untrusted_crash_dump.c (right):

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:48: struct ProgramTableData
*ptd = (struct ProgramTableData*)data;
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> Nit: spacing style

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:79: static void
PrintSections(FILE *core) {
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> 'Sections' -> 'Segments'

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:86: static int
PrintMappedAddressOne(
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> So you have two dl_iterate_phdr() loops.  One lists all objects' segments. 
The
> other looks up a specific address.  Why not do the latter in the Python script
> using the information dumped by the former?
I had originally though we might have something interesting that could be done
only from inside the module.
But on second thought, its just more that might fail.
Switched.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:147: if (a < 0x30000000 || a >
0x3fffffff) {
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> I don't think this is kicking in during your test, is it?  If not, leave it
out
> but put in a TODO for using exception handling to recover from bad reads.

Taken out.
This raises an interesting question.
If we use exception handling for this purpose, we'll either need to support a
per-thread handler, or dispatch using tls values or something similar (to be
able to temporarily wrap things per thread for the read).
When we get down to designing the api, we might want to consider how common that
sort of need is vs whether implementing per-thread stuff is surface.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:249: fprintf(stderr, "ADDR
%x\n", (unsigned int)CrashHandlerWrapper);
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> Remove debugging code

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:259: result =
NACL_SYSCALL(exception_handler)(g_PrevHandler, NULL);
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> FWIW, I'm not sure that attempting to restore this is necessarily helpful
> behaviour.  When this pattern was applied with mismatched init/shutdown on
> Windows, it led to a hole in IE: http://uninformed.org/index.cgi?v=4&a=5
> ("Taking Another Stab at the Unhandled Exception Filter").
> 
> It might be better just to omit this function.  It does not get called during
> the test because the crash happens first.

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:263: g_ProgramName = 0;
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> 0 -> NULL

Moot, gone.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.c:280: assert(guard !=
MAP_FAILED);
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> A stricter assertion would be
>   assert(guard == stack);

Done.

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/untrusted_crash_dump.h (right):

http://codereview.chromium.org/9316125/diff/30019/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump.h:1: /*
On 2012/02/13 19:04:08, Mark Seaborn wrote:
> Can this go under 'src/untrusted/crash_dump'?  We might as well put it in the
> right location to start with.  It would also make it clearer what is test code
> and what is software-under-test.

Done.

[Mark Seaborn, 2012-02-16 18:42:50]

I would OK this, but the current version seems to have some trybot failures, so
I suppose you should fix them first.

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
File src/untrusted/crash_dump/decode_dump.py (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/decode_dump.py:32: if options.main_nexe and filename in
['NaClMain', '(null)']:
"if options.main_nexe is not None ..."

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/decode_dump.py:100: for index in range(len(lines) / 2):
Nit: xrange() is preferred over range() in general (although it makes little
difference here)

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/decode_dump.py:102: afilename, lineno = lines[index * 2
+ 1].split(':')
Nit: split(':', 1) is preferable if you're expecting 2 results (although it
makes little difference here)

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/decode_dump.py:165: parser.add_option('-n', '--nmf',
dest='nmf',
dest='nmf_filename' would be a little more descriptive given that you also have
options.nmf_data.

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
File src/untrusted/crash_dump/untrusted_crash_dump.c (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/untrusted_crash_dump.c:40: uintptr_t addr;
Not used?

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/untrusted_crash_dump.c:56: fprintf(ptd->core,
"\"dlpi_name\": \"%s\",\n", info->dlpi_name);
Please put in a TODO for quoting special characters in the string.  There's a
chance we'll see backslashes in filenames since I believe an obscure operating
system uses them as a separator. :-)

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/untrusted_crash_dump.c:98: static void StackWalk(FILE
*core, uintptr_t ip, uintptr_t fp) {
'fp' -> 'frame_ptr' or 'frame'?

'ip' is also rather cryptic and suggests "IP address" too much.  In the
sel_rt_*.h headers we use "prog_ctr".  How about that?

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/untrusted_crash_dump.c:191: &g_PrevHandler);
Could use NULL instead of &g_PrevHandler now

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
File tests/untrusted_crash_dump/nacl.scons (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:8: env.Append(CCFLAGS=['-O0',
'-fno-omit-frame-pointer', '-g'])
In general gcc's debugging info is meant to work without having to disable
optimisation.  But I suppose your example function calls might get optimised
away with -O2 or -O3, which would break the test?  If so, you could comment
that.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:11: # TODO(bradnelson): Add x86-64 support
when frame point is available there.
'pointer'

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:16: env_browser = env.Clone()
Move this to just before it's used?

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:38: env.AlwaysBuild(run_test)
Isn't this unnecessary, given that the ".out" files we specify as targets are
never created?

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:52: untrusted_crash_dump_lib =
env_browser.ComponentLibrary(
Does this library need to be built a second time?  Can't you reuse the
non-browser build?

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:56: ),
Put ')' on previous line

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:57: EXTRA_LIBS=['srpc', 'platform', 'gio',
'imc', 'imc_syscalls',
'srpc' shouldn't be needed

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:64:
'${MAIN_DIR}/tests/untrusted_crash_dump/untrusted_crash_dump_test.c'),
Just use a relative path?

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:67: 'srpc', 'platform', 'gio', 'imc',
'imc_syscalls',
'srpc' shouldn't be needed

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:87: env.AlwaysBuild(run_test)
As above

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:88: env.SideEffect(crash_dump, run_test)
PPAPIBrowserTester() ought to take care of declaring this.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:92: crash_dump, untrusted_crash_dump_test,
Indent -1

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:97:
'run_inbrowser_untrusted_crash_dump_test',
Indent -3

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:100:
env_browser.Bit('disable_dynamic_plugin_loading'))
Comment disable_dynamic_plugin_loading with a TODO?

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
File tests/untrusted_crash_dump/untrusted_crash_dump.html (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/untrusted_crash_dump.html:7: <!DOCTYPE html PUBLIC
"-//W3C//DTD XHTML 1.0 Transitional//EN"
The HTML5 doctype is simpler :-)

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
File tests/untrusted_crash_dump/untrusted_crash_dump_test.c (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/untrusted_crash_dump_test.c:14: #include
"native_client/src/shared/srpc/nacl_srpc.h"
srpc #includes are not needed.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/untrusted_crash_dump_test.c:28: void layer3(int a,
int b, int c) {
Are all of these dummy arguments adding anything?  Are you trying to create
different-sized stack frames?  If so, you could comment that.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
File tests/untrusted_crash_dump/untrusted_crash_dump_test.py (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/untrusted_crash_dump_test.py:30: class Options:
If you change decode_dump to split the the parser creator into a separate
function, you can reuse that here, and pass long options to this script instead.

That would be neater than faking up an Options object.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/untrusted_crash_dump_test.py:55: lineno -= 1
The line textually before isn't necessarily where the function call was.

Maybe decode_dump should subtract 1 from return addresses before passing them to
addr2line?  I think that might be what gdb does.

This is just tweaking, though, so you don't have to do it at this point.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/untrusted_crash_dump_test.py:63: print >>sys.stderr,
(
Prefer sys.stderr.write() over print>>

https://chromiumcodereview.appspot.com/9316125/diff/40003/tools/code_hygiene.py
File tools/code_hygiene.py (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/tools/code_hygiene....
tools/code_hygiene.py:235: if 'is_untrusted_crash_dump' in props: return False
This needs a comment: i.e. we need "#ifdef __GLIBC__"

[bradn, 2012-02-16 20:13:09]

PTAL

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
File src/untrusted/crash_dump/decode_dump.py (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/decode_dump.py:32: if options.main_nexe and filename in
['NaClMain', '(null)']:
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> "if options.main_nexe is not None ..."

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/decode_dump.py:100: for index in range(len(lines) / 2):
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Nit: xrange() is preferred over range() in general (although it makes little
> difference here)

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/decode_dump.py:102: afilename, lineno = lines[index * 2
+ 1].split(':')
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Nit: split(':', 1) is preferable if you're expecting 2 results (although it
> makes little difference here)

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/decode_dump.py:165: parser.add_option('-n', '--nmf',
dest='nmf',
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> dest='nmf_filename' would be a little more descriptive given that you also
have
> options.nmf_data.

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
File src/untrusted/crash_dump/untrusted_crash_dump.c (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/untrusted_crash_dump.c:40: uintptr_t addr;
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Not used?

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/untrusted_crash_dump.c:56: fprintf(ptd->core,
"\"dlpi_name\": \"%s\",\n", info->dlpi_name);
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Please put in a TODO for quoting special characters in the string.  There's a
> chance we'll see backslashes in filenames since I believe an obscure operating
> system uses them as a separator. :-)

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/untrusted_crash_dump.c:98: static void StackWalk(FILE
*core, uintptr_t ip, uintptr_t fp) {
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> 'fp' -> 'frame_ptr' or 'frame'?
> 
> 'ip' is also rather cryptic and suggests "IP address" too much.  In the
> sel_rt_*.h headers we use "prog_ctr".  How about that?

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/src/untrusted/crash...
src/untrusted/crash_dump/untrusted_crash_dump.c:191: &g_PrevHandler);
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Could use NULL instead of &g_PrevHandler now

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
File tests/untrusted_crash_dump/nacl.scons (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:8: env.Append(CCFLAGS=['-O0',
'-fno-omit-frame-pointer', '-g'])
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> In general gcc's debugging info is meant to work without having to disable
> optimisation.  But I suppose your example function calls might get optimised
> away with -O2 or -O3, which would break the test?  If so, you could comment
> that.

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:11: # TODO(bradnelson): Add x86-64 support
when frame point is available there.
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> 'pointer'

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:16: env_browser = env.Clone()
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Move this to just before it's used?

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:38: env.AlwaysBuild(run_test)
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Isn't this unnecessary, given that the ".out" files we specify as targets are
> never created?

Scons in its infinite wisdom touches outputs (so this IS needed).
This is usually hidden as AddNodeToTestSuite calls AlwaysBuild on its first
argument.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:52: untrusted_crash_dump_lib =
env_browser.ComponentLibrary(
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Does this library need to be built a second time?  Can't you reuse the
> non-browser build?

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:56: ),
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Put ')' on previous line

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:57: EXTRA_LIBS=['srpc', 'platform', 'gio',
'imc', 'imc_syscalls',
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> 'srpc' shouldn't be needed

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:64:
'${MAIN_DIR}/tests/untrusted_crash_dump/untrusted_crash_dump_test.c'),
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Just use a relative path?

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:67: 'srpc', 'platform', 'gio', 'imc',
'imc_syscalls',
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> 'srpc' shouldn't be needed

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:87: env.AlwaysBuild(run_test)
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> As above

See above.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:88: env.SideEffect(crash_dump, run_test)
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> PPAPIBrowserTester() ought to take care of declaring this.

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:92: crash_dump, untrusted_crash_dump_test,
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Indent -1

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:97:
'run_inbrowser_untrusted_crash_dump_test',
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Indent -3

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/nacl.scons:100:
env_browser.Bit('disable_dynamic_plugin_loading'))
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Comment disable_dynamic_plugin_loading with a TODO?

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
File tests/untrusted_crash_dump/untrusted_crash_dump.html (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/untrusted_crash_dump.html:7: <!DOCTYPE html PUBLIC
"-//W3C//DTD XHTML 1.0 Transitional//EN"
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> The HTML5 doctype is simpler :-)

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
File tests/untrusted_crash_dump/untrusted_crash_dump_test.c (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/untrusted_crash_dump_test.c:14: #include
"native_client/src/shared/srpc/nacl_srpc.h"
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> srpc #includes are not needed.

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/untrusted_crash_dump_test.c:28: void layer3(int a,
int b, int c) {
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Are all of these dummy arguments adding anything?  Are you trying to create
> different-sized stack frames?  If so, you could comment that.

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
File tests/untrusted_crash_dump/untrusted_crash_dump_test.py (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/untrusted_crash_dump_test.py:30: class Options:
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> If you change decode_dump to split the the parser creator into a separate
> function, you can reuse that here, and pass long options to this script
instead.
> 
> That would be neater than faking up an Options object.

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/untrusted_crash_dump_test.py:55: lineno -= 1
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> The line textually before isn't necessarily where the function call was.
> 
> Maybe decode_dump should subtract 1 from return addresses before passing them
to
> addr2line?  I think that might be what gdb does.
> 
> This is just tweaking, though, so you don't have to do it at this point.

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tests/untrusted_cra...
tests/untrusted_crash_dump/untrusted_crash_dump_test.py:63: print >>sys.stderr,
(
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> Prefer sys.stderr.write() over print>>

Done.

https://chromiumcodereview.appspot.com/9316125/diff/40003/tools/code_hygiene.py
File tools/code_hygiene.py (right):

https://chromiumcodereview.appspot.com/9316125/diff/40003/tools/code_hygiene....
tools/code_hygiene.py:235: if 'is_untrusted_crash_dump' in props: return False
On 2012/02/16 18:42:50, Mark Seaborn wrote:
> This needs a comment: i.e. we need "#ifdef __GLIBC__"

Done.

[Mark Seaborn, 2012-02-17 01:13:40]

LGTM, though you still have some trybot failures:

scons-out/nacl_irt_test-x86-32-glibc/obj/tests/untrusted_crash_dump/inbrowser_untrusted_crash_dump_test.nexe
failed: Explicit dependency
`scons-out/nacl_irt_test-x86-32-glibc/staging/libuntrusted_crash_dump_lib.so'
not found, needed by target
`scons-out/nacl_irt_test-x86-32-glibc/obj/tests/untrusted_crash_dump/inbrowser_untrusted_crash_dump_test.nexe'.

I suspect you just need to declare the library differently.

http://codereview.chromium.org/9316125/diff/47008/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/untrusted_crash_dump_test.c (right):

http://codereview.chromium.org/9316125/diff/47008/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump_test.c:18: * Calling thru
several layers of functions, varying arguments to yield
'through'

http://codereview.chromium.org/9316125/diff/47008/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump_test.c:19: * differently size
stack frames.
'differently sized' or 'different size'

[bradn, 2012-02-17 01:28:01]

http://codereview.chromium.org/9316125/diff/47008/tests/untrusted_crash_dump/...
File tests/untrusted_crash_dump/untrusted_crash_dump_test.c (right):

http://codereview.chromium.org/9316125/diff/47008/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump_test.c:18: * Calling thru
several layers of functions, varying arguments to yield
On 2012/02/17 01:13:40, Mark Seaborn wrote:
> 'through'
Done.

Though Noah Webster is rolling in his grave:
http://en.wikipedia.org/wiki/English-language_spelling_reform

http://codereview.chromium.org/9316125/diff/47008/tests/untrusted_crash_dump/...
tests/untrusted_crash_dump/untrusted_crash_dump_test.c:19: * differently size
stack frames.
On 2012/02/17 01:13:40, Mark Seaborn wrote:
> 'differently sized' or 'different size'

Done.

[bradn, 2012-02-17 06:20:43]

Had to add json escaping after all to fix windows.
PTAL

[Mark Seaborn, 2012-02-17 17:37:28]

LGTM to changes, with a nit

http://codereview.chromium.org/9316125/diff/52045/src/untrusted/crash_dump/un...
File src/untrusted/crash_dump/untrusted_crash_dump.c (right):

http://codereview.chromium.org/9316125/diff/52045/src/untrusted/crash_dump/un...
src/untrusted/crash_dump/untrusted_crash_dump.c:42: static void JsonEscape(const
char *str, FILE *file) {
WriteJsonString() would be a little more descriptive

http://codereview.chromium.org/9316125/diff/52045/src/untrusted/crash_dump/un...
src/untrusted/crash_dump/untrusted_crash_dump.c:45: while ((ch = *str++)) {
I'd prefer to avoid assignments inside conditions, even with the double
brackets.

Maybe:
for (;;) {
  char ch = *str++;
  if (ch == 0) {
    break;
  } else if (ch == '"')
  ...
}

[bradn, 2012-02-17 18:02:34]

http://codereview.chromium.org/9316125/diff/52045/src/untrusted/crash_dump/un...
File src/untrusted/crash_dump/untrusted_crash_dump.c (right):

http://codereview.chromium.org/9316125/diff/52045/src/untrusted/crash_dump/un...
src/untrusted/crash_dump/untrusted_crash_dump.c:42: static void JsonEscape(const
char *str, FILE *file) {
On 2012/02/17 17:37:28, Mark Seaborn wrote:
> WriteJsonString() would be a little more descriptive

K, will fix in follow on CL.

http://codereview.chromium.org/9316125/diff/52045/src/untrusted/crash_dump/un...
src/untrusted/crash_dump/untrusted_crash_dump.c:45: while ((ch = *str++)) {
On 2012/02/17 17:37:28, Mark Seaborn wrote:
> I'd prefer to avoid assignments inside conditions, even with the double
> brackets.
> 
> Maybe:
> for (;;) {
>   char ch = *str++;
>   if (ch == 0) {
>     break;
>   } else if (ch == '"')
>   ...
> }

HAH, that's what I had and then changed it before upload as you've reacted badly
in the past to for(;;) and breaks :-)
New CL coming shortly.