Re: [PATCH 00/14] libfdt: Fix signed/unsigned comparison warnings

["André Przywara" <andre.przywara-5wv7dgnIgG8@public.gmane.org>]

On 25/09/2020 05:12, David Gibson wrote:

Hi David,

> On Mon, Sep 21, 2020 at 05:52:49PM +0100, Andre Przywara wrote:
>> When libfdt is compiled with -Wsign-compare or -Wextra, GCC emits quite
>> some warnings about the signedness of the operands not matching:
>> =================
>> libfdt/fdt.c:140:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>>    if ((absoffset < offset)
>> .....
>> =================
>>
>> This does not occur under normal conditions in the dtc repo, but might
>> show up when libfdt is embedded in another project. There have been reports
>> from U-Boot and Trusted-Firmware-A.
>>
>> The underlying issue is mostly due to C's promotion behaviour (ANSI C
>> section 6.1.3.8) when dealing with operands of different signedness
>> (but same size): Signed values get implictly casted to unsigned, which
>> is not typically what we want if they could have been negative.
>>
>> The Internet(TM) suggests that blindly applying casts is probably doing
>> more harm than it helps, so this series tries to fix the underlying
>> issues properly.
>> In libfdt, some types are somewhat suboptimal ("int bufsize" comes to mind);
>> some signed types are due to them being returned along wih error values in
>> other functions (node offsets).
>> So these fixes here have been based on the following assumptions:
>> - We cannot change the prototype of exported functions.
>> - It's better to change types (for local variables) than to cast.
>> - If we have established that a signed value is not negative, we can safely
>>   cast it to an unsigned type.
>>
>> I split up the fixes in small chunks, to make them easier to review.
>> The first four patches change types, the next six ones use casts after
>> we made sure the values are not negative.
>>
>> This is only covering libfdt for now (which is what those other projects
>> care about). There are more issues with dtc, but they can be addressed
>> later.
>>
>> Not sure if some of these checks should be gated by can_assume() calls.
>>
>> Please have a look, happy to discuss the invididual cases.
> 
> Thanks so much for these.  I've applied about half of them, the rest I
> have mostly minor comments for.

Many thanks for reviewing (and applying)! I will send the remaining
patches ASAP, fixed accordingly.

Cheers,
Andre