From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 79361 invoked by alias); 26 Dec 2016 21:34:38 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 79350 invoked by uid 89); 26 Dec 2016 21:34:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=xstrdup, H*M:bc7a, target_ops, Doing X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 26 Dec 2016 21:34:27 +0000 Received: from svr-orw-mbx-03.mgc.mentorg.com ([147.34.90.203]) by relay1.mentorg.com with esmtp id 1cLcuW-0002O0-RB from Luis_Gustavo@mentor.com ; Mon, 26 Dec 2016 13:34:24 -0800 Received: from [172.30.6.27] (147.34.91.1) by svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Mon, 26 Dec 2016 13:34:21 -0800 Subject: Re: [PATCH 6/6] Implement proper "startup-with-shell" support on gdbserver References: <1482464361-4068-1-git-send-email-sergiodj@redhat.com> <1482464361-4068-7-git-send-email-sergiodj@redhat.com> To: Sergio Durigan Junior , GDB Patches CC: From: Luis Machado Reply-To: Luis Machado Message-ID: Date: Mon, 26 Dec 2016 21:34:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <1482464361-4068-7-git-send-email-sergiodj@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) X-IsSubscribed: yes X-SW-Source: 2016-12/txt/msg00419.txt.bz2 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 > > * 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 > > * 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 > > * gdb.server/startup-with-shell.c: New file. > * gdb.server/startup-with-shell.exp: Likewise. > > gdb/doc/ChangeLog: > 2016-12-22 Sergio Durigan Junior > > * 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 > + > +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 . > + > +# 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.