Hooks for out-of-process (OOP) debugger.

Hooks in sel_ldr for out-of-process (OOP) debugger.

Only Windows hooks are implemented,
placeholders are provided for other platforms.

bug=none
test=manual:

1) change line win\nacl_oop_debugger_hooks.c:53
if (IsDebuggerPresent()) {  ->   if (true) {
2) build chrome
3) start dbgview.exe (download it from: http://technet.microsoft.com/en-us/sysinternals/bb896647)
4) start custom chrome (from step #2)
5) navigate to tha page with nexe
6) in dbgview you should get something like this:
11.62454796 [6060] {7AA7C9CF-89EC-4ed3-8DAD-6DC84302AB11} -version 1 -event AppCreate -nap 00000000001CD3F0 -mem_start 0000000C00000000 -user_entry_pt 0000000000020080 -initial_entry_pt 0000000008000080

11.62480354 [6060] {7AA7C9CF-89EC-4ed3-8DAD-6DC84302AB11} -version 1 -event ThreadCreate -natp 0000000002304C50

7) navigate off the page
8) in dbgview you should get something like this:
23.96814728 [6060] {7AA7C9CF-89EC-4ed3-8DAD-6DC84302AB11} -version 1 \
    -event ThreadExit -natp 0000000002304C50 -exit_code 1

23.96838570 [6060] {7AA7C9CF-89EC-4ed3-8DAD-6DC84302AB11} -version 1 \
    -event AppExit -exit_code 1
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=5639

[Mark Seaborn, 2011-06-13 19:40:51]

Some initial reactions.

You need to fill out "BUG" and "TEST" fields.  How would I test this change?

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
File src/trusted/service_runtime/nacl_app_thread.c (right):

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_app_thread.c:146:
natp->nap->debug_stub_callbacks->thread_exit_hook(natp);
There's already a hook facility right here.  Can't you reuse this in some way?

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
File src/trusted/service_runtime/nacl_oop_debugger_hooks.c (right):

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:9: #ifdef _WIN32 /* _WIN32
is defined on both 32-bit and 64-bit Windows */
Use NACL_WINDOWS instead.

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:25: (void*)nap->mem_start,
Style in this component is "(void *) foo".

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:67: #define kVarMsgSize
2048
Use "const int" (local) or "static const int" (global)

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:68: /* Determines whether
the calling process is being debugged by a debugger.
Style here is
/*
 * comment...
 */

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:78: char* post_pref_msg =
msg + sizeof(prefix) - 1;
Style here is "char *foo".

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
File src/trusted/service_runtime/nacl_oop_debugger_hooks.h (right):

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.h:1: /* Copyright (c) 2011
The Native Client Authors. All rights reserved.
Style here is:
/*
 * comment
 */

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.h:12: void
NaClOopDebuggerAppCreateHook(struct NaClApp* nap);
Style here is "struct Foo *foo".

[garianov1, 2011-06-13 20:09:10]

PTAL, added service_runtime.gyp

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
File src/trusted/service_runtime/nacl_app_thread.c (right):

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_app_thread.c:146:
natp->nap->debug_stub_callbacks->thread_exit_hook(natp);
On 2011/06/13 19:40:51, Mark Seaborn wrote:
> There's already a hook facility right here.  Can't you reuse this in some way?

My intention is to enable OOP debugger hooks all the time,
no flag is needed. At least on Windows, hooks code is short,
and seems safe to me.
Also, if we need to put these hooks under a flag, then it should be a different
flag, so that both in-process and
out-of-process debuggers can coexists (at least for now).

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
File src/trusted/service_runtime/nacl_oop_debugger_hooks.c (right):

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:9: #ifdef _WIN32 /* _WIN32
is defined on both 32-bit and 64-bit Windows */
On 2011/06/13 19:40:51, Mark Seaborn wrote:
> Use NACL_WINDOWS instead.

Done.

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:25: (void*)nap->mem_start,
On 2011/06/13 19:40:51, Mark Seaborn wrote:
> Style in this component is "(void *) foo".

Done.

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:67: #define kVarMsgSize
2048
On 2011/06/13 19:40:51, Mark Seaborn wrote:
> Use "const int" (local) or "static const int" (global)

Unfortunately, if I use 'const int', the following code
fails to compile:
char msg[sizeof(prefix) + kVarMsgSize];
>error C2057: expected constant expression

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:68: /* Determines whether
the calling process is being debugged by a debugger.
On 2011/06/13 19:40:51, Mark Seaborn wrote:
> Style here is
> /*
>  * comment...
>  */

Done.

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:78: char* post_pref_msg =
msg + sizeof(prefix) - 1;
On 2011/06/13 19:40:51, Mark Seaborn wrote:
> Style here is "char *foo".

Done.

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
File src/trusted/service_runtime/nacl_oop_debugger_hooks.h (right):

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.h:1: /* Copyright (c) 2011
The Native Client Authors. All rights reserved.
On 2011/06/13 19:40:51, Mark Seaborn wrote:
> Style here is:
> /*
>  * comment
>  */

Done.

http://codereview.chromium.org/6995160/diff/4003/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.h:12: void
NaClOopDebuggerAppCreateHook(struct NaClApp* nap);
On 2011/06/13 19:40:51, Mark Seaborn wrote:
> Style here is "struct Foo *foo".

Done.

[garianov1, 2011-06-13 20:14:04]

On 2011/06/13 19:40:51, Mark Seaborn wrote:
> Some initial reactions.
> 
> You need to fill out "BUG" and "TEST" fields.  How would I test this change?

I can file a bug 'nacl needs OOP debugger on Windows".
How to test it? I think only integration test is possible,
this code is not doing much without OOP debugger present :(
And I can't test OOP debugger without sel_ldr hooks!

[bsy, 2011-06-13 20:36:49]

http://codereview.chromium.org/6995160/diff/13/src/trusted/service_runtime/na...
File src/trusted/service_runtime/nacl_oop_debugger_hooks.c (right):

http://codereview.chromium.org/6995160/diff/13/src/trusted/service_runtime/na...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:1: /* Copyright (c) 2011
The Native Client Authors. All rights reserved.
/*
 * as before
 */

http://codereview.chromium.org/6995160/diff/13/src/trusted/service_runtime/na...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:9: #ifdef NACL_WINDOWS
this file has enough OS-conditionalizing that it is probably better to put
OS-specific copies in the per-OS directories.

http://codereview.chromium.org/6995160/diff/13/src/trusted/service_runtime/na...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:17: void
SendMessageToDebuggerAndHalt(const char *fmt, ...);
suggest adding ATTRIBUTE_FORMAT_PRINTF from
native_client/src/include/nacl_compiler_annotations.h so that in the eventuality
that MSVS gains printf format checks we can enable it in one place.

http://codereview.chromium.org/6995160/diff/13/src/trusted/service_runtime/na...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:26: (void
*)nap->mem_start,
prevailing style is "(target_type) expression" (space between cast and
expression).

[garianov1, 2011-06-13 20:52:32]

PTAL

http://codereview.chromium.org/6995160/diff/13/src/trusted/service_runtime/na...
File src/trusted/service_runtime/nacl_oop_debugger_hooks.c (right):

http://codereview.chromium.org/6995160/diff/13/src/trusted/service_runtime/na...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:1: /* Copyright (c) 2011
The Native Client Authors. All rights reserved.
On 2011/06/13 20:36:49, bsy wrote:
> /*
>  * as before
>  */

Done.

http://codereview.chromium.org/6995160/diff/13/src/trusted/service_runtime/na...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:9: #ifdef NACL_WINDOWS
On 2011/06/13 20:36:49, bsy wrote:
> this file has enough OS-conditionalizing that it is probably better to put
> OS-specific copies in the per-OS directories.

Right now I have only Windows code, so I'd like to keep other
platforms stubs here. Once I have hooks for linux,
I will create files in platf-spec directories.

http://codereview.chromium.org/6995160/diff/13/src/trusted/service_runtime/na...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:17: void
SendMessageToDebuggerAndHalt(const char *fmt, ...);
On 2011/06/13 20:36:49, bsy wrote:
> suggest adding ATTRIBUTE_FORMAT_PRINTF from
> native_client/src/include/nacl_compiler_annotations.h so that in the
eventuality
> that MSVS gains printf format checks we can enable it in one place.

Done.

http://codereview.chromium.org/6995160/diff/13/src/trusted/service_runtime/na...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:26: (void
*)nap->mem_start,
On 2011/06/13 20:36:49, bsy wrote:
> prevailing style is "(target_type) expression" (space between cast and
> expression).

Done.

[garianov1, 2011-06-13 21:02:14]

> this file has enough OS-conditionalizing that it is probably better to put
> OS-specific copies in the per-OS directories.

I do feel the same now. Please wait till I move it to win directory
and add I will add linux & mac stubs too.

[garianov1, 2011-06-13 21:16:22]

PTAL

[bsy, 2011-06-13 21:23:42]

http://codereview.chromium.org/6995160/diff/3006/src/trusted/service_runtime/...
File src/trusted/service_runtime/nacl_oop_debugger_hooks.c (right):

http://codereview.chromium.org/6995160/diff/3006/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:81: char prefix[] =
"{7AA7C9CF-89EC-4ed3-8DAD-6DC84302AB11} -version 1 ";
shouldn't this be char const prefix[] = "..."?  should the prefix bytes be
writable?

http://codereview.chromium.org/6995160/diff/3006/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:82: char
msg[sizeof(prefix) + kVarMsgSize];
note that trusted stack size used to be 32KB in size and was only recently
increased to 64KB.  a 2KB plus buffer automatic variable may cause problems if
this function is invoked at a bad place -- though it appears that the API isn't
intended to be called from sites that are nested too deeply in the syscall
processing.  Perhaps a comment in the .h file?

http://codereview.chromium.org/6995160/diff/3006/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:89: res =
_vsnprintf_s(post_pref_msg, kVarMsgSize, _TRUNCATE, fmt, marker);
msg is sizeof(prefix) + kVarMsgSize in size, but post_pref_msg is at the null
byte in prefix, or prefix + sizeof(prefix)-1.  (a "string" initializer for an
array allocates space for the terminating ASCII NUL too, so sizeof(prefix) ==
strlen(prefix)+1.)  so the available space -- for the sizeOfBuffer argument of
_vsnprintf_s -- is really kVarMsgSize+1.  asking for fewer usually isn't bad --
the suffix to the GUID is at most 2047 characters before the terminating ASCII
NUL, instead of 2048, and the API doesn't advertise kVarMsgSize anyway -- but
this appears to be a minor error.

http://codereview.chromium.org/6995160/diff/3006/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:108: }
should this abort if the buffer couldn't hold the message?  log?

[bsy, 2011-06-13 21:25:17]

ooops.  race condition in reviewing.  :-)  i added more comments to previous
rev, but they should apply to the post-move version too.  -bsy

On Mon, Jun 13, 2011 at 2:02 PM, <garianov@chromium.org> wrote:

> this file has enough OS-conditionalizing that it is probably better to put
>> OS-specific copies in the per-OS directories.
>>
>
> I do feel the same now. Please wait till I move it to win directory
> and add I will add linux & mac stubs too.
>
>
>
http://codereview.chromium.**org/6995160/<http://codereview.chromium.org/6995...
>



-- 
bennet s yee
i usually don't capitalize due to mild tendonitis

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

[garianov1, 2011-06-13 21:41:06]

PTAL

http://codereview.chromium.org/6995160/diff/3006/src/trusted/service_runtime/...
File src/trusted/service_runtime/nacl_oop_debugger_hooks.c (right):

http://codereview.chromium.org/6995160/diff/3006/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:81: char prefix[] =
"{7AA7C9CF-89EC-4ed3-8DAD-6DC84302AB11} -version 1 ";
On 2011/06/13 21:23:42, bsy wrote:
> shouldn't this be char const prefix[] = "..."?  should the prefix bytes be
> writable?

Done.

http://codereview.chromium.org/6995160/diff/3006/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:82: char
msg[sizeof(prefix) + kVarMsgSize];
On 2011/06/13 21:23:42, bsy wrote:
> note that trusted stack size used to be 32KB in size and was only recently
> increased to 64KB.  a 2KB plus buffer automatic variable may cause problems if
> this function is invoked at a bad place -- though it appears that the API
isn't
> intended to be called from sites that are nested too deeply in the syscall
> processing.  Perhaps a comment in the .h file?

Changed kVarMsgSize to 512, it should be plenty stll.

http://codereview.chromium.org/6995160/diff/3006/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:89: res =
_vsnprintf_s(post_pref_msg, kVarMsgSize, _TRUNCATE, fmt, marker);
On 2011/06/13 21:23:42, bsy wrote:
> msg is sizeof(prefix) + kVarMsgSize in size, but post_pref_msg is at the null
> byte in prefix, or prefix + sizeof(prefix)-1.  (a "string" initializer for an
> array allocates space for the terminating ASCII NUL too, so sizeof(prefix) ==
> strlen(prefix)+1.)  so the available space -- for the sizeOfBuffer argument of
> _vsnprintf_s -- is really kVarMsgSize+1.  asking for fewer usually isn't bad
--
> the suffix to the GUID is at most 2047 characters before the terminating ASCII
> NUL, instead of 2048, and the API doesn't advertise kVarMsgSize anyway -- but
> this appears to be a minor error.

Done.

http://codereview.chromium.org/6995160/diff/3006/src/trusted/service_runtime/...
src/trusted/service_runtime/nacl_oop_debugger_hooks.c:108: }
On 2011/06/13 21:23:42, bsy wrote:
> should this abort if the buffer couldn't hold the message?  log?

Sending partial message is probably better than aborting.

[bsy, 2011-06-13 22:49:58]

one nit.  o/w lgtm.

http://codereview.chromium.org/6995160/diff/9001/src/trusted/service_runtime/...
File src/trusted/service_runtime/win/nacl_oop_debugger_hooks.c (right):

http://codereview.chromium.org/6995160/diff/9001/src/trusted/service_runtime/...
src/trusted/service_runtime/win/nacl_oop_debugger_hooks.c:85: }
so with count == _TRUNCATE and post_pref_msg non-NULL, the only way that res ==
-1 might hold appears to be if fmt==NULL.  this is probably serious enough that
a LOG_FATAL is warranted, rather than ignoring it.  what do we know about the
invalid parameter handler?  i don't think we set it; does chrome?  in any case,
it's good defense-in-depth to LOG_FATAL out in case the invalid parameter
handler returns rather than (or after) triggering SEH/VEH.

[garianov1, 2011-06-14 15:55:57]

http://codereview.chromium.org/6995160/diff/9001/src/trusted/service_runtime/...
File src/trusted/service_runtime/win/nacl_oop_debugger_hooks.c (right):

http://codereview.chromium.org/6995160/diff/9001/src/trusted/service_runtime/...
src/trusted/service_runtime/win/nacl_oop_debugger_hooks.c:85: }
On 2011/06/13 22:49:58, bsy wrote:
> so with count == _TRUNCATE and post_pref_msg non-NULL, the only way that res
==
> -1 might hold appears to be if fmt==NULL.  this is probably serious enough
that
> a LOG_FATAL is warranted, rather than ignoring it.  what do we know about the
> invalid parameter handler?  i don't think we set it; does chrome?  in any
case,
> it's good defense-in-depth to LOG_FATAL out in case the invalid parameter
> handler returns rather than (or after) triggering SEH/VEH.

Done.

[garianov1, 2011-06-14 16:50:13]

committed as rev 5639