Re: [PATCH] libfdt: Correct signed/unsigned comparisons

[Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]

Hi André,

On Thu, 17 Sep 2020 at 04:27, André Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
>
> On 17/01/2020 09:23, David Gibson wrote:
>
> Hi,
>
> > On Thu, Jan 16, 2020 at 08:58:12PM +1300, Simon Glass wrote:
> >> Hi David,
> >>
> >> On Thu, 16 Jan 2020 at 19:50, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6iEOx6SR5NKn@public.gmane.orgu> wrote:
> >>>
> >>> On Sun, Jan 12, 2020 at 11:52:08AM -0700, Simon Glass wrote:
> >>>> These warnings appear when building U-Boot on x86 and some other targets.
> >>>> Correct them by adding casts.
> >>>>
> >>>> Example:
> >>>>
> >>>> scripts/dtc/libfdt/fdt.c: In function ‘fdt_offset_ptr’:
> >>>> scripts/dtc/libfdt/fdt.c:137:18: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
> >>>>    if ((absoffset < offset)
> >>>>
> >>>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >>>
> >>> Hmm.  So squashing warnings is certainly a good thing in general.
> >>>
> >>> Unfortunately, I'm really uncomfortable with most of these changes.
> >>> In a number of cases they are outright wrong.  In most of the others,
> >>> the code was already correct.  I dislike adding casts to suppress
> >>> spurious warnings on correct code because that can end up hiding real
> >>> problems which might be introduced by future changes.
> >>>
> >>> Case by case details below.
> >>
> >> Thanks for the review. I agree this is all really horrible,
> >> particularly in light of the fact that it doesn't fix bugs and perhaps
> >> introduces some.
> >>
> >> This was just a quick patch to silence the warnings. If we do make
> >> fixes here we should really make sure that there are test cases to
> >> trigger each check. I suspect we have some but not all.
> >
> > Yeah, adding some safety test cases for egregiously bad input like
> > negative buffer sizes is probably a good idea.
> >
> >> What do you think we should do? The warnings are going to be very
> >> annoying for people. I could perhaps split the patch up and work
> >> through things one by one.
> >
> > Yeah, we want to find some way to remove the warnings, and I think
> > splitting up and working piece by piece will be necessary.
>
> Has anyone done anything on that front?
> If not, I would take a deep breath and try to tackle this one by one. I
> was grudgingly ignoring this in U-Boot so far, but it popped up in
> Trusted Firmware now as well, so I have a business reason (TM).

No, I never got back to it. It would be great if you could take it on!

Regards,
Simon