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

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

On Friday, April 06 2018, Pedro Alves wrote:

> On 04/05/2018 07:46 PM, Sergio Durigan Junior wrote:
>> Changes from v3:
>> 
>> - Revisited commit log and fixed wrong copy&paste from GDB output.
>> 
>> - Use target_pid_to_str where applicable.  Use
>>   gdb::unique_xmalloc_ptr<char> to save results from
>>   target_pid_to_str in some cases.
>
> Arguably target_pid_to_str implementations should be
> using get_print_cell instead of a single static buffer, avoiding
> the problem.  But that's a larger change, so local copy is fine
> with me.

I can send a patch for this later.

>> @@ -2598,8 +2598,16 @@ kill_command (const char *arg, int from_tty)
>>      error (_("The program is not being run."));
>>    if (!query (_("Kill the program being debugged? ")))
>>      error (_("Not confirmed."));
>> +
>> +  const char *pid_str = target_pid_to_str (inferior_ptid);
>> +  int infnum = current_inferior ()->num;
>> +
>>    target_kill ();
>>  
>> +  if (print_inferior_events)
>> +    printf_unfiltered (_("[Inferior %d (process %s) has been killed]\n"),
>> +		       infnum, pid_str);
>
> This still seem risky -- Target backends use target_pid_to_str inside
> target_kill, e.g., when logging is enabled.  E.g., 
>
> linux_nat_kill -> -> stop_callback -> target_pid_to_str
>
> ISTM a deep copy like:
>
>   std::string pid_str = target_pid_to_str (inferior_ptid);
>
> would be safer/better.

Fair enough, I'll change it.

>>  /* The Current Inferior.  This is a strong reference.  I.e., whenever
>>     an inferior is the current inferior, its refcount is
>> @@ -123,7 +122,8 @@ add_inferior (int pid)
>>    struct inferior *inf = add_inferior_silent (pid);
>>  
>>    if (print_inferior_events)
>> -    printf_unfiltered (_("[New inferior %d]\n"), pid);
>> +    printf_unfiltered (_("[New inferior %d (process %d)]\n"),
>> +		       inf->num, pid);
>
> As discussed in a previous revision, using hardcoded "(process %d)"
> doesn't work properly, because PID can be a fake PID number, or the
> target may have no concept of processes, e.g., when debugging remote
> targets.  ISTRM this should use target_pid_to_str as well.

That's true, I apologize for overlooking this one.  And I also noticed
the problem with including the "process" string before, that's why I
sent the message "cancelling" this patch.

>
>> @@ -266,7 +263,8 @@ detach_inferior (inferior *inf)
>>    exit_inferior_1 (inf, 0);
>>  
>>    if (print_inferior_events)
>> -    printf_unfiltered (_("[Inferior %d detached]\n"), pid);
>> +    printf_unfiltered (_("[Inferior %d (process %d) detached]\n"),
>> +		       inf->num, pid);
>
> Ditto.

Got it.

>> -      if (info_verbose || debug_infrun)
>> +      if (print_inferior_events)
>>  	{
>> +	  gdb::unique_xmalloc_ptr<char>
>> +	    parent_pid (xstrdup (target_pid_to_str (parent_ptid)));
>> +	  gdb::unique_xmalloc_ptr<char>
>> +	    child_pid (xstrdup (target_pid_to_str (child_ptid)));
>
> std::string would be simpler than gdb::unique_xmalloc_ptr here:
>
> 	  std::string parent_pid = target_pid_to_str (parent_ptid));
> 	  std::string child_pid = target_pid_to_str (child_ptid));
>
> gdb::unique_xmalloc_ptr is handy when you have no way around
> malloc/free, but here you're in charge of the dup yourself.

That's true.  I guess I wasn't thinking in C++ terms.

>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 68c43f8312..4cb4badd8a 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);
>
> Hardcoded "(process %d)" here too.
>
> Fixing this issue in the several spots may affect your
> testsuite changes -- please be sure to rerun tests with 
> "target remote" afterwards.

I'm doing it, thanks.

>
>> +    }
>>    else
>>      {
>
>>  }
>>  
>> diff --git a/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp b/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp
>> index e05acb1711..616b6cf7a4 100644
>> --- a/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp
>> +++ b/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp
>> @@ -81,8 +81,15 @@ proc detach_and_expect_exit {inf_output_re test} {
>>      global inferior_spawn_id
>>      global gdb_prompt
>>  
>> +    set saw_inf_exit 0
>>      return_if_fail [gdb_test_multiple "detach" $test {
>> -	-re "Detaching from .*, process $decimal" {
>> +	-re "Detaching from .*, process $decimal\r\n\\\[Inferior $decimal \\(process $decimal\\) detached\\\]" {
>> +	}
>> +	# inf_output_re can also appear in the middle, so we catch
>> +	# this case here in order to avoid racy results.
>> +	-re "Detaching from .*, process $decimal\r\n${inf_output_re}\r\n\\\[Inferior $decimal \\(process $decimal\\) detached\\\]" {
>> +	    verbose -log "saw inferior exit"
>> +	    set saw_inf_exit 1
>>  	}
>
> I'm not sure I understand the need for this.  If you left this
> gdb_test_multiple exactly as it was before your patch, wouldn't it all
> work the same?

As I said in the other message, the problem here is that
${inf_output_re} can happen between the two messages.  For example:

  Detaching from program: .../gdb/testsuite/outputs/gdb.threads/process-dies-while-detaching/process-dies-while-detaching-1-detach, process 7440
  exited, status=0
  [Inferior 1 (process 7440) detached]

In this case, leave gdb_test_multiple as it was before doesn't catch
this case, which leads to a racy failure.  However, I noticed that my
patch also doesn't fix the failure (I thought it did, but then I saw it
happening again on the BuildBot).  That's another reason why I
"cancelled" this version of the patch.

I'm trying to come up with another solution for this race, but so far I
haven't had much success.  Of course, if you have any ideas feel free to
suggest them.  Otherwise, I'll see what I can do during the weekend (I'm
on PTO today).

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/