Re: [PATCH v2 6/9] gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on)

[Simon Marchi <simon.marchi@polymtl.ca>]

>>  - In follow_fork, no need to stop all threads of the inferior, the
>>    target has stopped all threads of all its inferiors before returning
>>    the event.
>>  - In follow_fork_inferior, we record the vfork parent thread in
>>    inferior::thread_waiting_for_vfork_done.
>>  - Back in handle_inferior_event, we also call keep_going.  However, we
>>    only want to resume the event thread here, not all inferior threads.
>>    In internal_resume_ptid (called by resume_1), we therefore now check
>>    whether one of the inferiors we are about to resume has
>>    thread_waiting_for_vfork_done set.  If so, we only resume that
>>    thread.  When resuming multiple inferiors, one vforking and one not
>>    vforking, we could resume the vforking thread from the vforking
>>    inferior plus all threads from the vforking inferior.  However, the
>                                        ^
> e
> Did you forget a "non-" here?

Yes thanks.  The last vforking -> non-vforking.

>> @@ -1034,6 +1046,53 @@ handle_vfork_child_exec_or_exit (int exec)
>>      }
>>  }
>>  
>> +/* Handle TARGET_WAITKIND_VFORK_DONE.  */
>> +
>> +static void
>> +handle_vfork_done (thread_info *event_thread)
>> +{
>> +  /* We only care about this event if inferior::thread_waiting_for_vfork_done is
>> +     set, that is if we are waiting for a vfork child not under our control
>> +     (because we detached it) to exec or exit.
>> +
>> +     If an inferior has vforked and we are debugging the child, we don't use
>> +     the vfork-done event to get notified about the end of the shared address
>> +     space window).  We rely instead on the child's exec or exit event, and the
>                     ^
> 
> This closing parens has no corresponding opening one.

Removed.

>> @@ -1908,6 +1967,16 @@ start_step_over (void)
>>  	  continue;
>>  	}
>>  
>> +      if (tp->inf->thread_waiting_for_vfork_done)
> 
> Should be tp->inf->thread_waiting_for_vfork_done != nullptr

Fixed.

>> @@ -3268,7 +3383,12 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
>>  	      error (_("Command aborted."));
>>  	  }
>>        }
>> -    else if (!cur_thr->resumed () && !thread_is_in_step_over_chain (cur_thr))
>> +    else if (!cur_thr->resumed ()
>> +	     && !thread_is_in_step_over_chain (cur_thr)
>> +	     /* In non-stop, forbid resume a thread if some other thread of

I fixed resume -> resuming.

>> +		that inferior is waiting for a vfork-done event (this means
>> +		breakpoints are out for this inferior).  */
>> +	     && !(non_stop && cur_thr->inf->thread_waiting_for_vfork_done))
> 
> Should be cur_thr->inf->thread_waiting_for_vfork_done != nullptr

Fixed.

>> diff --git a/gdb/testsuite/gdb.threads/vfork-multi-thread.c b/gdb/testsuite/gdb.threads/vfork-multi-thread.c
>> new file mode 100644
>> index 000000000000..cd3dfcc1e653
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.threads/vfork-multi-thread.c
>> @@ -0,0 +1,74 @@
>> +#include <assert.h>
>> +#include <pthread.h>
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <sys/wait.h>
>> +
>> +// Start with:
>> +//
>> +//   ./gdb -nx -q --data-directory=data-directory repro -ex "tb break_here_first" -ex r -ex "b should_break_here"
>> +//
>> +// Then do "continue".
>> +//
>> +// The main thread will likely cross should_break_here while we are handling
>> +// the vfork and breakpoints are removed, therefore missing the breakpoint.
>> +
>> +static volatile int release_vfork = 0;
>> +static volatile int release_main = 0;
>> +
>> +static void *vforker(void *arg)
> 
> shouldn’t vforker shoult be at column 0, even in test code?

Looks like I completely forgot to clean up this file to make it clean
for upstream.  It's missing a copyright header and the formatting is
off.  I'll fix all I can find, including what you pointed out below.

>> +# Run the test with the given GDB settings.
>> +
>> +proc do_test { target-non-stop non-stop follow-fork-mode detach-on-fork schedule-multiple } {
>> +    save_vars { ::GDBFLAGS } {
>> +	append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${target-non-stop}\""
>> +	append ::GDBFLAGS " -ex \"set non-stop ${non-stop}\""
>> +	clean_restart ${::binfile}
>> +    }
>> +
>> +    gdb_test_no_output "set follow-fork-mode ${follow-fork-mode}"
>> +    gdb_test_no_output "set detach-on-fork ${detach-on-fork}"
>> +    gdb_test_no_output "set schedule-multiple ${schedule-multiple}"
> 
> OOC, why do you use GDBFLAGS to set 2 settings and gdb_test_no_output
> for 3 settings?

Because these settings have an effect during the clean_restart, when
using some board files.  The native-extended-gdbserver board, in
particular.  This board defines "starting GDB" as:

  - starting GDB
  - starting GDBserver with --multi
  - connecting GDB to GDBserver with extended-remote

After clean_restart, the state is that GDB is connected to GDBserver but
not debugging anything, ready to attach or run a program.  And the
non-stop and maint target-non-stop settings must be in effect before the
remote connection is set up (for example GDB sends the QNonStop packet
during the initial connection), it would be too late to set them after
the clean_restart.

Thanks for the comments.  I saved the changes locally.  Since it's all
formatting and not functional changes, I won't send a new version right
away, as I'm sure there will be more comments about the functionality
eventually.

Simon