* Re: [PATCH] sim: ppc: fix some Wpointer-sign warnings
2021-05-19 16:32 ` Mike Frysinger
@ 2021-05-20 11:59 ` Tom de Vries
2021-05-20 12:08 ` Tom de Vries
2021-09-09 3:17 ` Mike Frysinger
2 siblings, 0 replies; 5+ messages in thread
From: Tom de Vries @ 2021-05-20 11:59 UTC (permalink / raw)
To: gdb-patches, andrew.burgess
[-- Attachment #1: Type: text/plain, Size: 558 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
>
Committed separately, as attached.
Thanks,
- Tom
[-- Attachment #2: 0001-sim-ppc-fix-Wpointer-sign-warning.patch --]
[-- Type: text/x-patch, Size: 1217 bytes --]
sim: ppc: fix Wpointer-sign warning
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:194:75: error: pointer targets in passing \
argument 4 of 'device_find_integer_array_property' differ in signedness \
[-Werror=pointer-sign]
int nr_cells
= device_find_integer_array_property(me, "available", 0, &dummy);
^
...
Fix this by changing the type of dummy.
---
sim/ppc/hw_memory.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sim/ppc/hw_memory.c b/sim/ppc/hw_memory.c
index 09c331c3295..46b22f7b6e3 100644
--- 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);
if ((nr_cells % 2) != 0)
device_error(me, "property \"available\" invalid - contains an odd number of cells");
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] sim: ppc: fix some Wpointer-sign warnings
2021-05-19 16:32 ` Mike Frysinger
2021-05-20 11:59 ` Tom de Vries
@ 2021-05-20 12:08 ` Tom de Vries
2021-09-09 3:17 ` Mike Frysinger
2 siblings, 0 replies; 5+ messages in thread
From: Tom de Vries @ 2021-05-20 12:08 UTC (permalink / raw)
To: gdb-patches, andrew.burgess
[-- 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 */
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] sim: ppc: fix some Wpointer-sign warnings
2021-05-19 16:32 ` Mike Frysinger
2021-05-20 11:59 ` Tom de Vries
2021-05-20 12:08 ` Tom de Vries
@ 2021-09-09 3:17 ` Mike Frysinger
2 siblings, 0 replies; 5+ messages in thread
From: Mike Frysinger @ 2021-09-09 3:17 UTC (permalink / raw)
To: Tom de Vries, gdb-patches, andrew.burgess
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread