From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6046 invoked by alias); 3 Jan 2017 21:35:51 -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 6024 invoked by uid 89); 3 Jan 2017 21:35:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.1 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=sk:invalid, sk:cmd_lis, disconnect, disconnected X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Jan 2017 21:35:39 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 57404A24D3; Tue, 3 Jan 2017 21:35:39 +0000 (UTC) Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v03LZcVW021411 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 3 Jan 2017 16:35:38 -0500 From: Sergio Durigan Junior To: Luis Machado Cc: GDB Patches , 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> X-URL: http://blog.sergiodj.net Date: Tue, 03 Jan 2017 21:35:00 -0000 In-Reply-To: (Luis Machado's message of "Mon, 26 Dec 2016 15:34:19 -0600") Message-ID: <874m1fq11y.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-01/txt/msg00034.txt.bz2 On Monday, December 26 2016, Luis Machado wrote: > 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. According to my previous comments, this is OK. >> +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." I was lazy and copied the comments from the previous set/show functions. I'll split the comments in two. >> +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? You're right, thanks. >> +#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" Will fix it, thanks. >> + 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. Thanks. >> + 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. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/