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

Sergio Durigan Junior
2017-01-03T21:35:51Z

On Monday, December 26 2016, Luis Machado wrote:

> On 12/22/2016 09:39 PM, Sergio Durigan Junior wrote:
>> This patch implements the proper support for the "startup-with-shell"
>> feature on gdbserver. A new packet is added, QStartupShell, and this
>> packet is sent on initialization. If the host sends a
>> "QStartupShell:" (i.e., without arguments), it means the inferior
>> shall be started without using a shell. Otherwise, the argument on
>> the packet is the shell to be used to start the inferior.
>>
>> I decided to use the "startup-with-shell" setting from the host in
>> order to decide whether to start the remote inferior using a shell or
>> not. However, since there is no easy way to change which shell to use
>> when starting things, I have also added a new command, "set/show
>> remote startup-shell", which can contain the shell that the remote
>> target will use.
>>
>> A documentation patch is included, along with a new testcase for the
>> feature.
>>
>> gdb/ChangeLog:
>> 2016-12-22 Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> * remote.c (remote_startup_shell_var): New variable.
>> (set_remote_startup_shell): New function.
>> (show_remote_startup_shell): Likewise.
>> Add PACKET_QStartupShell.
>> (remote_start_remote): Handle new PACKET_QStartupShell.
>> (struct protocol_feature remote_protocol_features): New entry for
>> PACKET_QStartupShell.
>> (_initialize_remote): Call "add_packet_config_cmd" for
>> QStartupShell. Add new set/show command "set remote
>> startup-shell".
>>
>> gdb/gdbserver/ChangeLog:
>> 2016-12-22 Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> * server.c (handle_general_set): Handle new packet
>> "QStartupShell".
>> (handle_query): Add "QStartupShell" to the list of supported
>> packets.
>>
>> gdb/testsuite/ChangeLog:
>> 2016-12-22 Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> * gdb.server/startup-with-shell.c: New file.
>> * gdb.server/startup-with-shell.exp: Likewise.
>>
>> gdb/doc/ChangeLog:
>> 2016-12-22 Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> * gdb.texinfo (startup-with-shell): Add note mentioning that the
>> setting is also used by remote targets.
>> (set remote startup-shell): New item.
>> ---
>> gdb/doc/gdb.texinfo | 14 +++
>> gdb/gdbserver/server.c | 22 ++++-
>> gdb/remote.c | 62 +++++++++++++
>> gdb/testsuite/gdb.server/startup-with-shell.c | 12 +++
>> gdb/testsuite/gdb.server/startup-with-shell.exp | 110 ++++++++++++++++++++++++
>> 5 files changed, 219 insertions(+), 1 deletion(-)
>> create mode 100644 gdb/testsuite/gdb.server/startup-with-shell.c
>> create mode 100644 gdb/testsuite/gdb.server/startup-with-shell.exp
>>
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index a0de7d1..3250cae 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -2174,6 +2174,10 @@ initialization file---such as @file{.cshrc} for C-shell,
>> $@file{.zshenv} for the Z shell, or the file specified in the
>> @samp{BASH_ENV} environment variable for BASH.
>>
>> +This setting is also used by @code{target extended-remote} to
>> +determine whether the target system will start the inferior using the
>> +shell (@pxref{set remote startup-shell}).
>> +
>> @anchor{set auto-connect-native-target}
>> @kindex set auto-connect-native-target
>> @item set auto-connect-native-target
>> @@ -20486,6 +20490,16 @@ extended-remote}. This should be set to a filename valid on the
>> target system. If it is not set, the target will use a default
>> filename (e.g.@: the last program run).
>>
>> +@item set remote startup-shell @var{shell}
>> +@itemx show remote startup-shell
>> +@anchor{set remote startup-shell}
>> +@cindex startup shell, for remote target
>> +Set the shell that is going to be used to start the inferior with
>> +@code{target extended-remote}. This should be set to a valid shell on
>> +the target system, and is only effective when
>> +@code{startup-with-shell} is on. If it is not set, the target system
>> +will use the shell pointed by the @code{SHELL} environment variable.
>> +
>> @item set remote interrupt-sequence
>> @cindex interrupt remote programs
>> @cindex select Ctrl-C, BREAK or BREAK-g
>> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
>> index 55eebd9..5faf0b5 100644
>> --- a/gdb/gdbserver/server.c
>> +++ b/gdb/gdbserver/server.c
>> @@ -863,6 +863,26 @@ handle_general_set (char *own_buf)
>> return;
>> }
>>
>> + if (startswith (own_buf, "QStartupShell:"))
>> + {
>> + const char *p = own_buf + strlen ("QStartupShell:");
>> +
>> + if (*p == '\0')
>> + {
>> + startup_with_shell = 0;
>> + xfree (startup_shell);
>> + }
>> + else
>> + {
>> + startup_with_shell = 1;
>> + xfree (startup_shell);
>> + startup_shell = xstrdup (p);
>> + }
>> +
>> + write_ok (own_buf);
>> + return;
>> + }
>> +
>> /* Otherwise we didn't know what packet it was. Say we didn't
>> understand it. */
>> own_buf[0] = 0;
>> @@ -2299,7 +2319,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+;QStartupShell+",
>> PBUFSIZ - 1);
>>
>> if (target_supports_catch_syscall ())
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 0b5f837..2d2a87a 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -86,6 +86,12 @@ static const struct program_space_data *remote_pspace_data;
>> location of the remote exec-file value. */
>> static char *remote_exec_file_var;
>>
>> +/* The variable that holds the startup shell that will be used by the
>> + remote target to start the inferior. If this variable is NULL or
>> + empty, we use the value of 'get_startup_shell'. */
>> +
>
> Spurious newline.

According to my previous comments, this is OK.

>> +static char *remote_startup_shell_var;
>> +
>> /* The size to align memory write packets, when practical. The protocol
>> does not guarantee any alignment, and gdb will generate short
>> writes and unaligned writes, but even as a best-effort attempt this
>> @@ -727,6 +733,22 @@ show_remote_exec_file (struct ui_file *file, int from_tty,
>> fprintf_filtered (file, "%s\n", remote_exec_file_var);
>> }
>>
>> +/* The "set/show remote startup-shell" set command hook. */
>> +
>
> Technically shouldn't we break this up into a couple comments?
>
> Or maybe "... set command hooks."

I was lazy and copied the comments from the previous set/show
functions. I'll split the comments in two.

>> +static void
>> +set_remote_startup_shell (char *ignored, int from_tty,
>> + struct cmd_list_element *c)
>> +{
>> + gdb_assert (remote_startup_shell_var != NULL);
>> +}
>> +
>> +static void
>> +show_remote_startup_shell (struct ui_file *file, int from_tty,
>> + struct cmd_list_element *cmd, const char *value)
>> +{
>> + fprintf_filtered (file, "%s\n", remote_startup_shell_var);
>> +}
>> +
>> static int
>> compare_pnums (const void *lhs_, const void *rhs_)
>> {
>> @@ -1423,6 +1445,7 @@ enum {
>> PACKET_QPassSignals,
>> PACKET_QCatchSyscalls,
>> PACKET_QProgramSignals,
>> + PACKET_QStartupShell,
>> PACKET_qCRC,
>> PACKET_qSearch_memory,
>> PACKET_vAttach,
>> @@ -4074,6 +4097,31 @@ 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 (packet_support (PACKET_QStartupShell) != PACKET_DISABLE)
>> + {
>> + if (startup_with_shell)
>> + {
>> + const char *sh;
>> +
>> + if (remote_startup_shell_var != NULL
>> + && *remote_startup_shell_var != '\0')
>> + sh = remote_startup_shell_var;
>> + else
>> + sh = get_startup_shell ();
>> +
>> + xsnprintf (rs->buf, get_remote_packet_size (),
>> + "QStartupShell:%s", sh);
>> + }
>> + else
>> + xsnprintf (rs->buf, get_remote_packet_size (),
>> + "QStartupShell:");
>> +
>> + putpkt (rs->buf);
>> + getpkt (&rs->buf, &rs->buf_size, 0);
>> + if (strcmp (rs->buf, "OK") != 0)
>> + error (_("Could not set remote startup-with-shell"));
>> + }
>> +
>> /* gdbserver < 7.7 (before its fix from 2013-12-11) did reply to any
>> unknown 'v' packet with string "OK". "OK" gets interpreted by GDB
>> as a reply to known packet. For packet "vFile:setfs:" it is an
>> @@ -4628,6 +4676,8 @@ static const struct protocol_feature remote_protocol_features[] = {
>> PACKET_QCatchSyscalls },
>> { "QProgramSignals", PACKET_DISABLE, remote_supported_packet,
>> PACKET_QProgramSignals },
>> + { "QStartupShell", PACKET_DISABLE, remote_supported_packet,
>> + PACKET_QStartupShell },
>> { "QStartNoAckMode", PACKET_DISABLE, remote_supported_packet,
>> PACKET_QStartNoAckMode },
>> { "multiprocess", PACKET_DISABLE, remote_supported_packet,
>> @@ -14081,6 +14131,9 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
>> add_packet_config_cmd (&remote_protocol_packets[PACKET_QProgramSignals],
>> "QProgramSignals", "program-signals", 0);
>>
>> + add_packet_config_cmd (&remote_protocol_packets[PACKET_QStartupShell],
>> + "QStartupShell", "startup-shell", 0);
>> +
>> add_packet_config_cmd (&remote_protocol_packets[PACKET_qSymbol],
>> "qSymbol", "symbol-lookup", 0);
>>
>> @@ -14380,6 +14433,15 @@ Show the remote pathname for \"run\""), NULL,
>> &remote_set_cmdlist,
>> &remote_show_cmdlist);
>>
>> + add_setshow_string_noescape_cmd ("startup-shell", class_files,
>> + &remote_startup_shell_var, _("\
>> +Set the remote shell for starting the inferior"), _("\
>> +Show the remote shell for starting the inferior"), NULL,
>> + set_remote_startup_shell,
>> + show_remote_startup_shell,
>> + &remote_set_cmdlist,
>> + &remote_show_cmdlist);
>> +
>> add_setshow_boolean_cmd ("range-stepping", class_run,
>> &use_range_stepping, _("\
>> Enable or disable range stepping."), _("\
>> diff --git a/gdb/testsuite/gdb.server/startup-with-shell.c b/gdb/testsuite/gdb.server/startup-with-shell.c
>> new file mode 100644
>> index 0000000..ba77b10
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.server/startup-with-shell.c
>> @@ -0,0 +1,12 @@
>
> Missing copyright notice in the source file?

You're right, thanks.

>> +#include <stdio.h>
>> +
>> +int
>> +main (int argc, char *argv[])
>> +{
>> + int i;
>> +
>> + for (i = 0; argv[i] != NULL; ++i)
>> + printf ("ARG %d = %s\n", i, argv[i]);
>> +
>> + return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.server/startup-with-shell.exp b/gdb/testsuite/gdb.server/startup-with-shell.exp
>> new file mode 100644
>> index 0000000..1bf3654
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.server/startup-with-shell.exp
>> @@ -0,0 +1,110 @@
>> +# This testcase is part of GDB, the GNU debugger.
>> +
>> +# Copyright 2016 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# Test startup-with-shell support using extended-remote.
>> +
>> +load_lib gdbserver-support.exp
>> +
>> +standard_testfile
>> +
>> +if { [skip_gdbserver_tests] } {
>
> untested "skipping gdbserver tests"

Will fix it, thanks.

>> + return 0
>> +}
>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
>> + untested "failed to compile"
>
> The above untested call is not needed as prepare_for_testing will
> handle it with the "failed to prepare" message.

Thanks.

>> + return -1
>> +}
>> +
>> +# 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"
>> +}
>> +
>> +proc invalid_remote_shell_test { } {
>> + global 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 on"
>> + gdb_test_no_output "set remote startup-shell /path/to/invalid/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" \
>> + "Running \"$binfile\" on the remote target failed" \
>> + "run to main (should fail)"
>> +}
>> +
>> +## Doing 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"
>> +}
>> +
>> +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"
>> +}
>> +
>> +with_test_prefix "startup_with_shell = on; run_args = \$TEST" {
>> + set env(TEST) "1234"
>> + initial_setup_simple "on" "\$TEST"
>> + gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"1234\"" \
>> + "testing first argument"
>> + unset env(TEST)
>> +}
>> +
>> +with_test_prefix "startup_with_shell = off; run_args = \$TEST" {
>> + set env(TEST) "1234"
>> + initial_setup_simple "off" "\$TEST"
>> + gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"\\\$TEST\"" \
>> + "testing first argument"
>> + unset env(TEST)
>> +}
>> +
>> +with_test_prefix "startup_with_shell = on; invalid remote startup-shell" {
>> + invalid_remote_shell_test
>> +}
>>
>
> Otherwise looks OK.

Thanks,

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