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. Cheers, Andre
