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

Sergio Durigan Junior
2017-03-07T20:59:16Z

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