From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 108833 invoked by alias); 17 Mar 2017 10:27:02 -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 108819 invoked by uid 89); 17 Mar 2017 10:27:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 spammy=H*RU:!192.168.0.101!, Hx-spam-relays-external:!192.168.0.101!, H*r:ip*192.168.0.101 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, 17 Mar 2017 10:27:00 +0000 Received: by mail-wm0-f45.google.com with SMTP id 196so7863801wmm.1 for ; Fri, 17 Mar 2017 03:27:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=NsRvxhAmosZAfFKMLi5wSOlCMPC9D9tKBxvBXqBxx4U=; b=dgAdwHfTxpsQEcrASDgQujfy7nJ9T3cAU9Og4l4wwXIPVHqEiVFMBMyZINbXQBM/lA RcQM1KIEfkzrpPJotTdMKSo+dWLM4xzQPaW1h6+DzSbrbWBCnuJH/Xog6EcToKGInUgM dHSA/idi3EPv70nZGH6S4DPXwEoEQpHG0QUuQ8AF1f7p7IPBwF1B+SgOmvSjs6bF8GY2 8Gp49ealuD7GACoQlK7KSNqsbvBx31PMCSzA0g5q7Dv35dWArnsqmiFrruRpeJ7SLZAX yhqZVehNiQNYFedrXkOn90aOKDmyyrtw5OmXJRV6DZMzslLMZrI/CwE1DT+UhymkJVur +HkQ== X-Gm-Message-State: AFeK/H2bVwxywRZQpE89dCKi3S/C1W6EuccKBoxRjodRBDTx6OJrhnhq9IlX6XgaXaw+qKcU X-Received: by 10.28.218.80 with SMTP id r77mr2139668wmg.0.1489746419142; Fri, 17 Mar 2017 03:26:59 -0700 (PDT) Received: from [192.168.0.101] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id 73sm2139006wml.19.2017.03.17.03.26.56 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 17 Mar 2017 03:26:57 -0700 (PDT) Subject: Re: [PATCH v4 4/5] Share fork_inferior et al with gdbserver To: Sergio Durigan Junior References: <20170208032257.15443-1-sergiodj@redhat.com> <20170308052931.25398-1-sergiodj@redhat.com> <20170308052931.25398-5-sergiodj@redhat.com> <025ebdb9-90d9-d54a-c055-57ed2406b812@redhat.com> <871stwk95q.fsf@redhat.com> Cc: GDB Patches , Luis Machado From: Pedro Alves Message-ID: Date: Fri, 17 Mar 2017 10:27: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: <871stwk95q.fsf@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-03/txt/msg00311.txt.bz2 On 03/17/2017 01:02 AM, Sergio Durigan Junior wrote: > On Monday, March 13 2017, Pedro Alves wrote: >> >> [I think at least some of my comments I sent earlier >> today on v3 still apply, so I won't repeat them.] > > You mean I haven't addressed everything you commented? If that is the > case, I truly apologize. I'll go over the comments again. No, I meant what I said. :-) I had sent comments to patches in v3 on the same day immediately before reading v4 and replying to this thread, and some of the comments I had sent to v3 applied to v4 too, so I didn't repeat them against v4: https://sourceware.org/ml/gdb-patches/2017-03/msg00189.html https://sourceware.org/ml/gdb-patches/2017-03/msg00190.html https://sourceware.org/ml/gdb-patches/2017-03/msg00191.html > Offhand I don't see how to get rid of these functions (on fork_inferior) > without creating even more stubs on gdbserver, but I'll see. The first line of action would be I think to look at why do we even need to reference inferior_ptid there, see if we can do something else that avoids it. >>> - if (new_argv[0] == NULL) >>> + if (new_argv.empty () || new_argv[0] == NULL) >>> { >>> /* GDB didn't specify a program to run. Use the program from the >>> last run with the new argument list. */ >>> >>> - if (program_argv == NULL) >>> + if (program_argv.empty ()) >>> { >>> write_enn (own_buf); >>> - freeargv (new_argv); >>> + free_vector_argv (new_argv); >>> return 0; >>> } >>> >>> - new_argv[0] = strdup (program_argv[0]); >>> - if (new_argv[0] == NULL) >>> + new_argv.push_back (strdup (program_argv[0])); >>> + if (new_argv.empty () || new_argv[0] == NULL) >> >> On the "empty()" check: you've just pushed an element to the vector, >> so the vector can't be empty. > > Indeed. > >> On the "== NULL" check: IIUC, the old NULL check was there to >> handle strdup returning NULL due to out-of-memory. >> See NULL checks and comments further above in this function. >> Now that you're using a std::vector, that doesn't work or make >> sense any longer, since if push_back fails to allocate space for >> its internal buffer (with operator new), our operator new replacement >> (common/new-op.c) calls malloc_failure, which aborts gdbserver. >> >> Not sure it makes sense to handle out-of-memory specially in >> the gdb/rsp-facing functions nowadays (maybe git blame/log/patch >> submission for that code shows some guidelines). Maybe (or, probably) >> it's OK to stop caring about it, but then we should consistently remove >> left over code, by using xstrdup instead and remove the NULL checks. > > OK, so from what I understood, this part of the code can be removed, > right? I guess removing all the leftover code is something to be done > in another patch series. Well, it only becomes "left over code" when we switch to use vector. Is it possible to split these C++fication changes (std::vector/std::string) to a separate preparatory patch? >>> +void >>> +free_vector_argv (std::vector &v) >>> +{ >>> + for (char *&i : v) >> >> Iterating over pointers, no need for reference indirection, >> copy is fine: >> >> for (char *el: v) >> xfree (el); > > Wait, I could swear I had removed this! Anyway, doing it now, thanks. You had removed it elsewhere in the patch, but missed here. > >>> + xfree (i); >>> + >>> + v.clear (); >>> +} >>> + >>> +/* See common/common-utils.h. */ >>> + >>> +std::string >>> +stringify_argv (const std::vector &argv) >>> +{ >>> + std::string ret; >>> + >>> + for (auto s : argv) >>> + ret += s + std::string (" "); >>> + >>> + /* Erase the last whitespace. */ >>> + ret.erase (ret.end () - 1); >> >> Are we always sure ARGV wasn't empty here? > > No. And I see what you mean: we should check it before erasing the last > whitespace. Will fix. Exactly. >>> @@ -627,6 +625,8 @@ win32_create_inferior (char *program, char **program_args) >>> int argc; >>> PROCESS_INFORMATION pi; >>> DWORD err; >>> + std::string program_args = stringify_argv (program_argv); >>> + char *args = (char *) program_args.c_str (); >> >> Why do we need the cast? > > Because "args" is a char *? I haven't checked if "args" could be made a > const, but I can. Right, why is it that "char *" needs to be non-const. The cast was just what made the alarm bell ring. :-) Thanks, Pedro Alves