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

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

Thanks for the review.  Comments below.

On Friday, February 17 2017, Pedro Alves wrote:

> 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.

Thanks, I've adopted your idea.

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

I've added a NEWS entry for QStartupWithShell under the "New remote
packets" section.  Do you want me to add another entry somewhere else?

>>  @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.

Fair enough.  I've adopted the texts.

>
>> @@ -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
>
> ?

Hm, I guess it just seemed like a good place to initialize the packet.
But you're right, this won't work if the user sets the
startup-with-shell parameter *after* connecting to the target.  So I'll
move this code to the extended_remote_create_inferior function, if
that's alright.

>> +
>> +# 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."

Fixed.

>> +
>> +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"

Fixed.

>> +}
>> +
>> +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"

Fixed the three nits.

> 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.

Hm, OK.  I'll take care of that.

Patch will be sent soon.

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/