Re: [PATCH v2 2/3] Target remote mode fork and exec events

[Pedro Alves <palves at redhat dot com>]

On 12/07/2015 10:14 PM, Don Breazeal wrote:

> On 11/20/2015 5:04 AM, Pedro Alves wrote:

>>> Note that follow-exec-mode is not supported, because target
>>> remote mode does not support the 'run' command.
>>
>> Not sure I don't understand this part/comment.
> 
> As I noted in my previous reply, this was an overly-pessimistic way of
> putting it.  I revised the docs etc to say something like this:
> "follow-exec-mode is of limited used in target remote mode, since the 'run'
> command is not supported."  The point being that the main reason a user
> would want to set follow-exec-mode would be to get control over which
> inferior is run when a 'run' command is executed.  It is supported in
> that the inferiors are created or not created as specified, and one could
> (for example) use follow-exec-mode to keep a record of the programs that
> were executed.

Right.  Note that we could support "run" with "target remote", as long
as the remote side supports vRun.  The problem with simply enabling
"run" is that:

 (gdb) target remote
 ...
 (gdb) run
 The program being debugged has been started already.
 Start it from the beginning? (y or n)

... saying yes will kill the inferior and close the connection, before
there's a chance to start the new process...


But, with "set detach-on-fork off", say the program forks, and then the child
execs, and then it exits.  The parent is still running so the "target remote"
connection is still up.  Switch to the child inferior (inferior 2), and
issue "run".  I can see people wondering why that doesn't work.

Likewise the simpler:

 (gdb) target remote ...
 *inferior 1 is now bound to progress N (the only process) running on the target*
 (gdb) add-inferior
 *inferior 2 added*
 (gdb) inferior 2
 (gdb) run

Should we allow that "run" to actually succeed in this case?



((((
Actually trying the detach-on-fork scenario above, we see very
confusing behavior (this is with your series, but I think
current master will have similar problems):

$ gdb -ex "tar rem :9999"
0x0000003615a011f0 in _start () from target:/lib64/ld-linux-x86-64.so.2
(gdb) set detach-on-fork off
(gdb) c
Continuing.
Reading /lib64/libm.so.6 from remote target...
Reading /lib64/libc.so.6 from remote target...
[New Thread 32274.32274]
Reading /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork from remote target...
Reading /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork from remote target...
Reading /lib64/libm.so.6 from remote target...
Reading /lib64/libc.so.6 from remote target...
Reading /lib64/ld-linux-x86-64.so.2 from remote target...
Reading /lib64/ld-linux-x86-64.so.2 from remote target...
[Inferior 1 (process 32258) exited normally]
Reading /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork from remote target...
(gdb) info inferiors
  Num  Description       Executable
* 1    <null>            target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
  2    process 32274     target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
(gdb) maint print target-stack
The current target stack is:
  - remote (Remote serial target in gdb-specific protocol)
  - exec (Local exec file)
  - None (None)
(gdb) run
Reading /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork from remote target...
`target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork' has disappeared; keeping its symbols.
Starting program: target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
Detaching after fork from child process 32279.
/bin/bash: /home/pedro/gdb/mygit/build/gdb/target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork: No such file or directory
During startup program exited with code 127.
(gdb) maint print target-stack
The current target stack is:
  - exec (Local exec file)
  - None (None)
(gdb)


Note that that last run tried to create the inferior in the
native target and its the native target that silently closes
the remote connection...:

(top-gdb) bt
#0  remote_close (self=0xdde300 <remote_ops>) at /home/pedro/gdb/mygit/src/gdb/remote.c:3403
#1  0x0000000000696101 in target_close (targ=0xdde300 <remote_ops>) at /home/pedro/gdb/mygit/src/gdb/target.c:3308
#2  0x0000000000691e8d in push_target (t=0xece890) at /home/pedro/gdb/mygit/src/gdb/target.c:697
#3  0x00000000004b9495 in inf_ptrace_create_inferior (ops=0xece890, exec_file=0x1bfa190 "target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork",
    allargs=0x1c09f60 "", env=0x12d0220, from_tty=1) at /home/pedro/gdb/mygit/src/gdb/inf-ptrace.c:105
#4  0x00000000004bf6dc in linux_nat_create_inferior (ops=0xece890, exec_file=0x1bfa190 "target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork",
    allargs=0x1c09f60 "", env=0x12d0220, from_tty=1) at /home/pedro/gdb/mygit/src/gdb/linux-nat.c:1194
#5  0x000000000062c018 in run_command_1 (args=0x0, from_tty=1, tbreak_at_main=0) at /home/pedro/gdb/mygit/src/gdb/infcmd.c:604
#6  0x000000000062c121 in run_command (args=0x0, from_tty=1) at /home/pedro/gdb/mygit/src/gdb/infcmd.c:639



I tried the "add-inferior" scenario too now, and it's also
quite confusing:

(gdb) info inferiors
  Num  Description       Executable
* 1    process 32274     target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
(gdb) add-inferior
Added inferior 2
(gdb) inferior 2
[Switching to inferior 2 [<null>] (<noexec>)]
(gdb) file /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
Reading symbols from /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork...done.
(gdb) set remote exec-file /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
(gdb) start
Temporary breakpoint 1 at 0x400671: main. (2 locations)
Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
Detaching after fork from child process 32353.
Detaching after fork from child process 32354.
Detaching after fork from child process 32356.
Error in re-setting breakpoint 1: Cannot access memory at address 0x400669
Error in re-setting breakpoint 1: Cannot access memory at address 0x400669
Error in re-setting breakpoint 1: Cannot access memory at address 0x400669
Error in re-setting breakpoint 1: Cannot access memory at address 0x400669
Error in re-setting breakpoint 1: Cannot access memory at address 0x400669
[Inferior 2 (process 32353) exited normally]
(gdb) info inferiors
  Num  Description       Executable
  1    <null>            target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
* 2    <null>            /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
(gdb) maint print target-stack
The current target stack is:
  - exec (Local exec file)
  - None (None)
(gdb)

Likewise here start closed the remote connection, and then
find_run_target() returns the native target.

"set auto-connect-native-target off" works around these issues.

))))

> Tested on x86_64 native, native-gdbserver, native-extended-gdbserver. No
> regressions, other than one issue with gdb.reverse/waitpid-reverse.exp.
> My version of GDB seemed to evoke an existing KFAIL, and after that I
> would get a new FAIL.  I didn't see the KFAIL on "master", and so the
> subsequent FAIL wouldn't show up either.

Sorry, I'm confused.  Does this series cause a new FAIL, or not?

> 
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 5e053b3..2e0e1d5 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -3959,9 +3959,11 @@ process_serial_event (void)
>  	  discard_queued_stop_replies (pid_to_ptid (pid));
>  	  write_ok (own_buf);
>  
> -	  if (extended_protocol)
> +	  if (extended_protocol || get_first_inferior (&all_threads) != NULL)

Replace that get_first_inferior() by get_first_thread() or even target_running().



> @@ -8800,41 +8797,7 @@ kill_new_fork_children (int pid, struct remote_state *rs)
>  }
>  
>  
> -static void
> -remote_kill (struct target_ops *ops)
> -{
> -
> -  /* Catch errors so the user can quit from gdb even when we
> -     aren't on speaking terms with the remote system.  */
> -  TRY
> -    {
> -      putpkt ("k");
> -    }
> -  CATCH (ex, RETURN_MASK_ERROR)
> -    {
> -      if (ex.error == TARGET_CLOSE_ERROR)
> -	{
> -	  /* If we got an (EOF) error that caused the target
> -	     to go away, then we're done, that's what we wanted.
> -	     "k" is susceptible to cause a premature EOF, given
> -	     that the remote server isn't actually required to
> -	     reply to "k", and it can happen that it doesn't
> -	     even get to reply ACK to the "k".  */
> -	  return;
> -	}
> -
> -	/* Otherwise, something went wrong.  We didn't actually kill
> -	   the target.  Just propagate the exception, and let the
> -	   user or higher layers decide what to do.  */
> -	throw_exception (ex);
> -    }
> -  END_CATCH
> -
> -  /* We've killed the remote end, we get to mourn it.  Since this is
> -     target remote, single-process, mourning the inferior also
> -     unpushes remote_ops.  */
> -  target_mourn_inferior ();
> -}
> +/* Send a kill request to the target using the 'vKill' packet.  */
>  
>  static int
>  remote_vkill (int pid, struct remote_state *rs)
> @@ -8861,56 +8824,99 @@ remote_vkill (int pid, struct remote_state *rs)
>      }
>  }
>  
> +/* Send a kill request to the target using the 'k' packet.  */
> +
>  static void
> -extended_remote_kill (struct target_ops *ops)
> +remote_kill_k (void)
>  {
> -  int res;
> +  /* Catch errors so the user can quit from gdb even when we
> +     aren't on speaking terms with the remote system.  */
> +  TRY
> +    {
> +      putpkt ("k");
> +    }
> +  CATCH (ex, RETURN_MASK_ERROR)
> +    {
> +      if (ex.error == TARGET_CLOSE_ERROR)
> +	{
> +	  /* If we got an (EOF) error that caused the target
> +	     to go away, then we're done, that's what we wanted.
> +	     "k" is susceptible to cause a premature EOF, given
> +	     that the remote server isn't actually required to
> +	     reply to "k", and it can happen that it doesn't
> +	     even get to reply ACK to the "k".  */
> +	  return;
> +	}
> +
> +      /* Otherwise, something went wrong.  We didn't actually kill
> +	 the target.  Just propagate the exception, and let the
> +	 user or higher layers decide what to do.  */
> +      throw_exception (ex);
> +    }
> +  END_CATCH
> +}

Can't tell why you moved the function below rather than rename it
in place.  I suspect that it was just a consequence of how v1 was
written.  If there's no reason for the move, please leave the function
where it was (and check for spurious changes afterwards), which helps
git blame archaeology.

> +
> +/* Target hook to kill the current inferior.  */
> +
> +static void
> +remote_kill (struct target_ops *ops)
> +{


> -static void
> -remote_mourn (struct target_ops *target)
> -{
> -  unpush_target (target);
> +      return;
> +    }
>  
> -  /* remote_close takes care of doing most of the clean up.  */
> -  generic_mourn_inferior ();
> +  error (_("Can't kill process"));
> +  return;

"return" is unreachable after "error".

Otherwise looks good to me.

Thanks,
Pedro Alves