On Fri, Jun 11, 2021 at 06:10:35PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness in
> the different legs of the conditional operator, in fdtget.c.
>
> In the questionable expression, we are constructing a 16-bit value out of
> two unsigned 8-bit values, however are relying on the compiler's
> automatic expansion of the uint8_t to a larger type, to survive the left
> shift. This larger type happens to be an "int", so this part of the
> expression becomes signed.
>
> Fix this by explicitly blowing up the uint8_t to a larger *unsigned* type,
> before doing the left shift. And while we are at it, convert the hardly
> readable conditional operator usage into a sane switch/case expression.
>
> This fixes "make fdtget", when compiled with -Wsign-compare.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
Hmm... this code is pretty confusing. I can't actually see any path
where size will be set to something other than 4, 1 or -1.
The change looks correct, despite that. Though I wonder if we should
add an fdt16_ld() helper for this case.
> ---
> fdtget.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fdtget.c b/fdtget.c
> index 777582e..1f6eb73 100644
> --- a/fdtget.c
> +++ b/fdtget.c
> @@ -62,8 +62,14 @@ static int show_cell_list(struct display_info *disp, const char *data, int len,
> for (i = 0; i < len; i += size, p += size) {
> if (i)
> printf(" ");
> - value = size == 4 ? fdt32_ld((const fdt32_t *)p) :
> - size == 2 ? (*p << 8) | p[1] : *p;
> + switch (size) {
> + case 4: value = fdt32_ld((const fdt32_t *)p); break;
> + case 2: value = ((unsigned)(*p) << 8) | p[1]; break;
> + case 1:
> + default:
> + value = *p;
> + break;
> + }
> printf(fmt, value);
> }
>
--
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
