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

Luis Machado
2017-02-01T19:07:08Z

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.