Make nacl_helper easily debuggable

Make nacl_helper easily debuggable

This makes it so that attaching gdb to a nacl_helper process, or using gdb
on a core dump from such a process, automatically finds the symbols of the
nacl_helper executable and all the shared libraries it uses.  The theory of
operation and the details are explained in comments in the code.

BUG= http://code.google.com/p/chromium/issues/detail?id=103436
TEST= nacl still works, gdb attach on nacl_helper process, or gdb on core file from one, find symbols

R=mseaborn@chromium.org,bradchen@google.com

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110120

[eaeltsin, 2011-11-12 01:35:23]

http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_linux.cc
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_linux.c...
chrome/nacl/nacl_helper_linux.cc:151: *bootstrap_r_debug = _r_debug;
Neat.

A very useful complement would be to introduce command line flag to start
nacl_helper right inside gdb (similar to --plugin-launcher='xterm -title plugin
-e gdb --args' or  --renderer-cmd-prefix='xterm -title renderer -e gdb --args').

However, in this early attach case gdb might examine our fake _r_debug before it
is initialized. The danger is it will never examine it again, as all subsequent
examination of _r_debug happens on hidden breakpoint set on
_r_debug._r_brk=_dl_debug_state call during the first examination (in our case
first examination happens when _r_brk is not initialized).

Does the above make sense? If yes, we might need some _r_brk trick. And we might
want to call that new _dl_debug_state after we actually initialize our _r_debug
...

[eaeltsin, 2011-11-12 01:52:18]

http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_linux.cc
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_linux.c...
chrome/nacl/nacl_helper_linux.cc:151: *bootstrap_r_debug = _r_debug;
Related note: if we want to support subsequent invocations of _dl_debug_state,
we should always keep these 2 _r_debug structures we have in sync ... 

On 2011/11/12 01:35:23, eaeltsin wrote:
> Neat.
> 
> A very useful complement would be to introduce command line flag to start
> nacl_helper right inside gdb (similar to --plugin-launcher='xterm -title
plugin
> -e gdb --args' or  --renderer-cmd-prefix='xterm -title renderer -e gdb
--args').
> 
> However, in this early attach case gdb might examine our fake _r_debug before
it
> is initialized. The danger is it will never examine it again, as all
subsequent
> examination of _r_debug happens on hidden breakpoint set on
> _r_debug._r_brk=_dl_debug_state call during the first examination (in our case
> first examination happens when _r_brk is not initialized).
> 
> Does the above make sense? If yes, we might need some _r_brk trick. And we
might
> want to call that new _dl_debug_state after we actually initialize our
_r_debug
> ...

[Roland McGrath, 2011-11-14 19:21:22]

On 2011/11/12 01:52:18, eaeltsin wrote:
>
http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_linux.c...
> chrome/nacl/nacl_helper_linux.cc:151: *bootstrap_r_debug = _r_debug;
> Related note: if we want to support subsequent invocations of _dl_debug_state,
> we should always keep these 2 _r_debug structures we have in sync ... 

That is not really feasible.  Fortunately, the only part of r_debug that
varies after startup is r_state, and gdb does not use that at all.

> On 2011/11/12 01:35:23, eaeltsin wrote:
> > A very useful complement would be to introduce command line flag to start
> > nacl_helper right inside gdb (similar to --plugin-launcher='xterm -title
> plugin
> > -e gdb --args' or  --renderer-cmd-prefix='xterm -title renderer -e gdb
> --args').

We could do that too.  It doesn't belong in this same change.

> > However, in this early attach case gdb might examine our fake _r_debug
before
> it
> > is initialized. The danger is it will never examine it again, as all
> subsequent
> > examination of _r_debug happens on hidden breakpoint set on
> > _r_debug._r_brk=_dl_debug_state call during the first examination (in our
case
> > first examination happens when _r_brk is not initialized).
> > 
> > Does the above make sense? If yes, we might need some _r_brk trick. And we
> might
> > want to call that new _dl_debug_state after we actually initialize our
> _r_debug

In my experience there isn't any need for that.  GDB re-examines the list
whenever it stops for some other reason.  It just won't stop especially for
the shared library insertions.  You can step along through the startup, and
when it has gotten far enough to copy into the bootstrap _r_debug, then
"info shared" will show you the list, and you can just "share" to load symbols.

It's conceivable we could do something with a dummy r_brk, but it's unclear
how fruitful that would really be.  I'm not going to put effort into that
until there is a specific usage pattern of gdb that you can cite doesn't
work but would work if gdb stopped at r_brk.


Thanks,
Roland

[Mark Seaborn, 2011-11-14 20:15:40]

http://codereview.chromium.org/8491060/diff/1/chrome/app/nacl_fork_delegate_l...
File chrome/app/nacl_fork_delegate_linux.cc (right):

http://codereview.chromium.org/8491060/diff/1/chrome/app/nacl_fork_delegate_l...
chrome/app/nacl_fork_delegate_linux.cc:34: const char kNaClHelperRDebug[] =
"--r_debug=0xXXXXXXXXXXXXXXXX";
Maybe comment: passing this argument with a template here saves us from having
to reorganise the contents of the stack in nacl_helper_bootstrap_linux.c.

http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_bootstr...
File chrome/nacl/nacl_helper_bootstrap_linux.c (right):

http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_bootstr...
chrome/nacl/nacl_helper_bootstrap_linux.c:37: static const size_t
kRDebugPrefixLen = sizeof R_DEBUG_TEMPLATE_PREFIX - 1;
consistency: elsewhere you use sizeof() (i.e. with brackets).

http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_bootstr...
chrome/nacl/nacl_helper_bootstrap_linux.c:385: if (!my_strcmp(arg,
kRDebugTemplate)) {
prefer "strcmp(x,y) == 0" since strcmp()'s result is not a boolean.

http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_bootstr...
File chrome/nacl/nacl_helper_bootstrap_linux.x (right):

http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_bootstr...
chrome/nacl/nacl_helper_bootstrap_linux.x:51: r_debug PT_LOAD;
Should you put FLAGS?  What's the default?  It doesn't seem to be in the docs. 
Does it default to inferring the flags from the sections?

http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_linux.cc
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_linux.c...
chrome/nacl/nacl_helper_linux.cc:151: *bootstrap_r_debug = _r_debug;
Is the symbol _r_debug part of the public ABI such that ld.so is guaranteed to
always export it?

http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_linux.c...
chrome/nacl/nacl_helper_linux.cc:165: l->l_name = argv0;
I find modifying the dynamic linker's data structures like this more scary than
just copying them to a location that gdb is expected to look at. :-(

[Roland McGrath, 2011-11-14 21:32:09]

On 2011/11/14 20:15:40, Mark Seaborn wrote:
>
http://codereview.chromium.org/8491060/diff/1/chrome/app/nacl_fork_delegate_l...
> chrome/app/nacl_fork_delegate_linux.cc:34: const char kNaClHelperRDebug[] =
> "--r_debug=0xXXXXXXXXXXXXXXXX";
> Maybe comment: passing this argument with a template here saves us from having
> to reorganise the contents of the stack in nacl_helper_bootstrap_linux.c.

I put the comment in nacl_helper_bootstrap_linux.c itself.

>
http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_bootstr...
> chrome/nacl/nacl_helper_bootstrap_linux.c:37: static const size_t
> kRDebugPrefixLen = sizeof R_DEBUG_TEMPLATE_PREFIX - 1;
> consistency: elsewhere you use sizeof() (i.e. with brackets).

Done.

>
http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_bootstr...
> chrome/nacl/nacl_helper_bootstrap_linux.c:385: if (!my_strcmp(arg,
> kRDebugTemplate)) {
> prefer "strcmp(x,y) == 0" since strcmp()'s result is not a boolean.

Done.

>
http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_bootstr...
> chrome/nacl/nacl_helper_bootstrap_linux.x:51: r_debug PT_LOAD;
> Should you put FLAGS?  What's the default?  It doesn't seem to be in the docs.

> Does it default to inferring the flags from the sections?

It defaults to inferring it, which is what we want.

>
http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_linux.c...
> chrome/nacl/nacl_helper_linux.cc:151: *bootstrap_r_debug = _r_debug;
> Is the symbol _r_debug part of the public ABI such that ld.so is guaranteed to
> always export it?

Yes.

>
http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_linux.c...
> chrome/nacl/nacl_helper_linux.cc:165: l->l_name = argv0;
> I find modifying the dynamic linker's data structures like this more scary
than
> just copying them to a location that gdb is expected to look at. :-(

Copying all those data structures is infeasibly fragile and ill-advised.
This is the only practical solution.

[Mark Seaborn, 2011-11-14 21:57:12]

LGTM.  It's a shame hacks like this are required to make debugging work.

On 14 November 2011 13:32, <mcgrathr@chromium.org> wrote:

> http://codereview.chromium.**org/8491060/diff/1/chrome/**
>
nacl/nacl_helper_linux.cc#**newcode165<http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_linux.cc#newcode165>
>
>> chrome/nacl/nacl_helper_linux.**cc:165: l->l_name = argv0;
>> I find modifying the dynamic linker's data structures like this more
>> scary than
>>
> just copying them to a location that gdb is expected to look at. :-(
>>
>
> Copying all those data structures is infeasibly fragile and ill-advised.
> This is the only practical solution.
>

I wasn't suggesting copying it recursively, just saying that modifying
ld.so's data structures is dodgy.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Mark Seaborn, 2011-11-14 21:57:14]

LGTM.  It's a shame hacks like this are required to make debugging work.

On 14 November 2011 13:32, <mcgrathr@chromium.org> wrote:

> http://codereview.chromium.**org/8491060/diff/1/chrome/**
>
nacl/nacl_helper_linux.cc#**newcode165<http://codereview.chromium.org/8491060/diff/1/chrome/nacl/nacl_helper_linux.cc#newcode165>
>
>> chrome/nacl/nacl_helper_linux.**cc:165: l->l_name = argv0;
>> I find modifying the dynamic linker's data structures like this more
>> scary than
>>
> just copying them to a location that gdb is expected to look at. :-(
>>
>
> Copying all those data structures is infeasibly fragile and ill-advised.
> This is the only practical solution.
>

I wasn't suggesting copying it recursively, just saying that modifying
ld.so's data structures is dodgy.

Mark