Re: [PATCH] sim: ppc: fix some Wpointer-sign warnings

[Mike Frysinger via Gdb-patches <gdb-patches@sourceware.org>]

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

On 19 May 2021 12:32, Mike Frysinger via Gdb-patches wrote:
> On 19 May 2021 12:46, Tom de Vries wrote:
> > --- a/sim/ppc/hw_memory.c
> > +++ b/sim/ppc/hw_memory.c
> > @@ -199,9 +199,9 @@ hw_memory_init_address(device *me)
> >  	 cell_nr += 2) {
> >        hw_memory_chunk *new_chunk = ZALLOC(hw_memory_chunk);
> >        device_find_integer_array_property(me, "available", cell_nr,
> > -					 &new_chunk->address);
> > +					 (signed_cell *)&new_chunk->address);
> >        device_find_integer_array_property(me, "available", cell_nr + 1,
> > -					 &new_chunk->size);
> > +					 (signed_cell *)&new_chunk->size);
> > 
> > --- a/sim/ppc/hw_opic.c
> > +++ b/sim/ppc/hw_opic.c
> > @@ -417,10 +417,12 @@ hw_opic_init_data(device *me)
> >        }
> >        if (!device_find_integer_array_property(me, "interrupt-ranges",
> >  					      reg_nr * 2,
> > -					      &opic->isu_block[isb].int_number)
> > +					      (signed_cell *)
> > +					        &opic->isu_block[isb].int_number)
> >  	  || !device_find_integer_array_property(me, "interrupt-ranges",
> >  						 reg_nr * 2 + 1,
> > -						 &opic->isu_block[isb].range))
> > +						 (signed_cell *)
> > +						   &opic->isu_block[isb].range))
> 
> these ones i'm not sure about.  it does fix the warnings, and it doesn't
> change the status quo behavior, but i don't think it's the actual fix we
> would want.  if the device tree has a negative number, it'll get converted
> to an unsigned number.  i haven't thought hard as to what the right fix
> would look like here.
> 
> i think we'd have to look at what other device tree users are doing like
> in boot loaders (e.g. u-boot) and in the linux kernel.

i poked this one a bit today.  i think your cast is OK here.  feel free to
correct anything below that i get wrong.

the dtc/libfdt code internally processes the fdt blob as a series of bytes
without any type information.  typing is left to the caller.  in the libfdt
code, they have core APIs that read/write bytes, and a few helper functions
to cast/convert those bytes to the right value (e.g. a u32).  in the ppc
sim code, the core APIs use signed integers, and then the callers convert
to unsigned, usually implicitly (e.g. the device_find_integer_property call
is assigned to an unsigned variable _a lot_).  it seems like the code just
doesn't happen to use unsigned & arrays a lot, so that's why there's only
these few places that tickle the signed warning errors.

we could add some core APIs to the ppc sim that deal with raw bytes and then
add some helpers to convert to the right type, but that seems like a lot of
lifting for what boils down to your cast here.  and i think a lot of the ppc
code should either get converted to existing sim common code, or we should
stand up proper APIs in the common code first, or use standard libraries to
do all the processing (e.g. libfdt).  either way, this device.c code would
all get deleted, and callers (like these hw_*.c files) would get converted.

i'll merge your fix with a bit more detail, and enable the warning too.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]