This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 03/08/2017 05:29 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:
> yyyy-mm-dd  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.
> 	(extended_remote_create_inferior): 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:
> yyyy-mm-dd  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:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* gdb.server/startup-with-shell.c: New file.
> 	* gdb.server/startup-with-shell.exp: Likewise.
> 
> gdb/doc/ChangeLog:
> yyyy-mm-dd  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                                        | 12 ++++
>  gdb/doc/gdb.texinfo                             | 31 +++++++++
>  gdb/gdbserver/server.c                          | 38 ++++++++++-
>  gdb/remote.c                                    | 20 ++++++
>  gdb/testsuite/gdb.server/startup-with-shell.c   | 29 ++++++++
>  gdb/testsuite/gdb.server/startup-with-shell.exp | 90 +++++++++++++++++++++++++
>  6 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/NEWS b/gdb/NEWS
> index cf58595..7c235f7 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,15 @@
>  
>  *** Changes since GDB 7.12
>  
> +* 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.
> +
>  * GDB now supports access to the PKU register on GNU/Linux. The register is
>    added by the Memory Protection Keys for Userspace feature which will be
>    available in future Intel CPUs.
> @@ -393,6 +402,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 ba56ab2..b6bc7c7 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
> @@ -20811,6 +20812,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}
> @@ -36420,6 +36425,32 @@ 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.
> +
> +If @var{value} is @samp{0}, @command{gdbserver} will not use a shell
> +to start the inferior.  If @var{value} is @samp{1},
> +@command{gdbserver} will use a shell to start the inferior.  All other
> +values are considered an error.
> +
> +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 given as hex digits.
> +@end table


It seems like the usual "This packet is not probed by default" text
is missing, along with a "Use of this packet is controlled by the"
blurb mentioning the packet control command (search for those sentences 
in the manual).

> +
>  @item qfThreadInfo
>  @itemx qsThreadInfo
>  @cindex list active threads, remote request
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 1677926..8411aad 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -847,6 +847,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;
> @@ -2283,7 +2308,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 ())
> @@ -3379,6 +3404,13 @@ 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.  I.e., execs a shell that\n"
> +	   "                        then execs PROG.  (default)\n"
> +	   "  --no-startup-with-shell\n"
> +	   "                        Exec PROG directly instead of using a shell.\n"
> +	   "                        Disables argument globbing and variable substitution\n"
> +	   "                        on UNIX-like systems.\n"
>  	   "\n"
>  	   "Debug options:\n"
>  	   "\n"
> @@ -3662,6 +3694,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 = true;
> +      else if (strcmp (*next_arg, "--no-startup-with-shell") == 0)
> +	startup_with_shell = false;
>        else if (strcmp (*next_arg, "--once") == 0)
>  	run_once = 1;
>        else
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 2f34c4c..cb6e194 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,
> @@ -4633,6 +4634,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,
> @@ -9614,6 +9617,20 @@ extended_remote_create_inferior (struct target_ops *ops,
>    if (extended_remote_supports_disable_randomization (ops))
>      extended_remote_disable_randomization (disable_randomization);
>  
> +  /* 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);
> +    }
> +
>    /* Now restart the remote server.  */
>    run_worked = extended_remote_run (args) != -1;
>    if (!run_worked)
> @@ -14112,6 +14129,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..81be68b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/startup-with-shell.exp
> @@ -0,0 +1,90 @@
> +# 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
> +}
> +
> +set unique_file [standard_output_file "unique-file.unique-extension"]
> +remote_exec build "touch $unique_file"
> +
> +# Initial setup for simple test (wildcard expansion, variable substitution).
> +
> +proc initial_setup_simple { startup_with_shell run_args } {
> +    global hex decimal binfile unique_file
> +
> +    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_test "remote put $unique_file [file tail $unique_file]"

Why do we need this "put"?  Because CWD is set to gdbserver's directly?
Does this mean that this "put" is putting the file in gdbserver's directory?

> +
> +    gdb_breakpoint main
> +
> +    gdb_test "run $run_args" \
> +	"Breakpoint ${decimal}, main \\(argc=${decimal}, argv=${hex}\\).*" \
> +	"run to main"
> +}
> +
> +## Run the actual tests
> +
> +with_test_prefix "startup_with_shell = on; run_args = *.unique-extension" {
> +    initial_setup_simple "on" "*.unique-extension"
> +    gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"unique-file\.unique-extension\"" \
> +	"first argument expanded"
> +}
> +
> +with_test_prefix "startup_with_shell = off; run_args = *.unique-extension" {
> +    initial_setup_simple "off" "*.unique-extension"
> +    gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"\\\*\.unique-extension\"" \
> +	"first argument not expanded"
> +}
> +
> +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)
> +}
> +
> +remote_exec build "rm -f $unique_file"

It's better to leave the file in place to help with debugging
testsuite failures.

I'm wondering if the test really needs to be specifically coded for
gdbserver.  If we use the normal run_tomain etc., wouldn't
the test work against native testing as well?  When Eli added support
for shell expansion to the Windows ports a while ago I noticed that there
are no tests for these things for native targets at all.  This new test
would address that nicely.

Thanks,
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]