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

[Tom de Vries <tdevries@suse.de>]

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

On 5/19/21 6:32 PM, Mike Frysinger wrote:
> On 19 May 2021 12:46, Tom de Vries wrote:
>> --- a/sim/ppc/hw_memory.c
>> +++ b/sim/ppc/hw_memory.c
>> @@ -190,7 +190,7 @@ hw_memory_init_address(device *me)
>>    if (device_find_property(me, "available") != NULL) {
>>      hw_memory_chunk **curr_chunk = &hw_memory->heap;
>>      int cell_nr;
>> -    unsigned_cell dummy;
>> +    signed_cell dummy;
>>      int nr_cells = device_find_integer_array_property(me, "available", 0, &dummy);
> 
> this one is fine
> 
>> @@ -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.

Ack.  That's out of scope for me though, so I'm gonna carry this updated
patch locally, and if not fixed otherwise, this'll end up in the
openSUSE gdb package.

Thanks,
- Tom



[-- Attachment #2: 0001-sim-ppc-silence-some-Wpointer-sign-warnings.patch --]
[-- Type: text/x-patch, Size: 2386 bytes --]

sim: ppc: silence some Wpointer-sign warnings

When compiling with --enable-werror and CFLAGS="-O0 -g -Wall", we run into:
...
src/sim/ppc/hw_memory.c: In function ‘hw_memory_init_address’:
src/sim/ppc/hw_memory.c:204:7: error: pointer targets in passing argument 
4 \
  of ‘device_find_integer_array_property’ differ in signedness \
  [-Werror=pointer-sign]
       &new_chunk->size);
       ^
...

Silence the warnings by adding an explicit pointer cast.  This makes the type
conversion explicit, and doesn't change the current behavior.

This may actually need a proper fix, but that's out of scope for this patch,
which merely aims to fix the build with parameters as used by the openSUSE gdb
package.

---
 sim/ppc/hw_memory.c | 4 ++--
 sim/ppc/hw_opic.c   | 6 ++++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/sim/ppc/hw_memory.c b/sim/ppc/hw_memory.c
index 46b22f7b6e3..c0826b71139 100644
--- 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);
       new_chunk->available = 1;
       *curr_chunk = new_chunk;
       curr_chunk = &new_chunk->next;
diff --git a/sim/ppc/hw_opic.c b/sim/ppc/hw_opic.c
index 8afe312a7ef..9404204aa2f 100644
--- 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))
 	device_error(me, "missing or invalid interrupt-ranges property entry %d", reg_nr);
       /* first reg entry specifies the address of both the IDU and the
          first set of ISU registers, adjust things accordingly */