Re: [PATCH v2 0/6] libfdt: Fix signed/unsigned comparison warnings

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

On 02/10/2020 13:32, David Gibson wrote:
> On Fri, Oct 02, 2020 at 10:25:09AM +0100, André Przywara wrote:
>> On 02/10/2020 02:03, David Gibson wrote:
>>
>> Hi David,
>>
>>> On Thu, Oct 01, 2020 at 05:46:24PM +0100, Andre Przywara wrote:
>>>> Those are the six remaining patches of the initial post to fix the
>>>> C comparison warnings.
>>>> I reworked the fixes according to David's comments, and took quite a
>>>> different approach for some of them.
>>>> Changelog below.
>>>>
>>>> The series is against https://github.com/dgibson/dtc/commits/main
>>>> ------------------------------------
>>>>
>>>> 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.
>>>>
>>>> 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.
>>>>
>>>> Please have a look, happy to discuss the invididual cases.
>>>
>>> Thanks again for this work.  I've applied all the remaining patches,
>>> although I have some comments for some followups I think would be
>>> good.
>>
>> Thanks, that's much appreciated! Happy to discuss any further comments.
>>
>>> At this point can we turn on -Wsign-compare by default?  That sounds
>>> like a good idea to stop these problems creeping back in.
>>
>> Not quite yet: the patches were only covering "make libfdt". I see more
>> reports for dtc and the tests. I should probably address those as long
>> as this is still in my "L1 cache" ...
>> Unless you want to turn on -Wsign-compare just for libfdt.
> 
> If you think you can get us there for the whole project reasonably
> soon, then wait for that.  If it looks like you might need to shelve
> this for a while, then it would be good to get it enabled for just
> libfdt.

So I have "make tests" covered now, that looks mostly reasonable.
The rest is more nasty, I needed to paper over a few of them to make
some progress. But "make all check" comes out clean now.

I will try to make reasonable patches out this mess next week.

Cheers,
Andre