From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 116317 invoked by alias); 7 Apr 2017 18:30:23 -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 116246 invoked by uid 89); 7 Apr 2017 18:30:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-8.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE,UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy=sooner, Fork, tiny X-HELO: mail-wm0-f45.google.com Received: from mail-wm0-f45.google.com (HELO mail-wm0-f45.google.com) (74.125.82.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Apr 2017 18:30:18 +0000 Received: by mail-wm0-f45.google.com with SMTP id t189so13735814wmt.1 for ; Fri, 07 Apr 2017 11:30:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=BNw9/zcdkLFzOnL8sPYjRlIbKGZ8LcpuD2V5cdDnAGs=; b=AD0vOioJoqEjU3ty0wB2t84L7Fxq94GXZhXLeTmPd64oeX7RcMXXwR7shTu3+rXfVh 6CrFWWnWjwCXF84C2pEdAeDsE/gI4Yfo0b6L05aJVXtIAPVxnKXL8rrvXci6eT16iOFe FesMTv9NWQ3myWCDdOlW029/ckL8NaVKCv6VfDA4hxci3rGInRuN0QQJrVjmAS5/KDcF aCLOzCZv3F2zH0atwo/0ow1syYTCijO5Nm82rhhd+GED/1jpQg4F7bnqNXyBaTc0Q4/n 4C9u+52HNU5wvOfW91WzP2pEbJHqAYg/sYkuJPw8kDre2jQ5oIOawBqOb1pjnJ+xz5V2 U80w== X-Gm-Message-State: AN3rC/7hxRxCQQPZQgrlZ0AZuRFU7tFW1hLJACuBIMZZKc4flUFaNSHGnSn4wRjuTBlPw6EF X-Received: by 10.28.18.21 with SMTP id 21mr551869wms.77.1491589816808; Fri, 07 Apr 2017 11:30:16 -0700 (PDT) Received: from [192.168.0.101] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id j138sm7244640wmg.10.2017.04.07.11.30.15 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 07 Apr 2017 11:30:16 -0700 (PDT) From: Pedro Alves Subject: Re: [PATCH v5 3/5] C++-fy and prepare for sharing fork_inferior To: Sergio Durigan Junior , GDB Patches References: <1482464361-4068-1-git-send-email-sergiodj@redhat.com> <20170330014915.4894-1-sergiodj@redhat.com> <20170330014915.4894-4-sergiodj@redhat.com> Message-ID: Date: Fri, 07 Apr 2017 18:30: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: <20170330014915.4894-4-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-04/txt/msg00188.txt.bz2 On 03/30/2017 02:49 AM, Sergio Durigan Junior wrote: > As a preparation for the next patch, which will move fork_inferior > from GDB to common/ (and therefore share it with gdbserver), it is > interesting to convert a few functions to C++. I've meanwhile realized that fork_inferior should move to "nat/", not "common/" ... This is code only used by the native targets. Sorry for not pointing it out sooner... :-/. > This patch touches functions related to parsing command-line arguments > to the inferior (see gdb/fork-child.c:breakup_args), the way the > arguments are stored on fork_inferior (using std::vector instead of > char **), and the code responsible for dealing with argv also on > gdbserver. > > I've taken this opportunity and decided to constify a few arguments to > fork_inferior/create_inferior as well, in order to make the code > cleaner. And now, on gdbserver, we're using xstrdup everywhere and > aren't checking for memory allocation failures anymore, as requested > by Pedro. It'd be good to say here _why_ that's the right thing to do. I.e., write to the future hacker doing archaeology. > --- a/gdb/fork-child.c > +++ b/gdb/fork-child.c > @@ -1,4 +1,4 @@ > -/* Fork a Unix child process, and set up to debug it, for GDB. > + /* Fork a Unix child process, and set up to debug it, for GDB. Spurious whitespace change. > static void > -breakup_args (char *scratch, char **argv) > +breakup_args (const std::string &scratch, std::vector &argv) > { ... > + > + std::string arg = scratch.substr (cur_pos, next_sep - cur_pos); > + This creates a temporary string (heap allocates) ... > + argv.push_back (xstrdup (arg.c_str ())); ... and here you create yet another copy. You should be able to avoid it by using e.g., savestring: char *arg = savestring (scratch.c_str () + cur_pos, next_sep - cur_pos); argv.push_back (arg); > + > + cur_pos = next_sep; > } > > - /* Null-terminate the vector. */ > - *argv = NULL; > + /* NULL-terminating the vector. */ FYI, the non-gerund version of the text reads as more natural English to me. > + argv.push_back (NULL); > } > > --- a/gdb/go32-nat.c > +++ b/gdb/go32-nat.c > @@ -631,8 +631,9 @@ go32_kill_inferior (struct target_ops *ops) > } > > static void > -go32_create_inferior (struct target_ops *ops, char *exec_file, > - char *args, char **env, int from_tty) > +go32_create_inferior (struct target_ops *ops, > + const char *exec_file, > + const std::string &allargs, char **env, int from_tty) > { > extern char **environ; > jmp_buf start_state; > @@ -641,6 +642,7 @@ go32_create_inferior (struct target_ops *ops, char *exec_file, > size_t cmdlen; > struct inferior *inf; > int result; > + char *args = (char *) allargs.c_str (); AFAICS, this could be const. Note that when you really need to append a single character, it's a tiny bit more efficient to write what you mean: shell_command += ' '; instead of appending a string that happens to have one character: + shell_command += " "; + shell_command += "'"; because the later means you're calling the operator+= overload that needs to handle / count the string length. I saw a few of those in the patch. Anyway, with those addressed and with any missing xstrdup that -Wwrite-string may have flagged added, this is good to go. I believe this stands on its own and doesn't have any dependency on the previous patches, so please go ahead and push. Thanks, Pedro Alves