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@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > > > 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:
signature.asc
Description: PGP signature
