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
