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

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Monday, March 26 2018, Pedro Alves wrote:

> On 03/09/2018 09:55 PM, Sergio Durigan Junior wrote:
>
>> But if we use 'add_thread_silent' (with the same configuration as
>> before):
>> 
>>   (gdb) run
>>   Starting program: a.out
>>   [Attaching after process 26807 fork to child process 26807.]
>>   [New inferior 26811]
>>   [Detaching after fork from child process 26811.]
>>   [Inferior 26807 detached]
>>   [Inferior 2 (process 26811) exited normally]
>
> I still think the "inferior PID" messages are misleading,
> because PID is not the inferior number.  I think it would be
> better if they read something like:
>
>     [Attaching after process 26807 fork to child process 26807.]
> -   [New inferior 26811]
> +   [New inferior 2 (process 26811)]
>     [Detaching after fork from child process 26811.]
> -   [Inferior 26807 detached]
> +   [Inferior 1 (process 26807) detached]
>     [Inferior 2 (process 26811) exited normally]
>
> I.e.:
>
>     [Attaching after process 26807 fork to child process 26807.]
>     [New inferior 2 (process 26811)]
>     [Detaching after fork from child process 26811.]
>     [Inferior 1 (process 26807) detached]
>     [Inferior 2 (process 26811) exited normally]
>
> Please consider fixing that at the same time.

Sorry, I believe I forgot to update this part of the commit message.
The current patch already fixes this problem, so we actually see:

  [Attaching after process 19317 fork to child process 19317]                                    
  [New inferior 2 (process 19321)]               
  [Detaching after fork from parent process 19317]                                               
  [Inferior 1 (process 19317) detached]          
  [Inferior 2 (process 19321) exited normally]   

I'll make sure to update the commit log.

>> @@ -2598,8 +2598,14 @@ 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."));
>> +  pid_t pid = ptid_get_pid (inferior_ptid);
>> +  int infnum = current_inferior ()->num;
>>    target_kill ();
>>  
>> +  if (print_inferior_events)
>> +    printf_unfiltered (_("[Inferior %d (process %d) has been killed]\n"),
>> +		       infnum, pid);
>
> The "process %d" part should be using target_pid_to_str instead.
> Not all targets have the concept of a "process" (e.g., when remote
> debugging a bare metal system), or have access to the actual pid
> number (older gdbservers).  In such case, the above prints
> the fake internal magic process id (magic_null_ptid).  E.g.:
>
>  $ ./gdb -q --batch -ex "set remote multiprocess-feature-packet off" -ex "tar rem :9999" -ex "info inferiors" -ex "kill"
>    Num  Description       Executable        
>  * 1    Remote target     gdb/binutils-gdb/build/gdb/gdbserver/gdbserver
>         ^^^^^^^^^^^^^
>  Kill the program being debugged? (y or n)
>  [Inferior 1 (process 42000) has been killed]
>               ^^^^^^^^^^^^^
>
> You'll need to capture the target_pid_to_str output
3> before killing, otherwise after target_kill the target
> might no longer be pushed on the target stack.
>
> This pattern appears in several places in the patch.  
>
> The fork-related paths should be fine to print inf->pid directly,
> since fork implies support for multiple processes.

Thanks, I'll fix this.

>
>> diff --git a/gdb/testsuite/gdb.base/catch-syscall.exp b/gdb/testsuite/gdb.base/catch-syscall.exp
>> index 2a8bf27e5c..20fa041155 100644
>> --- a/gdb/testsuite/gdb.base/catch-syscall.exp
>> +++ b/gdb/testsuite/gdb.base/catch-syscall.exp
>> @@ -179,7 +179,7 @@ proc check_for_program_end {} {
>>      # Deleting the catchpoints
>>      delete_breakpoints
>>  
>> -    gdb_continue_to_end
>> +    gdb_continue_to_end "" continue 1
>
> Changes like this appear several times in the patch -- can you
> expand on why they're needed?

They actually appear twice, both on catch-syscall.exp.  They're needed
because now we print the following line during the test:

  (gdb) continue
  Continuing.
  [Detaching after vfork from child process 22282] <--- THIS LINE
  [Inferior 1 (process 22281) exited normally]

And therefore we need to inform the gdb_continue_to_end procedure to
expect extra cruft after the "continue" command is issued.

>> +
>> +if { [use_gdb_stub] } {
>> +    untested "not supported on gdbserver"
>
> There should be a comment above this mentioning what
> wouldn't work with gdbserver, since it's not obvious to
> me.  (I think we may have gone through that in a previous
> iteration, but it'd be better if the reason was written
> down here).
>
> Note: it's not "on gdbserver", it's on "target remote" stubs.
> gdbserver+extended-remote works.  And there are stubs others
> than gdbserver.

Alright, here's what I added:

  # This test relies on "run", so it cannot run on target remote stubs.
  if { [use_gdb_stub] } {
      untested "not supported on target remote stubs"
      return
  }

>> diff --git a/gdb/testsuite/gdb.threads/process-dies-while-detaching.c b/gdb/testsuite/gdb.threads/process-dies-while-detaching.c
>> index b0fd84b483..1871f6cf81 100644
>> --- a/gdb/testsuite/gdb.threads/process-dies-while-detaching.c
>> +++ b/gdb/testsuite/gdb.threads/process-dies-while-detaching.c
>> @@ -80,6 +80,8 @@ parent_function (pid_t child)
>>    alarm (300);
>>  
>>    ret = waitpid (child, &status, 0);
>> +  /* Give a chance to GDB print its messages.  */
>> +  usleep (100);
>>  
>
> This is probably racy and I'd a prefer a better fix that
> avoids it.  Why did you need it?  What/how does the failure
> look like without this?  Why does it happen?

You're right, this is racy, now that I see it again I understand it
won't necessarily work in all cases.

This was needed because of the following situation:

  (gdb) detach
  Detaching from program: /dir/gdb/testsuite/outputs/gdb.threads/process-dies-while-detaching/process-dies-while-detaching-1-detach, process 25193
  signaled, sig=9  <--- THIS LINE
  [Inferior 1 (process 25193) detached]
  (gdb) FAIL: gdb.threads/process-dies-while-detaching.exp: multi-process: detach: killed outside: detach parent

As you can see, the line marked as "THIS LINE" above is sometimes
printed in the middle of GDB's output, which confuses dejagnu.  It's
important to say that this is also racy: the failure doesn't happen
every time the test is run.

I guess a "better" solution would be to expect extra output between the
"Detaching..." and "[Inferior...]" messages.  This would mean more
complicated regexps, though.

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