Re: [PATCH v2 0/6] libfdt: Fix signed/unsigned comparison warnings

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

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

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.

At this point can we turn on -Wsign-compare by default?  That sounds
like a good idea to stop these problems creeping back in.

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