diff mbox

Assert that 'length' > 0 on infcmd.c:construct_inferior_arguments

Message ID 87blqmm2o2.fsf@redhat.com
State New
Headers show

Commit Message

Sergio Durigan Junior Jan. 29, 2020, 8:24 p.m. UTC
On Wednesday, January 29 2020, I wrote:

> 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.

Here's the patch I ended up pushing.

c47f70e2ce7b347aadbde873aae6c2df92c42180

Thanks,
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ee2ba1c38d..535249196a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@ 
+2020-01-29  Pedro Alves  <palves@redhat.com>
+	    Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	* infcmd.c (construct_inferior_arguments): Assert that
+	'argc' is greater than 0.
+
 2020-01-29  Luis Machado  <luis.machado@linaro.org>
 
 	* aarch64-tdep.c (BRK_INSN_MASK): Define to 0xffe0001f.
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index b44adca88d..62890bde2a 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -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 (argc > 0);
+
   if (startup_with_shell)
     {
 #ifdef __MINGW32__