From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id ittKILOEhmf/6A4AWB0awg (envelope-from ) for ; Tue, 14 Jan 2025 10:37:23 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1736869043; bh=BDBH5N11SciEKZ5BTcc0HPu1Ww2OCwWPz0dYowNI/xA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=JEoJ1tkWZBkFMn2Agaz39h4MbmN79VLplpyrKr9uSljdJTHoRuyTQTaxdjFGFyl0D HLxw32eWbw6eaw0l1bEeXRybc0IMn0ZrDvKp4YT5L4wojH06GR1svoWqgq5VyH1P5N yaySFXlWzul5rXkDBlEXMcCpKtRIgXZ8O0iz37xI= Received: by simark.ca (Postfix, from userid 112) id 764061E100; Tue, 14 Jan 2025 10:37:23 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-5.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=unavailable autolearn_force=no version=4.0.0 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=QdWsOISQ; dkim=pass (1024-bit key) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=tgjy1bPi; dkim-atps=neutral Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id BAE8F1E05C for ; Tue, 14 Jan 2025 10:37:22 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 4A1B5385B515 for ; Tue, 14 Jan 2025 15:37:22 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 4A1B5385B515 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=QdWsOISQ; dkim=pass (1024-bit key) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=tgjy1bPi Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 10915385B50F for ; Tue, 14 Jan 2025 15:33:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 10915385B50F Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 10915385B50F Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736868780; cv=none; b=ZH3GcXugqtO0TwiKJg42qa7JTYIdGWM7AzzZK1SLOpUQatgGYpxr51qc7gwzWmDzlPO8AEGi42ME8FnUVpRiNKn6aY8zRrRcyfFnCI1FohRP8HDEcHrEelYBgEWDpoEgTZYKg9RoAB0JavqBhxHsX+Js1fzIT69p6qHlUoY1+Wk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736868780; c=relaxed/simple; bh=BDBH5N11SciEKZ5BTcc0HPu1Ww2OCwWPz0dYowNI/xA=; h=DKIM-Signature:DKIM-Signature:Message-ID:Date:MIME-Version: Subject:To:From; b=H6XHosH1+L9f0Z7OQx+LL/jILID098O0XAoYO+IKQHJvUWX/xgs+u7LBegfnddWCUIgrE8HYkc4559Us8VMvpC90IpOG1f7G1pfUQss4xxIaFwyvbeTJBO6yJ6Quqb2n7OS+OpIDcoBBNSYOSnV0jxvUqkNw0ALbNfvpjvhjFlc= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 10915385B50F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1736868779; bh=BDBH5N11SciEKZ5BTcc0HPu1Ww2OCwWPz0dYowNI/xA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=QdWsOISQlAxttOFCwN1FsLHm+iK1XGZS/R8mmpLPp9ZjXCiVqGrF53cYJ8qGsurCK 0rfPBXAmBGxkyH9nMBJIkNqzS+9tFz5TIOoukgwtRjYtFOlixS0MGayWxkit/GYDs/ hvqT4f6IPQD0KCn+Gk5XU9MJRpl7puLF5hAXLkz0= Received: by simark.ca (Postfix, from userid 112) id A89901E105; Tue, 14 Jan 2025 10:32:59 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1736868778; bh=BDBH5N11SciEKZ5BTcc0HPu1Ww2OCwWPz0dYowNI/xA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=tgjy1bPiDywXzjS0Va5yahJy85NlkGyGtbwY4P8DCQLAoS5/3e4jYkELbr6tcGhnj ttS+CwkdehaPmJXL18M/tKHDEJSxW19A0ygoRcckly6nXxOXI7seHo+EnIG0QO/wD9 /b61soJuwvkeiJPMuwEtr3iuam4BwTw3hFKM99Hc= Received: from [10.0.0.11] (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 73E381E05C; Tue, 14 Jan 2025 10:32:58 -0500 (EST) Message-ID: Date: Tue, 14 Jan 2025 10:32:58 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] gdbserver: convert program_args to a single string To: Andrew Burgess , gdb-patches@sourceware.org Cc: Guinevere Larsen References: Content-Language: en-US From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org On 2025-01-14 08:39, Andrew Burgess wrote: > This commit changes how gdbserver stores the inferior arguments from > being a vector of separate arguments into a single string with all of > the arguments combined together. > > Making this change might feel a little strange; intuitively it feels > like we would be better off storing the arguments as a vector, but > this change is part of a larger series of work that aims to improve > GDB's inferior argument handling. The full series was posted here: > > https://inbox.sourceware.org/gdb-patches/cover.1730731085.git.aburgess@redhat.com > > But asking people to review a 14 patch series in unreasonable, so I'm > instead posting the patches in smaller batches. This patch can stand > alone, and I do think this change makes sense on its own: > > First, GDB already stores the inferior arguments as a single string, > so doing this moves gdbserver into line with GDB. The common code > into which gdbserver calls requires the arguments to be a single > string, so currently each target's create_inferior implementation > merged the arguments anyway, so all this commit really does is move > the merging up the call stack, and store the merged result rather than > storing the separate parts. > > However, the biggest reason for why this commit is needed, is an issue > with passing arguments from GDB to gdbserver when starting a new > inferior. > > Consider: > > (gdb) set args $VAR > (gdb) run > ... > > When using a native target the inferior will see the value of $VAR > expanded by the shell GDB uses to start the inferior. However, if > using an extended-remote target the inferior will see literally $VAR, > the unexpanded name of the variable, the reason for this is that, > although GDB sends '$VAR' to gdbserver, when gdbserver receives this, > it converts this to '\$VAR', which prevents the variable from being > expanded by the shell. > > The reason for this is that construct_inferior_arguments escapes all > special shell characters within its arguments, and it is > construct_inferior_arguments that is used to combine the separate > arguments into a single string. > > In the future I will change construct_inferior_arguments so that > it can apply different escaping strategies. When this happens we will > want to escape arguments coming from the gdbserver command line > differently than arguments coming from GDB (via a vRun packet), which > means we need to call construct_inferior_arguments earlier, at the > point where we know if the arguments came from the gdbserver command > line, or from the vRun packet. > > This argument escaping issue is discussed in PR gdb/28392. > > This commit doesn't fix any issues, nor does it change > construct_inferior_arguments to actually do different escaping, that > will all come later. This is purely a restructuring. > > There should be no user visible changes after this commit. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392 > > Tested-By: Guinevere Larsen Some suggestions below, but otherwise: Approved-By: Simon Marchi > diff --git a/gdbserver/server.cc b/gdbserver/server.cc > index 55898f59556..efe63ae7515 100644 > --- a/gdbserver/server.cc > +++ b/gdbserver/server.cc > @@ -121,7 +121,20 @@ private: > /* The program name, adjusted if needed. */ > std::string m_path; > } program_path; > -static std::vector program_args; > + > +/* All program arguments are merged into a single string. This is similar > + to how GDB manages the inferior arguments, and actually makes our lives > + easier; the rules for how arguments are merged into a single string > + differ depending on where the arguments come from. Arguments arriving > + form the gdbserver command line are quoted, while arguments arriving > + from GDB (via a vRun packet) are already quoted. > + > + NOTE: The comment above is ahead of its time. The differences between > + how the PROGRAM_ARGS string is built up have not yet been implemented. > + A later patch in this series will make this change, and remove this > + note. */ I think this is a bit too much for a code comment, it belongs to the commit message (where it is already well explained). It would be enough to state what it is at the current time: /* All program arguments are merged into a single string. */ > @@ -4376,8 +4388,10 @@ captured_main (int argc, char *argv[]) > > n = argc - (next_arg - argv); > program_path.set (next_arg[0]); > + std::vector temp_arg_vector; > for (i = 1; i < n; i++) > - program_args.push_back (xstrdup (next_arg[i])); > + temp_arg_vector.push_back (next_arg[i]); > + program_args = construct_inferior_arguments (temp_arg_vector); Would that work, using std::vector's constructor that takes two iterators? std::vector temp_arg_vector (&next_arg[1], &next_arg[argc]); program_args = construct_inferior_arguments (temp_arg_vector); (not sure if the end iterator needs `argc` or `argc - 1`) or directly: program_args = construct_inferior_arguments ({&next_arg[1], &next_arg[argc]}); > diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc > index da858b65e6f..139c945a2ba 100644 > --- a/gdbserver/win32-low.cc > +++ b/gdbserver/win32-low.cc > @@ -492,12 +492,12 @@ create_process (const char *program, char *args, > > /* Start a new process. > PROGRAM is the program name. > - PROGRAM_ARGS is the vector containing the inferior's args. > + PROGRAM_ARGS is a string containing all the inferior's arguments. > Returns the new PID on success, -1 on failure. Registers the new > process with the process list. */ I think this comment should just be removed, there's no point in repeating the documentation from the base class. Simon