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. -- 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
