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

[Luis Machado <lgustavo@codesourcery.com>]

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.

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

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

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

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

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