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

[Luis Machado <lgustavo@codesourcery.com>]

On 01/18/2017 09:36 AM, Sergio Durigan Junior wrote:
> This patch implements the proper support for the "startup-with-shell"
> feature on gdbserver.  A new packet is added, QStartupWithShell, and
> it is sent on initialization.  If the host sends a
> "QStartupWithShell:1", it means the inferior shall be started using a
> shell.  If the host sends a "QStartupWithShell:0", it means the
> inferior shall be started without using a shell.  Any other value is
> considered an error.
>
> There is no way to remotely set the shell that will be used by the
> target to start the inferior.  In order to do that, the user must
> start gdbserver while providing a shell via the $SHELL environment
> variable.  The same is true for the host side.
>
> The "set startup-with-shell" setting from the host side is used to
> decide whether to start the remote inferior using a shell.  This same
> setting is also used to decide whether to use a shell to start the
> host inferior; this means that it is not really possible to start the
> inferior using different mechanisms on target and host.
>
> A documentation patch is included, along with a new testcase for the
> feature.
>
> gdb/ChangeLog:
> 2017-01-17  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* NEWS (Changes since GDB 7.12): Announce that GDBserver is now
> 	able to start inferiors	using a shell.
> 	(New remote packets): Announce new packet "QStartupWithShell".
> 	* remote.c: Add PACKET_QStartupWithShell.
> 	(remote_start_remote): Handle new PACKET_QStartupWithShell.
> 	(remote_protocol_features) <QStartupWithShell>: New entry for
> 	PACKET_QStartupWithShell.
> 	(_initialize_remote): Call "add_packet_config_cmd" for
> 	QStartupShell.
>
> gdb/gdbserver/ChangeLog:
> 2017-01-17  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* server.c (handle_general_set): Handle new packet
> 	"QStartupWithShell".
> 	(handle_query): Add "QStartupWithShell" to the list of supported
> 	packets.
> 	(gdbserver_usage): Add help text explaining the
> 	new "--startup-with-shell" and "--no-startup-with-shell" CLI
> 	options.
> 	(captured_main): Recognize and act upon the presence of the new
> 	CLI options.
>
> gdb/testsuite/ChangeLog:
> 2017-01-17  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* gdb.server/startup-with-shell.c: New file.
> 	* gdb.server/startup-with-shell.exp: Likewise.
>
> gdb/doc/ChangeLog:
> 2017-01-17  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* gdb.texinfo (Starting) <startup-with-shell>: Add @anchor.
> 	(Connecting) <Remote Packet>: Add "startup-with-shell"
> 	and "QStartupWithShell" to the table.
> 	(Remote Protocol) <QStartupWithShell>: New item, explaining the
> 	packet.
> ---
>  gdb/NEWS                                        | 10 +++
>  gdb/doc/gdb.texinfo                             | 26 ++++++++
>  gdb/gdbserver/server.c                          | 36 ++++++++++-
>  gdb/remote.c                                    | 20 ++++++
>  gdb/testsuite/gdb.server/startup-with-shell.c   | 29 +++++++++
>  gdb/testsuite/gdb.server/startup-with-shell.exp | 83 +++++++++++++++++++++++++
>  6 files changed, 203 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/NEWS b/gdb/NEWS
> index b976815..9bf8df4 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,13 @@
>
>  *** 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.
> +
>  * Building GDB and GDBserver now requires a C++11 compiler.
>
>    For example, GCC 4.8 or later.
> @@ -356,6 +363,9 @@ show max-value-size
>
>  * New remote packets
>
> +QStartupWithShell
> +  Indicates whether the inferior must be started with a shell or not.
> +
>  exec stop reason
>    Indicates that an exec system call was executed.
>
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 2b6b654..bfe11de 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -2153,6 +2153,7 @@ This command is available when debugging locally on most targets, excluding
>  @sc{djgpp}, Cygwin, MS Windows, and QNX Neutrino.
>
>  @kindex set startup-with-shell
> +@anchor{set startup-with-shell}
>  @item set startup-with-shell
>  @itemx set startup-with-shell on
>  @itemx set startup-with-shell off
> @@ -20772,6 +20773,10 @@ are:
>  @tab @code{QDisableRandomization}
>  @tab @code{set disable-randomization}
>
> +@item @code{startup-with-shell}
> +@tab @code{QStartupWithShell}
> +@tab @code{set startup-with-shell}
> +
>  @item @code{conditional-breakpoints-packet}
>  @tab @code{Z0 and Z1}
>  @tab @code{Support for target-side breakpoint condition evaluation}
> @@ -36330,6 +36335,27 @@ by supplying an appropriate @samp{qSupported} response (@pxref{qSupported}).
>  This should only be done on targets that actually support disabling
>  address space randomization.
>
> +@item QStartupWithShell:@var{value}
> +@cindex startup with shell, remote request
> +@cindex @samp{QStartupWithShell} packet
> +On UNIX-like targets, it is possible to start the inferior using a
> +shell program.  This is the default behavior on both @value{GDBN} and
> +@command{gdbserver} (@pxref{set startup-with-shell}).  This packet is
> +used to inform @command{gdbserver} whether it should start the
> +inferior using a shell or not.

Missing documentation aboue what VALUE should be and the range of values 
it may contain?

> +
> +This packet is only available in extended mode (@pxref{extended
> +mode}).
> +
> +Reply:
> +@table @samp
> +@item OK
> +The request succeeded.
> +
> +@item E @var{nn}
> +An error occurred.  The error number @var{nn} is give as hex digits.
> +@end table
> +
>  @item qfThreadInfo
>  @itemx qsThreadInfo
>  @cindex list active threads, remote request
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index a26ad52..56bd613 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -866,6 +866,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 = 1;
> +      else if (strcmp (value, "0") == 0)
> +	startup_with_shell = 0;
> +      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;
> @@ -2302,7 +2327,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 ())
> @@ -3396,6 +3421,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"
>  	   "\n"
>  	   "Debug options:\n"
>  	   "\n"
> @@ -3679,6 +3709,10 @@ captured_main (int argc, char *argv[])
>  	disable_randomization = 1;
>        else if (strcmp (*next_arg, "--no-disable-randomization") == 0)
>  	disable_randomization = 0;
> +      else if (strcmp (*next_arg, "--startup-with-shell") == 0)
> +	startup_with_shell = 1;
> +      else if (strcmp (*next_arg, "--no-startup-with-shell") == 0)
> +	startup_with_shell = 0;
>        else if (strcmp (*next_arg, "--once") == 0)
>  	run_once = 1;
>        else
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 2f7954a..67319f6 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -1428,6 +1428,7 @@ enum {
>    PACKET_QPassSignals,
>    PACKET_QCatchSyscalls,
>    PACKET_QProgramSignals,
> +  PACKET_QStartupWithShell,
>    PACKET_qCRC,
>    PACKET_qSearch_memory,
>    PACKET_vAttach,
> @@ -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);
> +    }
> +
>    /* 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
> @@ -4633,6 +4648,8 @@ static const struct protocol_feature remote_protocol_features[] = {
>      PACKET_QCatchSyscalls },
>    { "QProgramSignals", PACKET_DISABLE, remote_supported_packet,
>      PACKET_QProgramSignals },
> +  { "QStartupWithShell", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_QStartupWithShell },
>    { "QStartNoAckMode", PACKET_DISABLE, remote_supported_packet,
>      PACKET_QStartNoAckMode },
>    { "multiprocess", PACKET_DISABLE, remote_supported_packet,
> @@ -14110,6 +14127,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_QStartupWithShell],
> +			 "QStartupWithShell", "startup-with-shell", 0);
> +
>    add_packet_config_cmd (&remote_protocol_packets[PACKET_qSymbol],
>  			 "qSymbol", "symbol-lookup", 0);
>
> 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..6278447
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/startup-with-shell.c
> @@ -0,0 +1,29 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2017 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/>.  */
> +
> +#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..8992593
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/startup-with-shell.exp
> @@ -0,0 +1,83 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2017 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] } {
> +    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"
> +}
> +
> +## 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)
> +}
>

Otherwise i have no further comments.