Re: [PATCH v3 6/6] Implement proper "startup-with-shell" support on gdbserver

[Pedro Alves <palves@redhat.com>]

On 02/08/2017 03:22 AM, Sergio Durigan Junior wrote:

>  
>  *** Changes since GDB 7.12
>  
> +* GDBserver is now able to start inferiors using a shell.  When using
> +  "target extended-remote", the host GDB honors the value of "set
> +  startup-with-shell" in order to inform GDBserver whether the remote
> +  inferior should be started with a shell or not.  When using "target
> +  remote", it is possible to disable the startup with shell by using
> +  the new parameter "--no-startup-with-shell" when starting GDBserver.
> +

IMO, this is missing the "something that makes users curious to
try the feature".  I.e., talking in terms of user-visible features.
I.e., talk about why starting with a shell would be useful.

I'd suggest reworking/extended like this:

* On Unix systems, GDBserver now does globbing expansion
  and variable substitution in inferior command line arguments.

  This is done by starting inferiors using a shell, like GDB does.
  See "set startup-with-shell" in the user manual for how to disable
  this from GDB when using "target extended-remote".
  When using "target remote", you can disable the startup with shell
  by using the new "--no-startup-with-shell" GDBserver command
  line option.


Note, you'll need NEWS entries for the new remote protocol packets too.




>  @item qfThreadInfo
>  @itemx qsThreadInfo
>  @cindex list active threads, remote request
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 025f7c4..e2c4a30 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -867,6 +867,31 @@ handle_general_set (char *own_buf)
>        return;
>      }
>  
> +  if (startswith (own_buf, "QStartupWithShell:"))
> +    {
> +      char *value = own_buf + strlen ("QStartupWithShell:");
> +
> +      if (strcmp (value, "1") == 0)
> +	startup_with_shell = true;
> +      else if (strcmp (value, "0") == 0)
> +	startup_with_shell = false;
> +      else
> +	{
> +	  /* Unknown value.  */
> +	  fprintf (stderr, "Unknown value to startup-with-shell: %s\n",
> +		   own_buf);
> +	  write_enn (own_buf);
> +	  return;
> +	}
> +
> +      if (remote_debug)
> +	debug_printf (_("[Inferior will %s started with shell]"),
> +		      startup_with_shell ? "be" : "not be");
> +
> +      write_ok (own_buf);
> +      return;
> +    }
> +
>    /* Otherwise we didn't know what packet it was.  Say we didn't
>       understand it.  */
>    own_buf[0] = 0;
> @@ -2303,7 +2328,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>  	}
>  
>        sprintf (own_buf,
> -	       "PacketSize=%x;QPassSignals+;QProgramSignals+",
> +	       "PacketSize=%x;QPassSignals+;QProgramSignals+;QStartupWithShell+",
>  	       PBUFSIZ - 1);
>  
>        if (target_supports_catch_syscall ())
> @@ -3397,6 +3422,11 @@ gdbserver_usage (FILE *stream)
>  	   "  --no-disable-randomization\n"
>  	   "                        Don't disable address space randomization when\n"
>  	   "                        starting PROG.\n"
> +	   "  --startup-with-shell\n"
> +	   "                        Start PROG using a shell.\n"
> +	   "  --no-startup-with-shell\n"
> +	   "                        Don't start PROG using a shell (i.e., use the exec*\n"
> +	   "                        family of functions).\n"

I wonder whether this "i.e., use the exec family of functions" comment
remark really belongs here.  If you're not very familiar with
what the internal implementation, I think it meaningless.  If you are
familiar with the internals, then like me you'll wonder what does
that mean, because we also use exec to run the shell?  :-)

Maybe replace with:

 --startup-with-shell
                      Start PROG using a shell.  I.e., exec a shell that
                      then execs PROG.  (default)
 --no-startup-with-shell
                      Exec PROG  directly instead of using a shell. 
                      Disables argument globbing, and variable substitution
                      on Unix-like systems.


> @@ -4079,6 +4080,20 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
>    if (packet_support (PACKET_QAllow) != PACKET_DISABLE)
>      remote_set_permissions (target);
>  
> +  /* If startup-with-shell is on, we inform gdbserver to start the
> +     remote inferior using a shell.  */
> +  if (packet_support (PACKET_QStartupWithShell) != PACKET_DISABLE)
> +    {
> +      xsnprintf (rs->buf, get_remote_packet_size (),
> +		 "QStartupWithShell:%d", startup_with_shell ? 1 : 0);
> +      putpkt (rs->buf);
> +      getpkt (&rs->buf, &rs->buf_size, 0);
> +      if (strcmp (rs->buf, "OK") != 0)
> +	error (_("\
> +Remote replied unexpectedly while setting startup-with-shell: %s"),
> +	       rs->buf);
> +    }

Can you explain the rationale for doing this here?  What about:

 (gdb) target extended-remote ....
 (gdb) set startup-with-shell off
 (gdb) run

?

> +
> +# Initial setup for simple test (wildcard expansion, variable substitution).
> +
> +proc initial_setup_simple { startup_with_shell run_args } {
> +    global hex decimal binfile
> +
> +    clean_restart $binfile
> +    # Make sure we're disconnected, in case we're testing with an
> +    # extended-remote board, therefore already connected.
> +    gdb_test "disconnect" ".*"
> +
> +    gdb_test_no_output "set startup-with-shell $startup_with_shell"
> +
> +    set target_exec [gdbserver_download_current_prog]
> +    gdbserver_start_extended
> +    gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
> +
> +    gdb_breakpoint main
> +
> +    gdb_test "run $run_args" \
> +	"Breakpoint ${decimal}, main \\(argc=${decimal}, argv=${hex}\\).*" \
> +	"run to main"
> +}
> +
> +## Doing the actual tests

"Run the actual tests."

> +
> +with_test_prefix "startup_with_shell = on; run_args = *.log" {
> +    initial_setup_simple "on" "*.log"
> +    gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"config\.log\"" \
> +	"testing first argument"

"test first argument".  Or better:

    gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"config\.log\"" \
 	"first argument expanded"

> +}
> +
> +with_test_prefix "startup_with_shell = off; run_args = *.log" {
> +    initial_setup_simple "off" "*.log"
> +    gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"\\\*\.log\"" \
> +	"testing first argument"

 	"first argument not expanded"

Relying on "config.log" existing on the current dir, and that showing
up as first argument seems fragile.  Better would be to create some
file with some unique-ish name in the standard output dir, and use
a pattern that is unlikely to find anything else.

Thanks,
Pedro Alves