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

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2710 bytes --]

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+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> 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.

I think the very first step, is to find definitive info on what
exactly the defined behaviour of C is with a signed vs. unsigned
comparison.

The help text of -Wsign-compare seems to imply that assuming:

	signed int a;
	unsigned int b;

then 
	if (a < b) ...

is equivalent to
	if ((unsigned int)a < b) ...

But I thought that this was not the case.  Rather, I thought it was
supposed to always evaluate to true if b > INT_MAX.  We need to know
which is the case as a starting point.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]