Initial commit for i#95. Detach appears to work, but cleanup is still very broken. THis commit exis…

Reviewer: zhaoqin@google.com
Initial commit for i#95. Detach appears to work, but cleanup is still very broken. THis commit exists to get reviews on the approach, coding style, missing things, potential bugs, etc.

Detach appears to work, but cleanup remains in an inconsistent, broken state.


Initial commit of i#95 for detaching. Working on getting the general structure of detaching right.

[Peter Goodman, 2013-09-12 15:13:58]

https://codereview.appspot.com/13314047/diff/1/core/dispatch.c
File core/dispatch.c (right):

https://codereview.appspot.com/13314047/diff/1/core/dispatch.c#newcode562
core/dispatch.c:562: # ifdef DR_APP_EXPORTS
Obviously I need to guard for UNIX here as well. Whoops!

https://codereview.appspot.com/13314047/diff/1/core/dispatch.c#newcode624
core/dispatch.c:624: # ifdef DR_APP_EXPORTS
Obviously I need to guard for UNIX here as well. Whoops!

https://codereview.appspot.com/13314047/diff/1/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/13314047/diff/1/core/unix/os.c#newcode3453
core/unix/os.c:3453: /* FIXME i#95: Prevent multiple simultaneous detached using
a
Out-of-date comment.

https://codereview.appspot.com/13314047/diff/1/core/unix/os.c#newcode3483
core/unix/os.c:3483: /* NOTE : we will release the locks grabbed in
synch_with_all_threads
Comment might be out of date.

https://codereview.appspot.com/13314047/diff/1/core/unix/signal.c
File core/unix/signal.c (right):

https://codereview.appspot.com/13314047/diff/1/core/unix/signal.c#newcode1091
core/unix/signal.c:1091: 
TODO: Restore app signal handlers. Was not quite sure of the right way to do
this. I think it should be a sort of "opposite" of intercept_signal, as it done
in signal_init.

[Peter Goodman, 2013-09-12 16:11:28]

Reviewer: zhaoqin@google.com
Minor updates, haven't yet tested on Windows.

Added some extra ifdefs, made it so that dr_app_cleanup doesn't process exit
when detach was done. So, big memory leak atm, but runsuite seems to be passing
on Linux.


Detach appears to work, but cleanup remains in an inconsistent, broken state.


Initial commit of i#95 for detaching. Working on getting the general structure
of detaching right.

[Peter Goodman, 2013-09-12 18:09:39]

Reviewer: zhaoqin@google.com
Minor update.

Minor ifdef check.


Added some extra ifdefs, made it so that dr_app_cleanup doesn't process exit
when detach was done. So, big memory leak atm, but runsuite seems to be passing
on Linux.


Detach appears to work, but cleanup remains in an inconsistent, broken state.


Initial commit of i#95 for detaching. Working on getting the general structure
of detaching right.

[bruening, 2013-09-13 05:18:44]

https://codereview.appspot.com/13314047/diff/1/core/unix/signal.c
File core/unix/signal.c (right):

https://codereview.appspot.com/13314047/diff/1/core/unix/signal.c#newcode1091
core/unix/signal.c:1091: 
On 2013/09/12 15:13:58, Peter Goodman wrote:
> TODO: Restore app signal handlers. Was not quite sure of the right way to do
> this. I think it should be a sort of "opposite" of intercept_signal, as it
done
> in signal_init.

Invoking signal_thread_exit() will do this via signal_info_exit_sigaction()

[zhaoqin, 2013-09-18 21:12:46]

some comment

https://codereview.appspot.com/13314047/diff/11001/core/dispatch.c
File core/dispatch.c (right):

https://codereview.appspot.com/13314047/diff/11001/core/dispatch.c#newcode629
core/dispatch.c:629: }
why not do the os_detach_all_threads in dispatch_at_stopping_point but here, any
reason it must be after enter_nolinking, if so, add comment to explain.

https://codereview.appspot.com/13314047/diff/11001/core/dynamo.c
File core/dynamo.c (right):

https://codereview.appspot.com/13314047/diff/11001/core/dynamo.c#newcode2527
core/dynamo.c:2527: if (doing_detach) {
add XXX comment about why we return here temporarily.

https://codereview.appspot.com/13314047/diff/11001/core/os_shared.h
File core/os_shared.h (right):

https://codereview.appspot.com/13314047/diff/11001/core/os_shared.h#newcode1069
core/os_shared.h:1069: bool doing_detach;
missing extern?
I am not sure if it is a good idea to declare a variable in header file.

also missing #ifdef DR_APP_EXPORTS?

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode1064
core/unix/os.c:1064: signal_exit(false /* restore_app_signals */);
you mean signal handler

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3423
core/unix/os.c:3423: /* not static only for a few asserts in other files */
the comment kind of strange, maybe /* used in ... also */

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3441
core/unix/os.c:3441: and since no one ever waits on it we can't deadlock on it
either */
comment style

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3444
core/unix/os.c:3444: return false;
so if some thread came later, it will return and call
dynamo_thread_not_under_dynamo(dcontext), is it ok?

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3475
core/unix/os.c:3475: * problems) ignore it. FIXME: retry instead? */
style

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3481
core/unix/os.c:3481: * it during process exit cleanup */
style, */ start a new line

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3491
core/unix/os.c:3491: hotp_only_detach_helper();
style, no comment out code

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3519
core/unix/os.c:3519: *             translate_mcontext? This might avoid lock
precedence
you mean violate?

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3525
core/unix/os.c:3525: &wrapper);
no comment out code

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3540
core/unix/os.c:3540: signal_exit(true /* restore_app_signals */);
can you explain why we do signal_exit now not after end_synch_all?

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3560
core/unix/os.c:3560: LOG(GLOBAL, LOG_ALL, 1, "Detach : Starting to clean up
thread state.\n");
why extra space

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3562
core/unix/os.c:3562: /* FIXME i#95: Clean up per-thread state. */
Clean => clean

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3568
core/unix/os.c:3568: /* FIXME i#95: Unload DR? */
I do not think we need unload DR, right, especially for static linked.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os_private.h
File core/unix/os_private.h (right):

https://codereview.appspot.com/13314047/diff/11001/core/unix/os_private.h#new...
core/unix/os_private.h:179: void signal_exit(bool restore_app_signals);
you mean signal handler?

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c
File core/unix/signal.c (right):

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:1094: */
add ASSERT_NOT_IMPLEMENTED

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:3189: ASSERT(!for_app IF_UNIX(|| doing_detach));
we are in unix folder, do we really need IF_UNIX?

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:6444: *             the app stack.
the alignment is strange, left aligned should be fine.

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:6459: *             to the base of the stack.
alignment

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:6461: call_switch_stack((dcontext_t *) &detach_info, xsp,
sig_detach_go_native,
add comment why we case (dcontext_t *), fool compiler?

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:6567: } else if (ostd->detach != NULL && !(*ostd->detach)) {
why *ostd->detach could be true.
If not, should it in ASSERT instead?

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:6568: sig_detach(dcontext, frame, ostd->detach);  /* no
return */
nit: move /* no return */ to next line

[Peter Goodman, 2013-09-19 16:27:19]

https://codereview.appspot.com/13314047/diff/11001/core/dispatch.c
File core/dispatch.c (right):

https://codereview.appspot.com/13314047/diff/11001/core/dispatch.c#newcode629
core/dispatch.c:629: }
On 2013/09/18 21:12:46, zhaoqin wrote:
> why not do the os_detach_all_threads in dispatch_at_stopping_point but here,
any
> reason it must be after enter_nolinking, if so, add comment to explain.

Originally I put it there because I thought that I would be doing the segment
selector restoring of the master thread in or after os_detach_all_threads, so I
wanted to make sure the enter_nolinking would continue to work.

There might have also been a locking concern, but that might have not been real
because dispatch_enter_native is called with no locks held.

https://codereview.appspot.com/13314047/diff/11001/core/dynamo.c
File core/dynamo.c (right):

https://codereview.appspot.com/13314047/diff/11001/core/dynamo.c#newcode2527
core/dynamo.c:2527: if (doing_detach) {
On 2013/09/18 21:12:46, zhaoqin wrote:
> add XXX comment about why we return here temporarily. 

Done.

https://codereview.appspot.com/13314047/diff/11001/core/os_shared.h
File core/os_shared.h (right):

https://codereview.appspot.com/13314047/diff/11001/core/os_shared.h#newcode1069
core/os_shared.h:1069: bool doing_detach;
On 2013/09/18 21:12:46, zhaoqin wrote:
> missing extern?
> I am not sure if it is a good idea to declare a variable in header file.
> 
> also missing #ifdef DR_APP_EXPORTS?

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode1064
core/unix/os.c:1064: signal_exit(false /* restore_app_signals */);
On 2013/09/18 21:12:46, zhaoqin wrote:
> you mean signal handler

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3423
core/unix/os.c:3423: /* not static only for a few asserts in other files */
On 2013/09/18 21:12:46, zhaoqin wrote:
> the comment kind of strange, maybe /* used in ... also */

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3441
core/unix/os.c:3441: and since no one ever waits on it we can't deadlock on it
either */
On 2013/09/18 21:12:46, zhaoqin wrote:
> comment style

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3444
core/unix/os.c:3444: return false;
On 2013/09/18 21:12:46, zhaoqin wrote:
> so if some thread came later, it will return and call
> dynamo_thread_not_under_dynamo(dcontext), is it ok?

I am not sure, I guess that depends on the extent of the cleanup done by
dynamo_thread_not_under_dynamo. If dynamo_thread_not_under_dynamo restores app
signal handlers, then the synch all might not work. Or, the synch all just might
do something weird because the thread count is one less. I don't want a thread
doing dr_app_stop to remain attached if it contends with another one doing the
same, so perhaps the best way forward would be to put it into some state, e.g.
just before or just after the dr_app_stop and then put it to sleep. Clearly this
case it worth further discussion.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3475
core/unix/os.c:3475: * problems) ignore it. FIXME: retry instead? */
On 2013/09/18 21:12:46, zhaoqin wrote:
> style

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3481
core/unix/os.c:3481: * it during process exit cleanup */
On 2013/09/18 21:12:46, zhaoqin wrote:
> style, */ start a new line

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3491
core/unix/os.c:3491: hotp_only_detach_helper();
On 2013/09/18 21:12:46, zhaoqin wrote:
> style, no comment out code

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3519
core/unix/os.c:3519: *             translate_mcontext? This might avoid lock
precedence
On 2013/09/18 21:12:46, zhaoqin wrote:
> you mean violate?

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3525
core/unix/os.c:3525: &wrapper);
On 2013/09/18 21:12:46, zhaoqin wrote:
> no comment out code

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3540
core/unix/os.c:3540: signal_exit(true /* restore_app_signals */);
On 2013/09/18 21:12:46, zhaoqin wrote:
> can you explain why we do signal_exit now not after end_synch_all?

Can't explain :-P Likely didn't make sense. Moved to below
end_synch_with_all_threads.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3560
core/unix/os.c:3560: LOG(GLOBAL, LOG_ALL, 1, "Detach : Starting to clean up
thread state.\n");
On 2013/09/18 21:12:46, zhaoqin wrote:
> why extra space

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3562
core/unix/os.c:3562: /* FIXME i#95: Clean up per-thread state. */
On 2013/09/18 21:12:46, zhaoqin wrote:
> Clean => clean

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os.c#newcode3568
core/unix/os.c:3568: /* FIXME i#95: Unload DR? */
On 2013/09/18 21:12:46, zhaoqin wrote:
> I do not think we need unload DR, right, especially for static linked.

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/os_private.h
File core/unix/os_private.h (right):

https://codereview.appspot.com/13314047/diff/11001/core/unix/os_private.h#new...
core/unix/os_private.h:179: void signal_exit(bool restore_app_signals);
On 2013/09/18 21:12:46, zhaoqin wrote:
> you mean signal handler?

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c
File core/unix/signal.c (right):

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:1094: */
On 2013/09/18 21:12:46, zhaoqin wrote:
> add ASSERT_NOT_IMPLEMENTED

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:3189: ASSERT(!for_app IF_UNIX(|| doing_detach));
On 2013/09/18 21:12:46, zhaoqin wrote:
> we are in unix folder, do we really need IF_UNIX?

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:6444: *             the app stack.
On 2013/09/18 21:12:46, zhaoqin wrote:
> the alignment is strange, left aligned should be fine.

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:6459: *             to the base of the stack.
On 2013/09/18 21:12:46, zhaoqin wrote:
> alignment

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:6461: call_switch_stack((dcontext_t *) &detach_info, xsp,
sig_detach_go_native,
On 2013/09/18 21:12:46, zhaoqin wrote:
> add comment why we case (dcontext_t *), fool compiler?

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:6567: } else if (ostd->detach != NULL && !(*ostd->detach)) {
On 2013/09/18 21:12:46, zhaoqin wrote:
> why *ostd->detach could be true.
> If not, should it in ASSERT instead?

Done.

https://codereview.appspot.com/13314047/diff/11001/core/unix/signal.c#newcode...
core/unix/signal.c:6568: sig_detach(dcontext, frame, ostd->detach);  /* no
return */
On 2013/09/18 21:12:46, zhaoqin wrote:
> nit: move /* no return */ to next line

Really? I was following the lead of line 6564:
sig_take_over(sc);  /* no return */

:-P

[bruening, 2013-09-19 16:29:13]

https://codereview.appspot.com/13314047/diff/11001/core/dispatch.c
File core/dispatch.c (right):

https://codereview.appspot.com/13314047/diff/11001/core/dispatch.c#newcode629
core/dispatch.c:629: }
As synch_with_all_threads() says:
 * NOTE - Requires the caller is !could_be_linking (i.e. not in an 
 * enter_couldbelinking state)

[Peter Goodman, 2013-09-30 20:18:43]

Reviewer: bruening@google.com
Fixes issue 95 for UNIX. Implemented attach/detach all.

Implements dr_app_stop_all_and_detach (i#95) for UNIX, and partially for
Windows.
  1) Windows re-attach doesn't work; currently deadlocks because of an not
cleaned
     up mutex contended event.
  2) New startstopall UNIX test case for added API.

[bruening, 2013-10-08 18:03:53]

Main concern is the code duplication when it looks like it can be shared.

Typo in commit msg.

Commit msg should perhaps mention that we may move away from supporting detach +
reattach, as we'll switch stopall to mean "go native all" and un-tie it from
cleanup as discussed, in the future.

https://codereview.appspot.com/13314047/diff/24001/core/config.c
File core/config.c (right):

https://codereview.appspot.com/13314047/diff/24001/core/config.c#newcode695
core/config.c:695: memset(&config, 0, sizeof config);
This is a non-trivial performance hit as this struct is pretty big.  We don't
want to pay this cost on the regular release-build exit path.  Can we only do
this when detaching?  Perhaps should have a comment about reverting if we switch
away from stop-plus-cleanup as discussed.

https://codereview.appspot.com/13314047/diff/24001/core/dispatch.c
File core/dispatch.c (right):

https://codereview.appspot.com/13314047/diff/24001/core/dispatch.c#newcode578
core/dispatch.c:578: * Then there might be the case that the itimer refcount
never reaches 0.
How about removing this and instead avoiding the other call, then?

https://codereview.appspot.com/13314047/diff/24001/core/dynamo.c
File core/dynamo.c (right):

https://codereview.appspot.com/13314047/diff/24001/core/dynamo.c#newcode974
core/dynamo.c:974: * thread.
?  Are you assuming that "toexit" is the current thread?  If so, please add an
assert and comment up top.

https://codereview.appspot.com/13314047/diff/24001/core/dynamo.c#newcode2556
core/dynamo.c:2556: dr_app_stop_all_and_cleanup(void)
There should be a XXX comment that we're going to get rid of this routine, per
discussion (i#95-c#12), and replace with separate stop-all and cleanup.

https://codereview.appspot.com/13314047/diff/24001/core/dynamo.c#newcode2559
core/dynamo.c:2559: detach_helper(IF_WINDOWS(DETACH_RESUME_TYPE));
detach_internal() is the exposed interface.  On Windows it creates a new thread
to do the detach, for ease of cleanup.  If this is going to call the non-exposed
interface it needs some explanation, and probably the name should be changed to
make it an exposed interface: "detach_internal_not_on_dstack()"?

https://codereview.appspot.com/13314047/diff/24001/core/dynamo.c#newcode2559
core/dynamo.c:2559: detach_helper(IF_WINDOWS(DETACH_RESUME_TYPE));
On Windows this assumes it's on a new thread which it can kill (note how
detach_helper kills the current thread).  That clearly won't work here.  Xref
all the confusion and discussion around start/stop with stop being really a "go
native" and not a "detach".

https://codereview.appspot.com/13314047/diff/24001/core/dynamo.c#newcode2562
core/dynamo.c:2562: * re-attach. */
style: */

https://codereview.appspot.com/13314047/diff/24001/core/dynamo.c#newcode2562
core/dynamo.c:2562: * re-attach. */
Comment that this is incomplete and is one reason we're not going to keep
supporting this model

https://codereview.appspot.com/13314047/diff/24001/core/dynamo.c#newcode2571
core/dynamo.c:2571: IF_UNIX( dynamo_exiting = false; )
This var is there on Windows too.  Why is it in IF_UNIX?

https://codereview.appspot.com/13314047/diff/24001/core/heap.c
File core/heap.c (right):

https://codereview.appspot.com/13314047/diff/24001/core/heap.c#newcode1668
core/heap.c:1668: DEBUG_DECLARE( heapmgt = &temp_heapmgt; )
This is not the initial state.  The current state is memcpy-ed into this struct
a few lines above.

This needs a comment as to why it's here.  It's rather odd to do this only in
debug build.  So this whole detach+reattach only works in debug build?  That
should be in the commit message.  Needs explanation, and probably a XXX to
remove this once stop-all is decoupled from cleanup.

https://codereview.appspot.com/13314047/diff/24001/core/lib/dr_app.h
File core/lib/dr_app.h (right):

https://codereview.appspot.com/13314047/diff/24001/core/lib/dr_app.h#newcode100
core/lib/dr_app.h:100: * \warning This function is unimplemented on Windows.
Needs a user-visible comment that this routine is temporary and we plan to
remove it in the near future.

https://codereview.appspot.com/13314047/diff/24001/core/optionsx.h
File core/optionsx.h (right):

https://codereview.appspot.com/13314047/diff/24001/core/optionsx.h#newcode1860
core/optionsx.h:1860: /* FIXME: since we have dynamic options this option can be
false for most of the time,
Can you change this to XXX as it's not a fatal issue

https://codereview.appspot.com/13314047/diff/24001/core/os_shared.h
File core/os_shared.h (right):

https://codereview.appspot.com/13314047/diff/24001/core/os_shared.h#newcode1073
core/os_shared.h:1073: extern bool doing_detach;
Why aren't detach_internal and the DETACH_BAD_STATE* enum here too?  And
detach_internal_not_on_dstack() or whatever we rename the other internal (as in,
not via nudge from another process) interface to.

It seems good to implement DETACH_BAD_STATE*, etc. for Linux as well as Windows.

https://codereview.appspot.com/13314047/diff/24001/core/unix/loader.c
File core/unix/loader.c (right):

https://codereview.appspot.com/13314047/diff/24001/core/unix/loader.c#newcode213
core/unix/loader.c:213: * re-attaching and os_loader_init_epilogue().
It doesn't seem like this is enough.  There are several other global vars; a
buffer, a sofar var.  As you have it, it will append to the old buffer, with
incorrect results.  The criterion of "no asserts" is not sufficient of a test to
prove you've properly re-initialized everything.

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c
File core/unix/os.c (right):

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode1404
core/unix/os.c:1404: /* Thread-specific TLS exit. */
style: no punctuation unless it's a complete sentence

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode1406
core/unix/os.c:1406: os_tls_exit_thread(local_state_t *local_state) {
all other thread exit routines are named "thread_exit", not "exit_thread", so
how about reversing here to match that convention?

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode1410
core/unix/os.c:1410: (((byte*)local_state) - offsetof(os_local_state_t, state));
why not just pass in os_tls instead of local_state

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2621
core/unix/os.c:2621: /* Not static only for a few asserts in other files. Used
in dynamo.c, signal.c,
style: global vars or types should be either at the top of the file or with a
separator comment.  how about a banner comment separator here.

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2624
core/unix/os.c:2624: bool doing_detach = false;
Very similar to win32/os.c declaration

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2631
core/unix/os.c:2631: DECLARE_CXTSWPROT_VAR(static volatile int
dynamo_detaching_flag, LOCK_FREE_STATE);
Duplicates win32/os.c decl, down to the comment

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2648
core/unix/os.c:2648: * on it we can't deadlock on it either.
More duplication with identical comment

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2662
core/unix/os.c:2662: SELF_UNPROTECT_DATASEC(DATASEC_RARELY_PROT);
More duplication

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2674
core/unix/os.c:2674: return;
This is all duplicated.  This much duplication should be avoided.  There's not
even a comment saying "XXX: this is all copy-pasted from win32/os.c and we
really should share it."

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2721
core/unix/os.c:2721: #endif
More duplication, down to exact comments.  All of this should be shared.  It
seems like it should be easy to share the first half of this routine.

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2727
core/unix/os.c:2727: /* FIXME i#95: Call
dynamo_process_exit_with_thread_info()??? */
Yes.  I would share the Windows code, which calls that.  It says in its comment
that it's meant for the detach path too.

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2742
core/unix/os.c:2742: DEBUG_DECLARE(ok =) thread_get_mcontext(synched_threads[i],
&mc);
style: put release-build code on separate line to aid readability (seeing
DEBUG_DECLARE at start makes it too easy to assume whole line is debug).  See
all the existing code: it puts it on a separate line.

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2744
core/unix/os.c:2744: DEBUG_DECLARE(ok =) translate_mcontext(synched_threads[i],
&mc,
ditto

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2749
core/unix/os.c:2749: DEBUG_DECLARE(ok =) thread_set_mcontext(synched_threads[i],
&mc);
ditto

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2763
core/unix/os.c:2763: /* Busy wait on all threads to mark their detach status
fields as
Busy wait?  Why not use a condition var just like os_thread_suspend() and others
use, which uses a futex?  It's no more work, it avoids the busy wait, and it
matches the existing code instead of adding additional conventions of status
codes and condition variables.

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2766
core/unix/os.c:2766: * FIXME i#95: Implement this as a futex.
You have a lot of fixmes reference i#95, but I don't see any corresponding
entries or list of todo items in i#95.

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2778
core/unix/os.c:2778: thread_sleep(10);
Should be a simple matter to switch to a futex (would have been easier than
writing this code I would think), right?  If doing any sleep,  use
DYNAMO_OPTION(synch_with_sleep_time) like the existing code does.

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2813
core/unix/os.c:2813: 
what about dynamo_initialized = false?

https://codereview.appspot.com/13314047/diff/24001/core/unix/os.c#newcode2817
core/unix/os.c:2817: do_once_generation++; /* Increment the generation in case
we re-attach */
Worth commenting that this would go away w/ the separation of stop from cleanup,
for start/stop interface.

https://codereview.appspot.com/13314047/diff/24001/core/unix/os_exports.h
File core/unix/os_exports.h (right):

https://codereview.appspot.com/13314047/diff/24001/core/unix/os_exports.h#new...
core/unix/os_exports.h:104: detach_helper(void);
see earlier comment about naming this more appropriately and moving to
os_shared.h

https://codereview.appspot.com/13314047/diff/24001/core/unix/os_private.h
File core/unix/os_private.h (right):

https://codereview.appspot.com/13314047/diff/24001/core/unix/os_private.h#new...
core/unix/os_private.h:55: # define ASM_SUFFIX "q"
The name is not clear enough.  Suffix for what?

https://codereview.appspot.com/13314047/diff/24001/core/unix/os_private.h#new...
core/unix/os_private.h:61: # define ASM_SUFFIX ""
If it's a pointer-sized suffix (name should be made clearer) why wouldn't it be
"l"?

https://codereview.appspot.com/13314047/diff/24001/core/unix/os_private.h#new...
core/unix/os_private.h:80: } thread_detach_status_t;
See earlier comments on using futex-ready condition variables, like all the
other cross-thread synch in use by DR.

https://codereview.appspot.com/13314047/diff/24001/core/unix/os_private.h#new...
core/unix/os_private.h:127: volatile thread_detach_status_t detach_status; /*
for detach */
Ditto.

https://codereview.appspot.com/13314047/diff/24001/core/unix/signal.c
File core/unix/signal.c (right):

https://codereview.appspot.com/13314047/diff/24001/core/unix/signal.c#newcode...
core/unix/signal.c:1746: * we might reattach.
?  This doesn't make sense to me.  The conditional above is whether DR installed
a sigstack (basically always true).  If DR did not, then the app's sigstack is
still in place, so why do you need to make this syscall?

https://codereview.appspot.com/13314047/diff/24001/core/unix/signal.c#newcode...
core/unix/signal.c:3193: ASSERT(!for_app || doing_detach);
Why do you need this?  Why is a signal sent while detaching any different? 
Needs a comment.

https://codereview.appspot.com/13314047/diff/24001/core/unix/signal.c#newcode...
core/unix/signal.c:6358: static void sig_detach_go_native(dcontext_t *info_)
style: follow the naming conventions of the project -- every single routine
calls a dcontext_t "dcontext"

https://codereview.appspot.com/13314047/diff/24001/core/unix/signal.c#newcode...
core/unix/signal.c:6360: struct sig_detach_info *info = (struct sig_detach_info
*) info_;
??  How can you make this cast?

https://codereview.appspot.com/13314047/diff/24001/core/unix/signal.c#newcode...
core/unix/signal.c:6366: DEBUG_DECLARE(int rc =)
sigaltstack_syscall(info->app_sigstack, NULL);
style: separate lines

https://codereview.appspot.com/13314047/diff/24001/core/unix/signal.c#newcode...
core/unix/signal.c:6390: thread_sig_info_t *info = (thread_sig_info_t
*)dcontext->signal_field;
style: in the code base most casts that are standalone statements have a space
after )

https://codereview.appspot.com/13314047/diff/24001/core/unix/signal.c#newcode...
core/unix/signal.c:6402: sizeof info->app_sigblocked);
style: all existing uses of sizeof use parens

https://codereview.appspot.com/13314047/diff/24001/core/unix/signal.c#newcode...
core/unix/signal.c:6406: * the app stack.
Can we get a list of all these in the issue

https://codereview.appspot.com/13314047/diff/24001/core/unix/signal.c#newcode...
core/unix/signal.c:6422: detach_info.app_sigstack = &(info->app_sigstack);
Worth a comment on why you can't restore this now

https://codereview.appspot.com/13314047/diff/24001/core/unix/signal.c#newcode...
core/unix/signal.c:6434: call_switch_stack((dcontext_t *) &detach_info, xsp,
sig_detach_go_native,
Just change call_switch_stack to take in "void *param".  It does not assume
anything about the param, just passes it through.  That seems much cleaner than
this to me, as it results in very confusing code up above.  FTR I believe I
mentioned this in the past.

https://codereview.appspot.com/13314047/diff/24001/core/utils.h
File core/utils.h (right):

https://codereview.appspot.com/13314047/diff/24001/core/utils.h#newcode1211
core/utils.h:1211: extern int do_once_generation;
Put a comment explaining why you need this and that it can be reverted if we
remove support for detach + re-attach and instead switch to go-native w/o
cleanup.

https://codereview.appspot.com/13314047/diff/24001/core/win32/ntdll.c
File core/win32/ntdll.c (right):

https://codereview.appspot.com/13314047/diff/24001/core/win32/ntdll.c#newcode652
core/win32/ntdll.c:652: nt_wrappers_intercepted = true;
Needs a comment as to why we would do this.

This is called on the fast path so we want as little as possible here.  Perhaps
the set_ntdll_base should be moved down here and all should be grouped under a
comment about reverting if we change from stop_all_and_cleanup.

https://codereview.appspot.com/13314047/diff/24001/core/win32/ntdll.c#newcode929
core/win32/ntdll.c:929: static PEB *own_peb = NULL;
It doesn't seem like we need to do this -- the PEB isn't going anywhere, and the
code was cleaner before with this in the function scope and without this extra
complexity.

https://codereview.appspot.com/13314047/diff/24001/core/win32/os.c
File core/win32/os.c (right):

https://codereview.appspot.com/13314047/diff/24001/core/win32/os.c#newcode1115
core/win32/os.c:1115: tls_dcontext_offs = TLS_UNINITIALIZED;
Comment as to why

https://codereview.appspot.com/13314047/diff/24001/core/win32/os.c#newcode7677
core/win32/os.c:7677: }
else assert it's not on dstack

https://codereview.appspot.com/13314047/diff/24001/core/win32/os_exports.h
File core/win32/os_exports.h (right):

https://codereview.appspot.com/13314047/diff/24001/core/win32/os_exports.h#ne...
core/win32/os_exports.h:405: DETACH_RESUME_TYPE          = -3,
Comment what this assumes: on app stack and not on separate thread

https://codereview.appspot.com/13314047/diff/24001/core/x86/interp.c
File core/x86/interp.c (right):

https://codereview.appspot.com/13314047/diff/24001/core/x86/interp.c#newcode2845
core/x86/interp.c:2845: bb->start_pc == instr_get_raw_bits(mov)));
Is this a fix for a different bug?  If so, let's commit it separately.

https://codereview.appspot.com/13314047/diff/24001/suite/tests/api/startstopa...
File suite/tests/api/startstopall.c (right):

https://codereview.appspot.com/13314047/diff/24001/suite/tests/api/startstopa...
suite/tests/api/startstopall.c:167: print("ERROR: should not be under DynamoRIO
before dr_app_setup_and_start!\n");
style: line too long

https://codereview.appspot.com/13314047/diff/24001/suite/tests/api/startstopa...
suite/tests/api/startstopall.c:182: print("ERROR: should be under DynamoRIO
before dr_app_stop_all_and_cleanup!\n");
style: line too long

https://codereview.appspot.com/13314047/diff/24001/suite/tests/api/startstopa...
suite/tests/api/startstopall.c:185: print("ERROR: should not be under DynamoRIO
after dr_app_stop_all_and_cleanup!\n");
style: line too long

[bruening, 2013-10-08 18:06:27]

On 2013/10/08 18:03:53, bruening wrote:
> It seems good to implement DETACH_BAD_STATE*, etc. for Linux as well as
Windows.

I meant this as "in the future"