[5/5] sim/iq2000: silence pointer-sign warnings

Message ID 89f7a81c43e4b9acf725de1668f7754692e6dd83.1665578246.git.aburgess@redhat.com
State Committed
Commit feab6abfe23b5b1724cb3c00059254e8f1bc5225
Headers
Series Silence some build warnings in various simulators |

Commit Message

Andrew Burgess Oct. 12, 2022, 12:38 p.m. UTC
  When building the iq2000 simulator I see a few warnings like this:

  /tmp/build/sim/../../src/sim/iq2000/iq2000.c: In function ‘fetch_str’:
  /tmp/build/sim/../../src/sim/iq2000/iq2000.c:50:54: error: pointer targets in passing argument 3 of ‘sim_read’ differ in signedness [-Werror=pointer-sign]
     50 |   sim_read (CPU_STATE (current_cpu), CPU2DATA(addr), buf, nr);
        |                                                      ^~~
        |                                                      |
        |                                                      char *

I've silenced these warnings by casting buf to 'unsigned char *'.
With this change I now see no warnings when compiling iq2000.c, so
I've removed the line from Makefile.in that disables -Werror.

Makefile.in was also disabling -Werror when compiling mloop.c,
however, I'm not seeing any warnings when compiling that file, so I've
removed the -Werror disable in that case too.
---
 sim/iq2000/Makefile.in | 3 ---
 sim/iq2000/iq2000.c    | 9 ++++++---
 2 files changed, 6 insertions(+), 6 deletions(-)
  

Comments

Mike Frysinger Oct. 23, 2022, 12:32 p.m. UTC | #1
On 12 Oct 2022 13:38, Andrew Burgess via Gdb-patches wrote:
> When building the iq2000 simulator I see a few warnings like this:
> 
>   /tmp/build/sim/../../src/sim/iq2000/iq2000.c: In function ‘fetch_str’:
>   /tmp/build/sim/../../src/sim/iq2000/iq2000.c:50:54: error: pointer targets in passing argument 3 of ‘sim_read’ differ in signedness [-Werror=pointer-sign]
>      50 |   sim_read (CPU_STATE (current_cpu), CPU2DATA(addr), buf, nr);
>         |                                                      ^~~
>         |                                                      |
>         |                                                      char *
> 
> I've silenced these warnings by casting buf to 'unsigned char *'.
> With this change I now see no warnings when compiling iq2000.c, so
> I've removed the line from Makefile.in that disables -Werror.

i left this warning in place so someone would fix it properly rather than
just cast it away
-mike
  
Andrew Burgess Oct. 24, 2022, 3:45 p.m. UTC | #2
Mike Frysinger <vapier@gentoo.org> writes:

> On 12 Oct 2022 13:38, Andrew Burgess via Gdb-patches wrote:
>> When building the iq2000 simulator I see a few warnings like this:
>> 
>>   /tmp/build/sim/../../src/sim/iq2000/iq2000.c: In function ‘fetch_str’:
>>   /tmp/build/sim/../../src/sim/iq2000/iq2000.c:50:54: error: pointer targets in passing argument 3 of ‘sim_read’ differ in signedness [-Werror=pointer-sign]
>>      50 |   sim_read (CPU_STATE (current_cpu), CPU2DATA(addr), buf, nr);
>>         |                                                      ^~~
>>         |                                                      |
>>         |                                                      char *
>> 
>> I've silenced these warnings by casting buf to 'unsigned char *'.
>> With this change I now see no warnings when compiling iq2000.c, so
>> I've removed the line from Makefile.in that disables -Werror.
>
> i left this warning in place so someone would fix it properly rather than
> just cast it away

Sorry for not understanding that.  If you'd like to expand on "properly"
I'll take a pass at fixing it.

Thanks,
Andrew
  
Mike Frysinger Oct. 26, 2022, 8:51 a.m. UTC | #3
On 24 Oct 2022 16:45, Andrew Burgess wrote:
> Mike Frysinger <vapier@gentoo.org> writes:
> 
> > On 12 Oct 2022 13:38, Andrew Burgess via Gdb-patches wrote:
> >> When building the iq2000 simulator I see a few warnings like this:
> >> 
> >>   /tmp/build/sim/../../src/sim/iq2000/iq2000.c: In function ‘fetch_str’:
> >>   /tmp/build/sim/../../src/sim/iq2000/iq2000.c:50:54: error: pointer targets in passing argument 3 of ‘sim_read’ differ in signedness [-Werror=pointer-sign]
> >>      50 |   sim_read (CPU_STATE (current_cpu), CPU2DATA(addr), buf, nr);
> >>         |                                                      ^~~
> >>         |                                                      |
> >>         |                                                      char *
> >> 
> >> I've silenced these warnings by casting buf to 'unsigned char *'.
> >> With this change I now see no warnings when compiling iq2000.c, so
> >> I've removed the line from Makefile.in that disables -Werror.
> >
> > i left this warning in place so someone would fix it properly rather than
> > just cast it away
> 
> Sorry for not understanding that.  If you'd like to expand on "properly"
> I'll take a pass at fixing it.

whatever doesn't require a lot of casting :).  sprinkling casts around means
leaving landmines for ourselves in the future.

in this particular case, i think the only reasonable answer is to change the
sim_read & sim_write APIs to work on void* much like the C library funcs.  i
get that void pointers are basically implicit casts, but i think that's still
the right way to go unless i'm forgetting something obvious.

i also vaguely recall that we really shouldn't be using sim_read & sim_write
in sim backends in the first place, but instead leaning on sim_core_* APIs.
-mike
  

Patch

diff --git a/sim/iq2000/Makefile.in b/sim/iq2000/Makefile.in
index 9f64adcecb4..757ed28ef6b 100644
--- a/sim/iq2000/Makefile.in
+++ b/sim/iq2000/Makefile.in
@@ -37,9 +37,6 @@  ALL_CPU_CFLAGS = -DHAVE_CPU_IQ2000BF -DHAVE_CPU_IQ10BF
 
 SIM_EXTRA_CLEAN = iq2000-clean
 
-# Some modules don't build cleanly yet.
-iq2000.o mloop.o: SIM_WERROR_CFLAGS =
-
 ## COMMON_POST_CONFIG_FRAG
 
 arch = iq2000
diff --git a/sim/iq2000/iq2000.c b/sim/iq2000/iq2000.c
index b685a31b07a..2c01434c308 100644
--- a/sim/iq2000/iq2000.c
+++ b/sim/iq2000/iq2000.c
@@ -47,7 +47,8 @@  fetch_str (SIM_CPU *current_cpu, PCADDR pc, DI addr)
                           pc, read_map, CPU2DATA(addr + nr)) != 0)
     nr++;
   buf = NZALLOC (char, nr + 1);
-  sim_read (CPU_STATE (current_cpu), CPU2DATA(addr), buf, nr);
+  sim_read (CPU_STATE (current_cpu), CPU2DATA(addr), (unsigned char *) buf,
+	    nr);
   return buf;
 }
 
@@ -82,7 +83,8 @@  do_syscall (SIM_CPU *current_cpu, PCADDR pc)
 
     case TARGET_NEWLIB_SYS_write:
       buf = zalloc (PARM3);
-      sim_read (CPU_STATE (current_cpu), CPU2DATA(PARM2), buf, PARM3);
+      sim_read (CPU_STATE (current_cpu), CPU2DATA(PARM2),
+		(unsigned char *) buf, PARM3);
       SET_H_GR (ret_reg,
 		sim_io_write (CPU_STATE (current_cpu),
 			      PARM1, buf, PARM3));
@@ -105,7 +107,8 @@  do_syscall (SIM_CPU *current_cpu, PCADDR pc)
       SET_H_GR (ret_reg,
 		sim_io_read (CPU_STATE (current_cpu),
 			     PARM1, buf, PARM3));
-      sim_write (CPU_STATE (current_cpu), CPU2DATA(PARM2), buf, PARM3);
+      sim_write (CPU_STATE (current_cpu), CPU2DATA(PARM2),
+		 (unsigned char *) buf, PARM3);
       free (buf);
       break;