From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by sourceware.org (Postfix) with ESMTPS id 7AD7C386F81E for ; Tue, 12 May 2020 15:57:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 7AD7C386F81E Received: from submission (posteo.de [89.146.220.130]) by mout02.posteo.de (Postfix) with ESMTPS id 242DD2400FE for ; Tue, 12 May 2020 17:57:58 +0200 (CEST) Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 49M2Wl6sm9z6tmY; Tue, 12 May 2020 17:57:55 +0200 (CEST) Subject: Re: [PATCH v2 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases To: Simon Marchi , Christian Biesinger , gdb-patches References: <20200429111638.1327262-1-m.weghorn@posteo.de> <20200429111638.1327262-3-m.weghorn@posteo.de> From: Michael Weghorn Message-ID: Date: Tue, 12 May 2020 17:57:50 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 May 2020 15:58:01 -0000 On 07/05/2020 21.49, Simon Marchi wrote: > Hi Michael, > > I think the patch looks good, there are just a few nits to fix, and a suggestion. > >> diff --git a/gdb/infcmd.c b/gdb/infcmd.c >> index 8f7482347c..3ac64b3164 100644 >> --- a/gdb/infcmd.c >> +++ b/gdb/infcmd.c >> @@ -151,12 +151,9 @@ get_inferior_args (void) >> { >> if (current_inferior ()->argc != 0) >> { >> - char *n; >> - >> - n = construct_inferior_arguments (current_inferior ()->argc, >> + std::string n = construct_inferior_arguments (current_inferior ()->argc, >> current_inferior ()->argv); > > Align the last line with the parenthesis, like it was before the change. Done in the new version: https://sourceware.org/pipermail/gdb-patches/2020-May/168347.html > >> - set_inferior_args (n); >> - xfree (n); >> + set_inferior_args (n.c_str()); > > Space before parenthesis. > Also done. >> } >> >> if (current_inferior ()->args == NULL) >> diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc >> index 71b9a11e02..3f117d5ef0 100644 >> --- a/gdbsupport/common-inferior.cc >> +++ b/gdbsupport/common-inferior.cc >> @@ -28,15 +28,15 @@ bool startup_with_shell = true; >> /* Compute command-line string given argument vector. This does the >> same shell processing as fork_inferior. */ >> >> -char * >> -construct_inferior_arguments (int argc, char **argv) >> +std::string >> +construct_inferior_arguments (int argc, char * const *argv) >> { >> - char *result; >> + gdb_assert (argc >= 0); >> + if (argc == 0 || argv[0] == NULL) { >> + return ""; >> + } > > Omit the braces when there is just one single line statement. > > When there are braces, places them like this: > > if (condition) > { > stmt1; > stmt2; > } > > Did you actually encounter the case (argc > 0 && argv[0] == NULL)? In > other words, does it really constitute a valid input to this function? > The gdbserver code used to add an extra NULL arg, so that case would have been hit by just replacing the existing 'stringify_argv' calls by 'construct_inferior_args' (as done in the follow-up patch [1]) and starting a program without args (like 'gdbserver localhost:50505 ./main'). I had only added the patch to actually remove that extra arg later [2]. With that patch in place that should actually not have happened any more, so was indeed unnecessary. In any case, I have removed that in the next version along with the switch to using a std::string for the result right away as suggested. [1] https://sourceware.org/pipermail/gdb-patches/2020-April/168038.html [2] https://sourceware.org/pipermail/gdb-patches/2020-April/168040.html >> >> - /* ARGC should always be at least 1, but we double check this >> - here. This is also needed to silence -Werror-stringop >> - warnings. */ >> - gdb_assert (argc > 0); >> + char *result; >> >> if (startup_with_shell) >> { >> @@ -145,5 +145,8 @@ construct_inferior_arguments (int argc, char **argv) >> } >> } >> >> - return result; >> + std::string str_result(result); > > Space before parenthesis. Done. > > If we are going to return a string anyway, I'd vote for building the result > into an std::string directly. You can remove the part that tries to guess > the length of the string, and use the appropriate std::string methods / > operators to append to it. > > If you do this, I'm thinking that the `argc == 0` check at the beginning of > the function might be unnecessary, as the code could handle it naturally: > all the loops will be skipped and we'll return an empty string. True, thanks for the hint. I have adapted this accordingly. Michael