From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 47164 invoked by alias); 17 Feb 2017 16:05:59 -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 47150 invoked by uid 89); 17 Feb 2017 16:05:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=disconnected, disconnect, debug_printf, family 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; Fri, 17 Feb 2017 16:05:57 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (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 3A33C6AAF9; Fri, 17 Feb 2017 16:05:57 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1HG5s6p025010; Fri, 17 Feb 2017 11:05:55 -0500 Subject: Re: [PATCH v3 6/6] Implement proper "startup-with-shell" support on gdbserver To: Sergio Durigan Junior , GDB Patches References: <1482464361-4068-1-git-send-email-sergiodj@redhat.com> <20170208032257.15443-1-sergiodj@redhat.com> <20170208032257.15443-7-sergiodj@redhat.com> Cc: Luis Machado From: Pedro Alves Message-ID: Date: Fri, 17 Feb 2017 16:05:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170208032257.15443-7-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-02/txt/msg00495.txt.bz2 On 02/08/2017 03:22 AM, Sergio Durigan Junior wrote: > > *** 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. > + IMO, this is missing the "something that makes users curious to try the feature". I.e., talking in terms of user-visible features. I.e., talk about why starting with a shell would be useful. I'd suggest reworking/extended like this: * 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. Note, you'll need NEWS entries for the new remote protocol packets too. > @item qfThreadInfo > @itemx qsThreadInfo > @cindex list active threads, remote request > diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c > index 025f7c4..e2c4a30 100644 > --- a/gdb/gdbserver/server.c > +++ b/gdb/gdbserver/server.c > @@ -867,6 +867,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; > @@ -2303,7 +2328,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 ()) > @@ -3397,6 +3422,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" I wonder whether this "i.e., use the exec family of functions" comment remark really belongs here. If you're not very familiar with what the internal implementation, I think it meaningless. If you are familiar with the internals, then like me you'll wonder what does that mean, because we also use exec to run the shell? :-) Maybe replace with: --startup-with-shell Start PROG using a shell. I.e., exec a shell that then execs PROG. (default) --no-startup-with-shell Exec PROG directly instead of using a shell. Disables argument globbing, and variable substitution on Unix-like systems. > @@ -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); > + } Can you explain the rationale for doing this here? What about: (gdb) target extended-remote .... (gdb) set startup-with-shell off (gdb) run ? > + > +# 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 "Run 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" "test first argument". Or better: gdb_test "print argv\[1\]" "\\\$$decimal = $hex \"config\.log\"" \ "first argument expanded" > +} > + > +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" "first argument not expanded" Relying on "config.log" existing on the current dir, and that showing up as first argument seems fragile. Better would be to create some file with some unique-ish name in the standard output dir, and use a pattern that is unlikely to find anything else. Thanks, Pedro Alves