From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id UMITKoiJh2d71A8AWB0awg (envelope-from ) for ; Wed, 15 Jan 2025 05:10:16 -0500 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=NpEcS1wG; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id A8FA91E100; Wed, 15 Jan 2025 05:10:16 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-6.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=unavailable autolearn_force=no version=4.0.0 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 1DE301E08E for ; Wed, 15 Jan 2025 05:10:16 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C1887385DC29 for ; Wed, 15 Jan 2025 10:10:15 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C1887385DC29 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=NpEcS1wG Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id B3E23385DDCD for ; Wed, 15 Jan 2025 10:09:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B3E23385DDCD Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org B3E23385DDCD Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736935770; cv=none; b=q0zVV8E9h0Xg0a6loTmLN9Qs1kJJDovlu7HvGbahtQHxo2hF4NlxXtNZyUtNbMAExeMu//jbV4ETeL/+apoA84VsBziZGAiO04rlkDiGLJDbl/wxFY1ur5PbQXM6whjLTj+jx8I0E6uHmHBNB4nc1aCPvqtkl868BTz6pLZ56hk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736935770; c=relaxed/simple; bh=bKaf3DffubZwwzxEIn247qyANtT0SNiQmjCfr4oa62s=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=eX8C84I75CeUJVyDvJrG3TBbqL9Xjc2Eqdjh0Hcw/mpzxGTEYiWCuPOrSBdO8MpKcBizdZc3hAWws33vjgqhSKryjZovSU58LXghLlGk+BolZzI8eo/PYXlUZ56v6Reptv2uJEiYgQBoCtDaY+G39JvpNWb0fjU46Xj3gFA6NLg= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B3E23385DDCD DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1736935770; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=c0auYawJKUNjNCBnk518a8gX3w6GJm97Yz1o5g3ryjg=; b=NpEcS1wGee2rO09fCKMfzeuc19fjztS/IXum50m8wW0vN8Dd0vU5WQypAMgd5xCG655SBJ JttWcuBdGCy0nqW1gBVdMVkkLolDsdkVdlGDWUZepExUmkOSihAwozPL7IwAFe40HYyAzF TNbP+5oP7nLyrnABSEs1We3mBiAcb6Y= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-99-I_sxOJLnNFq0_Gb9g-7Sgg-1; Wed, 15 Jan 2025 05:09:29 -0500 X-MC-Unique: I_sxOJLnNFq0_Gb9g-7Sgg-1 X-Mimecast-MFC-AGG-ID: I_sxOJLnNFq0_Gb9g-7Sgg Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-43621907030so25980805e9.1 for ; Wed, 15 Jan 2025 02:09:29 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736935768; x=1737540568; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=c0auYawJKUNjNCBnk518a8gX3w6GJm97Yz1o5g3ryjg=; b=IN1Rjni6TQfSuFxR5nt7RUMdiCT+XbtN9Vp9KLe1Zp0wLm6zOAm8SSIMqPyRclRMzT duiGU8nJ4YbCyvPpkG1hPThcxwNVi1TiKmcpav9xvf2QLN1SkIVtAhIPRm0Ih8QZD5bg e5phybS4mzn1YBfTyK4XIssLx+UL+hHO/mOcqUeAPzdgvit41I5KoOgxLx4wmNXIyP2K Bz7D+Uz2g7rwWXYpSQ2qW23IOBpHMUE+arh4lqUWLqgoi0jxi1UAuEgo+HEikcGYT7mI a/rxLEubOOXcnJGnj7j69FDHRn1cDWO4/rlN152n8ntX9UE5NHGfCsMc6MT3OyvElfhK Fing== X-Forwarded-Encrypted: i=1; AJvYcCWcPu6RKzyp7pGAjT+NnIniBQjYnEcnkLs3Tgr25/7XED48oGXBt6ltarqEoSOf6wbmpncGRutktM4Zyg==@sourceware.org X-Gm-Message-State: AOJu0YwfPEQu3Yp+Cng5OE6vTSidenqqk+y5rVw5khZ4jlOHOLpS72x1 EM0usAn0CkUsSIfmYYYRc+ZmhfsLphO+m43pFZM5QrNWHgEjE5vK/2kwN8p/zwCFK6OATIOkdyh OkJ3vthAqpOPzrtKPdUZ/jl9dBjOnlNCt6UTqWHGQG+u/YOOtvSbPynG5RZk= X-Gm-Gg: ASbGncsp06d5+XFFQZrj9EefVx+CQ8cW/ASNWDbqz/rGFpmNPxU+G1F2+5w7AKJt1yr w4TUyIQkOQxDaXGrfD7oZseI43v4V5lh/n8ZSsBHDY5whmwAAkv9ZXD6eV7rUcp5VsgRkY3zKSD YDZSQWdyDjrYHPAK2TdV8HlHHF08HLyBqNDR37PstU1WCYIGF8Q8Kil7owPfoyIIr5Z6YasaMW7 RY1o+4mT+3OERb0X86P/3wbixzn1XFrr9RxTcAwR+GSkdBsQbHlM84RiKuK8iZUqa2j9YfNiCoN 3nUNBg== X-Received: by 2002:a05:600c:4f4e:b0:436:1ac2:1acf with SMTP id 5b1f17b1804b1-436e26e28cemr249438745e9.20.1736935768084; Wed, 15 Jan 2025 02:09:28 -0800 (PST) X-Google-Smtp-Source: AGHT+IFNlDKiuBVsubFus1DRAAM+pKZBnXCzBBFecjuwpd/vQJLhgeySNxe/5w1WprQ6qB4sMwwSTA== X-Received: by 2002:a05:600c:4f4e:b0:436:1ac2:1acf with SMTP id 5b1f17b1804b1-436e26e28cemr249438455e9.20.1736935767707; Wed, 15 Jan 2025 02:09:27 -0800 (PST) Received: from localhost (44.226.159.143.dyn.plus.net. [143.159.226.44]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-437c749a127sm17961195e9.7.2025.01.15.02.09.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Jan 2025 02:09:27 -0800 (PST) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Cc: Guinevere Larsen Subject: Re: [PATCH] gdbserver: convert program_args to a single string In-Reply-To: References: Date: Wed, 15 Jan 2025 10:09:26 +0000 Message-ID: <87ikqgpew9.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: tz8ib6eaW8F1O6sxY-L3YDObKm3R_FgMU9I0oSQoH1U_1736935768 X-Mimecast-Originator: redhat.com Content-Type: text/plain 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 Simon Marchi writes: > 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 I made the improvements you suggested, and pushed this. Thanks, Andrew > >> 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