Re: [PATCH v6] Enable 'set print inferior-events' and improve detach/fork/kill/exit messages

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Tuesday, April 17 2018, Pedro Alves wrote:

> On 04/16/2018 09:04 PM, Sergio Durigan Junior wrote:
>> Changes from v5:
>> 
>> - Save result from 'target_pid_to_str' using std::string instead of
>>   'const char *', avoiding a possible race condition.
>
> Sorry Sergio, we're getting there, but this will need more work.

I'm the one who apologizes.

>> Built and regtested on BuildBot, without regressions.
>> 
>
>> 	* gdb.threads/process-dies-while-detaching.c
>> 	(parent_function): Use 'usleep' in order to avoid
>> 	race-conditions.
>
> This is stale.  There's no 'usleep' any more.

Removed.

>> @@ -121,9 +120,11 @@ struct inferior *
>>  add_inferior (int pid)
>>  {
>>    struct inferior *inf = add_inferior_silent (pid);
>> +  std::string infpid = target_pid_to_str (pid_to_ptid (pid));
>>  
>>    if (print_inferior_events)
>> -    printf_unfiltered (_("[New inferior %d]\n"), pid);
>> +    printf_unfiltered (_("[New inferior %d (%s)]\n"),
>> +		       inf->num, infpid.c_str ());
>
> Let's call target_pid_to_str directly here without the
> deep copy.
>
> You needed to call target_pid_to_str early in kill_command
> because there first kill the target, and print the
> string _afterwards_.  "target_kill" may pop the target from the
> target stack, so we can't use target_pid_to_str after the
> target is gone already.
>
> But here, we're not messing with the target stack.
>
> (Nor are we calling target_pid_to_str twice in a row in
> the same expression.)

I confess I knew this, but decided to do a deep copy anyway because I
was aiming for consistency.  But no problem at all, I'll simplify this
part.

>
>>  void
>> @@ -261,12 +259,13 @@ void
>>  detach_inferior (inferior *inf)
>>  {
>>    /* Save the pid, since exit_inferior_1 will reset it.  */
>> -  int pid = inf->pid;
>> +  std::string infpid = target_pid_to_str (pid_to_ptid (inf->pid));
>>  
>>    exit_inferior_1 (inf, 0);
>>  
>>    if (print_inferior_events)
>> -    printf_unfiltered (_("[Inferior %d detached]\n"), pid);
>> +    printf_unfiltered (_("[Inferior %d (%s) detached]\n"),
>> +		       inf->num, infpid.c_str ());
>
> Same here.

Fixed.

>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index f54a38833b..be118c5d97 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -5137,7 +5137,12 @@ remote_detach_1 (int from_tty, inferior *inf)
>>    /* If doing detach-on-fork, we don't mourn, because that will delete
>>       breakpoints that should be available for the followed inferior.  */
>>    if (!is_fork_parent)
>> -    target_mourn_inferior (inferior_ptid);
>> +    {
>> +      target_mourn_inferior (inferior_ptid);
>> +      if (print_inferior_events)
>> +	printf_unfiltered (_("[Inferior %d (process %d) detached]\n"),
>> +			   inf->num, pid);
>
> This is still using "(process %d)", which is incorrect
> for target remote.

You're right, sorry about this.

> Also, here, if debugging in "target remote" mode, and this is 
> the last live process, that target_mourn_inferior call will
> unpush the remote target from the target stack.  So this is another
> case where you need to call target_pid_to_str _before_ calling
> target_mourn_inferior.

Good point.  This is fixed.

> As mentioned before, not all targets print "process $decimal" in
> their target_pid_to_str implementation.  So your testsuite changes
> will need to adapt too.
>
> Please try this: hack away gdbserver/server.c's support for
> the RSP multi-process extensions (look for "multiprocess+"), and
> test with "target remote".  That hack will emulate older gdbservers and
> other stubs that don't support telling gdb the process's pid.
> Manually confirm that you get "Remote target" instead of
> "process $pid" -- see remote.c's implementation of
> target_pid_to_str.  Then run the testsuite against that gdbserver.

[ After a quick chat to clarify things...  ]

I will make sure to adjust all regexps and take into account the many
outputs that can happen here.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/