[PATCH][gdb/build] Fix -Werror=bool-compare warning in update_static_array_size

[Tom de Vries]

On 11/18/20 4:50 PM, Simon Marchi wrote:
> On 2020-11-18 4:29 a.m., Tom de Vries wrote:
>> [ was: Re: [pushed] gdb: make get_array_bounds return bool ]
>>
>> On 11/18/20 12:47 AM, Simon Marchi via Gdb-patches wrote:
>>> Obvious change from int to bool.  I took the opportunity to move the doc
>>> to the header file.
>>>
>>> gdb/ChangeLog:
>>>
>>> 	* gdbtypes.h (get_array_bounds): Return bool, adjust some
>>> 	callers.  Move doc here.
>>> 	* gdbtypes.c (get_array_bounds): Return bool
>>>
>>
>> I'm running into a build breaker (build with CFLAGS -g -O0 -Wall,
>> configured with --enable-gdb-build-warnings --enable-werror):
>> ...
>> src/gdb/gdbtypes.c: In function 'bool update_static_array_size(type*)':
>> src/gdb/gdbtypes.c:1250:64: error: comparison of constant '0' with
>> boolean expression is always true [-Werror=bool-compare]
>>     && get_array_bounds (element_type, &low_bound, &high_bound) >= 0
>>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> ...
> 
> Oh, I see it too, my mistake.  I disabled -Werror in my build in order
> to bisect and build old commits that would give warnings, so I missed
> this warning (I re-enabled -Werror now).
> 

Yeah, I know what you mean, I also manage this manually in my build
script, which is not ideal.  FWIW, I'm recently playing with the idea of
doing this version-based, by parsing gdb/version.h.in or some such, but
haven't gotten round to playing around with that.

>> This is due to this change, and code introduced by recent commit:
>> ...
>> commit b72795a8f573c36aaeedd1a64f58ad52b4c72439
>> Author: Tom Tromey <tromey@adacore.com>
>> Date:   Wed Nov 4 08:49:16 2020 -0700
>>
>>     Fix bit strides for -fgnat-encodings=minimal
>> ...
>> which compares get_array_bounds >= 0:
>> ...
>> +      /* If this array's element is itself an array with a bit stride,
>> +        then we want to update this array's bit stride to reflect the
>> +        size of the sub-array.  Otherwise, we'll end up using the
>> +        wrong size when trying to find elements of the outer
>> +        array.  */
>> +      if (element_type->code () == TYPE_CODE_ARRAY
>> +         && TYPE_LENGTH (element_type) != 0
>> +         && TYPE_FIELD_BITSIZE (element_type, 0) != 0
>> +         && get_array_bounds (element_type, &low_bound, &high_bound) >= 0
>> +         && high_bound >= low_bound)
>> +       TYPE_FIELD_BITSIZE (type, 0)
>> +         = ((high_bound - low_bound + 1)
>> +            * TYPE_FIELD_BITSIZE (element_type, 0));
>> +
>> ...
>>
>> I suppose that this code was written with the return codes of
>> get_discrete_bounds in mind, where -1 means "bounds not set".
>>
>> So, I think this can be fixed by just dropping the compare:
>> ...
>> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
>> index cde07702734..b822a369a16 100644
>> --- a/gdb/gdbtypes.c
>> +++ b/gdb/gdbtypes.c
>> @@ -1247,7 +1247,7 @@ update_static_array_size (struct type *type)
>>        if (element_type->code () == TYPE_CODE_ARRAY
>>           && TYPE_LENGTH (element_type) != 0
>>           && TYPE_FIELD_BITSIZE (element_type, 0) != 0
>> -         && get_array_bounds (element_type, &low_bound, &high_bound) >= 0
>> +         && get_array_bounds (element_type, &low_bound, &high_bound)
>>           && high_bound >= low_bound)
>>         TYPE_FIELD_BITSIZE (type, 0)
>>           = ((high_bound - low_bound + 1)
>> ...
>>
>> I'll put this through a round of build and test on x86_64-linux.
>>
>> Any comments?
> 
> Looks good, thanks for cleaning up after me.
> 

Np of course :) , committed.

Thanks,
- Tom