Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Sergio Durigan Junior <sergiodj@redhat.com>
To: Pedro Alves <palves@redhat.com>
Cc: GDB Patches <gdb-patches@sourceware.org>
Subject: Re: [PATCH] Assert that 'length' > 0 on infcmd.c:construct_inferior_arguments
Date: Wed, 29 Jan 2020 20:24:00 -0000	[thread overview]
Message-ID: <87ftfym2yl.fsf@redhat.com> (raw)
In-Reply-To: <9d7548f2-68e5-4b58-2fc2-9745b50e1dd4@redhat.com> (Pedro Alves's	message of "Wed, 29 Jan 2020 20:13:47 +0000")

On Wednesday, January 29 2020, Pedro Alves wrote:

> On 1/29/20 7:54 PM, Sergio Durigan Junior wrote:
>> On Wednesday, January 29 2020, Pedro Alves wrote:
>
>>> Uppercase ARGC.  But putting
>>>
>>>  gdb_assert (argc > 0);
>>>
>>> at the top of the function instead as I originally suggested also works
>>> for me (tried current gcc master), which seems a bit better to me, as it
>>> covers both branches at once.  Did it not work for you?  This makes gcc see
>>> that the loops always run at least once.
>> 
>> Yes, this should work.
>> 
>> Updated patch below.
>
> The version you sent won't build for sure -- you're checking
> length instead of argc:
>
>  @@ -268,6 +268,11 @@ construct_inferior_arguments (int argc, char **argv)
>   {
>     char *result;
>   
>  +  /* argc should always be at least 1, but we double check this
>  +     here.  This is also needed to silence -Werror-stringop
>  +     warnings.  */
>  +  gdb_assert (length > 0);

Congrats, this is the new challenge called "How many interactions does
it take to get a simple patch accepted?"

Sorry about this brain fart.  I should slow down.

> This is OK with that fixed, but please update the commit log, here:
>
>  "The solution here is to explicit check that 'length' is greater than
>  0.  Since we're dealing with 'argc', I think it's pretty much
>  guaranteed that it's going to be at least 1."
>
> I'd go with:
>
>  The solution here is to assert that 'argc' is greater than
>  0 on entry, which makes GCC understand that the loops always
>  run at least once, and thus 'length' is always > 0.
>
> ... and also update the commit's subject.  I'd use
>
>  Fix -Werror-stringop error on infcmd.c:construct_inferior_arguments
>
> instead, to talk in terms of what you're fixing instead of how
> you're fixing it.

OK, I'll do those things *calmly* and put your name as the author of the
patch.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/


  reply	other threads:[~2020-01-29 20:18 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-29 18:05 Sergio Durigan Junior
2020-01-29 19:20 ` Pedro Alves
2020-01-29 19:58   ` Sergio Durigan Junior
2020-01-29 20:04     ` Sergio Durigan Junior
2020-01-29 20:18     ` Pedro Alves
2020-01-29 20:24       ` Sergio Durigan Junior [this message]
2020-01-29 23:25         ` Sergio Durigan Junior

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87ftfym2yl.fsf@redhat.com \
    --to=sergiodj@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=palves@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox