[PATCH 0/5] Silence some build warnings in various simulators

[PATCH 0/5] Silence some build warnings in various simulators

[Andrew Burgess]

This series silences some simulator build warnings.

---

Andrew Burgess (5):
  sim/cgen: mask uninitialized variable warning in cgen-run.c
  sim/ppc: fix warnings related to printf format strings
  sim/ppc: mark device_error function as ATTRIBUTE_NORETURN
  sim/erc32: avoid dereferencing type-punned pointer warnings
  sim/iq2000: silence pointer-sign warnings

 sim/common/cgen-run.c  |  5 +++++
 sim/erc32/Makefile.in  |  3 ---
 sim/erc32/exec.c       | 12 +++++++++---
 sim/iq2000/Makefile.in |  3 ---
 sim/iq2000/iq2000.c    |  9 ++++++---
 sim/ppc/device.h       |  2 +-
 sim/ppc/igen.c         |  2 +-
 sim/ppc/ld-cache.c     |  4 ++--
 sim/ppc/ld-decode.c    |  2 +-
 sim/ppc/ld-insn.c      | 41 +++++++++++++++--------------------------
 10 files changed, 40 insertions(+), 43 deletions(-)

-- 
2.25.4

[PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c

[Andrew Burgess]

I see an uninitialized variable warning (with gcc 9.3.1) from
cgen-run.c, like this:

  /tmp/build/sim/../../src/sim/cris/../common/cgen-run.c: In function ‘sim_resume’:
  /tmp/build/sim/../../src/sim/cris/../common/cgen-run.c:259:5: warning: ‘engine_fns$’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    259 |    (* engine_fns[next_cpu_nr]) (cpu);
        |    ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /tmp/build/sim/../../src/sim/cris/../common/cgen-run.c:232:14: note: ‘engine_fns$’ was declared here
    232 |   ENGINE_FN *engine_fns[MAX_NR_PROCESSORS];
        |              ^~~~~~~~~~

This is a false positive - we over allocate engine_fn, and then only
initialize the nr_cpus entries which we will later go on to use.

However, we can easily silence this warning by initializing the unused
entries in engine_fns to NULL, this might also help if anyone ever
looks at engine_fns in a debugger, it should now be obvious which
entries are in use, and which are not.

With this change the warning is gone.

There should be no change in behaviour with this commit.
---
 sim/common/cgen-run.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sim/common/cgen-run.c b/sim/common/cgen-run.c
index 9a13b0ca416..a9a493c01b9 100644
--- a/sim/common/cgen-run.c
+++ b/sim/common/cgen-run.c
@@ -242,6 +242,11 @@ engine_run_n (SIM_DESC sd, int next_cpu_nr, int nr_cpus, int max_insns, int fast
       prime_cpu (cpu, max_insns);
     }
 
+  /* Ensure the remaining engine_fns slots are initialized, this silences a
+     compiler warning when engine_fns is used below.  */
+  for (i = nr_cpus; i < MAX_NR_PROCESSORS; ++i)
+    engine_fns[i] = NULL;
+
   while (1)
     {
       SIM_ENGINE_PREFIX_HOOK (sd);
-- 
2.25.4

Re: [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c

[Mike Frysinger]

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

On 12 Oct 2022 13:38, Andrew Burgess via Gdb-patches wrote:
> --- a/sim/common/cgen-run.c
> +++ b/sim/common/cgen-run.c
> @@ -242,6 +242,11 @@ engine_run_n (SIM_DESC sd, int next_cpu_nr, int nr_cpus, int max_insns, int fast
>        prime_cpu (cpu, max_insns);
>      }
>  
> +  /* Ensure the remaining engine_fns slots are initialized, this silences a
> +     compiler warning when engine_fns is used below.  */
> +  for (i = nr_cpus; i < MAX_NR_PROCESSORS; ++i)
> +    engine_fns[i] = NULL;

engine_fns is declared in this func.  why not assign it and let gcc handle
the rest ?
-  ENGINE_FN *engine_fns[MAX_NR_PROCESSORS];
+  ENGINE_FN *engine_fns[MAX_NR_PROCESSORS] = {};
-mike

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

Re: [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c

[Andrew Burgess]

Mike Frysinger <vapier@gentoo.org> writes:

> On 12 Oct 2022 13:38, Andrew Burgess via Gdb-patches wrote:
>> --- a/sim/common/cgen-run.c
>> +++ b/sim/common/cgen-run.c
>> @@ -242,6 +242,11 @@ engine_run_n (SIM_DESC sd, int next_cpu_nr, int nr_cpus, int max_insns, int fast
>>        prime_cpu (cpu, max_insns);
>>      }
>>  
>> +  /* Ensure the remaining engine_fns slots are initialized, this silences a
>> +     compiler warning when engine_fns is used below.  */
>> +  for (i = nr_cpus; i < MAX_NR_PROCESSORS; ++i)
>> +    engine_fns[i] = NULL;
>
> engine_fns is declared in this func.  why not assign it and let gcc handle
> the rest ?
> -  ENGINE_FN *engine_fns[MAX_NR_PROCESSORS];
> +  ENGINE_FN *engine_fns[MAX_NR_PROCESSORS] = {};

How's the patch below?

Thanks,
Andrew

---

commit 79f4f1d82d1da482e223079deb453eda7b2d2323
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Mon Oct 24 16:55:07 2022 +0100

    sim/cgen: initialize variable at creation in engine_run_n
    
    Zero initialize engine_fns entirely at creation, then override those
    fields we intend to use, rather than zero just initializing the unused
    fields later on.
    
    There should be no user visible changes after this commit.

diff --git a/sim/common/cgen-run.c b/sim/common/cgen-run.c
index a9a493c01b9..1ace067a395 100644
--- a/sim/common/cgen-run.c
+++ b/sim/common/cgen-run.c
@@ -229,7 +229,7 @@ static void
 engine_run_n (SIM_DESC sd, int next_cpu_nr, int nr_cpus, int max_insns, int fast_p)
 {
   int i;
-  ENGINE_FN *engine_fns[MAX_NR_PROCESSORS];
+  ENGINE_FN *engine_fns[MAX_NR_PROCESSORS] = {};
 
   SIM_ASSERT (nr_cpus <= MAX_NR_PROCESSORS);
   SIM_ASSERT (next_cpu_nr >= 0 && next_cpu_nr < nr_cpus);
@@ -242,11 +242,6 @@ engine_run_n (SIM_DESC sd, int next_cpu_nr, int nr_cpus, int max_insns, int fast
       prime_cpu (cpu, max_insns);
     }
 
-  /* Ensure the remaining engine_fns slots are initialized, this silences a
-     compiler warning when engine_fns is used below.  */
-  for (i = nr_cpus; i < MAX_NR_PROCESSORS; ++i)
-    engine_fns[i] = NULL;
-
   while (1)
     {
       SIM_ENGINE_PREFIX_HOOK (sd);

Re: [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c

[Mike Frysinger]

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

On 24 Oct 2022 16:57, Andrew Burgess wrote:
> Mike Frysinger <vapier@gentoo.org> writes:
> commit 79f4f1d82d1da482e223079deb453eda7b2d2323
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Mon Oct 24 16:55:07 2022 +0100
> 
>     sim/cgen: initialize variable at creation in engine_run_n
>     
>     Zero initialize engine_fns entirely at creation, then override those
>     fields we intend to use, rather than zero just initializing the unused
>     fields later on.
>     
>     There should be no user visible changes after this commit.
> 
> diff --git a/sim/common/cgen-run.c b/sim/common/cgen-run.c
> index a9a493c01b9..1ace067a395 100644
> --- a/sim/common/cgen-run.c
> +++ b/sim/common/cgen-run.c
> @@ -229,7 +229,7 @@ static void
>  engine_run_n (SIM_DESC sd, int next_cpu_nr, int nr_cpus, int max_insns, int fast_p)
>  {
>    int i;
> -  ENGINE_FN *engine_fns[MAX_NR_PROCESSORS];
> +  ENGINE_FN *engine_fns[MAX_NR_PROCESSORS] = {};
>  
>    SIM_ASSERT (nr_cpus <= MAX_NR_PROCESSORS);
>    SIM_ASSERT (next_cpu_nr >= 0 && next_cpu_nr < nr_cpus);
> @@ -242,11 +242,6 @@ engine_run_n (SIM_DESC sd, int next_cpu_nr, int nr_cpus, int max_insns, int fast
>        prime_cpu (cpu, max_insns);
>      }
>  
> -  /* Ensure the remaining engine_fns slots are initialized, this silences a
> -     compiler warning when engine_fns is used below.  */

this comment is useful, so i would retain it

otherwise lgtm
-mike

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

Re: [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c

[Andrew Burgess]

Mike Frysinger <vapier@gentoo.org> writes:

> On 24 Oct 2022 16:57, Andrew Burgess wrote:
>> Mike Frysinger <vapier@gentoo.org> writes:
>> commit 79f4f1d82d1da482e223079deb453eda7b2d2323
>> Author: Andrew Burgess <aburgess@redhat.com>
>> Date:   Mon Oct 24 16:55:07 2022 +0100
>> 
>>     sim/cgen: initialize variable at creation in engine_run_n
>>     
>>     Zero initialize engine_fns entirely at creation, then override those
>>     fields we intend to use, rather than zero just initializing the unused
>>     fields later on.
>>     
>>     There should be no user visible changes after this commit.
>> 
>> diff --git a/sim/common/cgen-run.c b/sim/common/cgen-run.c
>> index a9a493c01b9..1ace067a395 100644
>> --- a/sim/common/cgen-run.c
>> +++ b/sim/common/cgen-run.c
>> @@ -229,7 +229,7 @@ static void
>>  engine_run_n (SIM_DESC sd, int next_cpu_nr, int nr_cpus, int max_insns, int fast_p)
>>  {
>>    int i;
>> -  ENGINE_FN *engine_fns[MAX_NR_PROCESSORS];
>> +  ENGINE_FN *engine_fns[MAX_NR_PROCESSORS] = {};
>>  
>>    SIM_ASSERT (nr_cpus <= MAX_NR_PROCESSORS);
>>    SIM_ASSERT (next_cpu_nr >= 0 && next_cpu_nr < nr_cpus);
>> @@ -242,11 +242,6 @@ engine_run_n (SIM_DESC sd, int next_cpu_nr, int nr_cpus, int max_insns, int fast
>>        prime_cpu (cpu, max_insns);
>>      }
>>  
>> -  /* Ensure the remaining engine_fns slots are initialized, this silences a
>> -     compiler warning when engine_fns is used below.  */
>
> this comment is useful, so i would retain it

I added a variant of this comment back, and pushed this patch.

Thanks,
Andrew

---

commit a09f33be653fb112586be126f3d5ab848aaed095
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Mon Oct 24 16:55:07 2022 +0100

    sim/cgen: initialize variable at creation in engine_run_n
    
    Zero initialize engine_fns entirely at creation, then override those
    fields we intend to use, rather than zero just initializing the unused
    fields later on.
    
    There should be no user visible changes after this commit.

diff --git a/sim/common/cgen-run.c b/sim/common/cgen-run.c
index a9a493c01b9..b6400a69c13 100644
--- a/sim/common/cgen-run.c
+++ b/sim/common/cgen-run.c
@@ -229,7 +229,9 @@ static void
 engine_run_n (SIM_DESC sd, int next_cpu_nr, int nr_cpus, int max_insns, int fast_p)
 {
   int i;
-  ENGINE_FN *engine_fns[MAX_NR_PROCESSORS];
+  /* Ensure that engine_fns is fully initialized, this silences a compiler
+     warning when engine_fns is used below.  */
+  ENGINE_FN *engine_fns[MAX_NR_PROCESSORS] = {};
 
   SIM_ASSERT (nr_cpus <= MAX_NR_PROCESSORS);
   SIM_ASSERT (next_cpu_nr >= 0 && next_cpu_nr < nr_cpus);
@@ -242,11 +244,6 @@ engine_run_n (SIM_DESC sd, int next_cpu_nr, int nr_cpus, int max_insns, int fast
       prime_cpu (cpu, max_insns);
     }
 
-  /* Ensure the remaining engine_fns slots are initialized, this silences a
-     compiler warning when engine_fns is used below.  */
-  for (i = nr_cpus; i < MAX_NR_PROCESSORS; ++i)
-    engine_fns[i] = NULL;
-
   while (1)
     {
       SIM_ENGINE_PREFIX_HOOK (sd);

[PATCH 2/5] sim/ppc: fix warnings related to printf format strings

[Andrew Burgess]

This commit is a follow on to:

  commit 182421c9d2eea8c4877d983a2124e591f0aca710
  Date:   Tue Oct 11 15:02:08 2022 +0100

      sim/ppc: fixes for arguments to printf style functions

where commit 182421c9d2ee addressed issues with printf format
arguments that were causing the compiler to give an error, this commit
addresses issues that caused the compiler to emit a warning.

This commit is mostly either changing the format string to match the
argument, or in some cases, excess, unused arguments are removed.
---
 sim/ppc/igen.c      |  2 +-
 sim/ppc/ld-cache.c  |  4 ++--
 sim/ppc/ld-decode.c |  2 +-
 sim/ppc/ld-insn.c   | 41 +++++++++++++++--------------------------
 4 files changed, 19 insertions(+), 30 deletions(-)

diff --git a/sim/ppc/igen.c b/sim/ppc/igen.c
index 6a6dbc30f31..27b48638276 100644
--- a/sim/ppc/igen.c
+++ b/sim/ppc/igen.c
@@ -442,7 +442,7 @@ main(int argc,
 	        code |= generate_with_icache;
                 break;
               default:
-                error (NULL, "Expecting -Ggen-icache or -Ggen-icache=<N>\n");
+		error ("Expecting -Ggen-icache or -Ggen-icache=<N>\n");
               }
           }
 	}
diff --git a/sim/ppc/ld-cache.c b/sim/ppc/ld-cache.c
index f57f7db650a..b30b1f4d898 100644
--- a/sim/ppc/ld-cache.c
+++ b/sim/ppc/ld-cache.c
@@ -88,13 +88,13 @@ static void
 dump_cache_rule(cache_table* rule,
 		int indent)
 {
-  dumpf(indent, "((cache_table*)0x%x\n", rule);
+  dumpf(indent, "((cache_table*)0x%p\n", rule);
   dumpf(indent, " (type %s)\n", i2name(rule->type, cache_type_map));
   dumpf(indent, " (field_name \"%s\")\n", rule->field_name);
   dumpf(indent, " (derived_name \"%s\")\n", rule->derived_name);
   dumpf(indent, " (type-def \"%s\")\n", rule->type_def);
   dumpf(indent, " (expression \"%s\")\n", rule->expression);
-  dumpf(indent, " (next 0x%x)\n", rule->next);
+  dumpf(indent, " (next 0x%p)\n", rule->next);
   dumpf(indent, " )\n");
 }
 
diff --git a/sim/ppc/ld-decode.c b/sim/ppc/ld-decode.c
index 68d9f5f4f52..83ff1216b30 100644
--- a/sim/ppc/ld-decode.c
+++ b/sim/ppc/ld-decode.c
@@ -120,7 +120,7 @@ dump_decode_rule(decode_table *rule,
     dumpf(indent, " (special_mask 0x%x)\n", rule->special_mask);
     dumpf(indent, " (special_value 0x%x)\n", rule->special_value);
     dumpf(indent, " (special_constant 0x%x)\n", rule->special_constant);
-    dumpf(indent, " (next 0x%x)\n", rule->next);
+    dumpf(indent, " (next 0x%p)\n", rule->next);
   }
   dumpf(indent, " )\n");
 }
diff --git a/sim/ppc/ld-insn.c b/sim/ppc/ld-insn.c
index 3910af3fdf6..c89c81c1073 100644
--- a/sim/ppc/ld-insn.c
+++ b/sim/ppc/ld-insn.c
@@ -827,29 +827,18 @@ static void
 dump_insn_field(insn_field *field,
 		int indent)
 {
-
-  printf("(insn_field*)0x%lx\n", (unsigned long)field);
-
-  dumpf(indent, "(first %d)\n", field->first);
-
-  dumpf(indent, "(last %d)\n", field->last);
-
-  dumpf(indent, "(width %d)\n", field->width);
-
+  printf ("(insn_field*)0x%p\n", field);
+  dumpf (indent, "(first %d)\n", field->first);
+  dumpf (indent, "(last %d)\n", field->last);
+  dumpf (indent, "(width %d)\n", field->width);
   if (field->is_int)
-    dumpf(indent, "(is_int %d)\n", field->val_int);
-
+    dumpf (indent, "(is_int %d)\n", field->val_int);
   if (field->is_slash)
-    dumpf(indent, "(is_slash)\n");
-
+    dumpf (indent, "(is_slash)\n");
   if (field->is_string)
-    dumpf(indent, "(is_string `%s')\n", field->val_string);
-  
-  dumpf(indent, "(next 0x%x)\n", field->next);
-  
-  dumpf(indent, "(prev 0x%x)\n", field->prev);
-  
-
+    dumpf (indent, "(is_string `%s')\n", field->val_string);
+  dumpf (indent, "(next 0x%p)\n", field->next);
+  dumpf (indent, "(prev 0x%p)\n", field->prev);
 }
 
 static void
@@ -860,13 +849,13 @@ dump_insn_fields(insn_fields *fields,
 
   printf("(insn_fields*)%p\n", fields);
 
-  dumpf(indent, "(first 0x%x)\n", fields->first);
-  dumpf(indent, "(last 0x%x)\n", fields->last);
+  dumpf(indent, "(first 0x%p)\n", fields->first);
+  dumpf(indent, "(last 0x%p)\n", fields->last);
 
   dumpf(indent, "(value 0x%x)\n", fields->value);
 
   for (i = 0; i < insn_bit_size; i++) {
-    dumpf(indent, "(bits[%d] ", i, fields->bits[i]);
+    dumpf(indent, "(bits[%d]", i);
     dump_insn_field(fields->bits[i], indent+1);
     dumpf(indent, " )\n");
   }
@@ -961,16 +950,16 @@ dump_insn_table(insn_table *table,
     dump_opcode_field(table->opcode, indent+1, 1);
     dumpf(indent, " )\n");
 
-    dumpf(indent, "(nr_entries %d)\n", table->entries);
+    dumpf(indent, "(nr_entries %d)\n", table->nr_entries);
     dumpf(indent, "(entries ");
     dump_insn_table(table->entries, indent+1, table->nr_entries);
     dumpf(indent, " )\n");
 
-    dumpf(indent, "(sibling ", table->sibling);
+    dumpf(indent, "(sibling ");
     dump_insn_table(table->sibling, indent+1, levels-1);
     dumpf(indent, " )\n");
 
-    dumpf(indent, "(parent ", table->parent);
+    dumpf(indent, "(parent ");
     dump_insn_table(table->parent, indent+1, 0);
     dumpf(indent, " )\n");
 
-- 
2.25.4

Re: [PATCH 2/5] sim/ppc: fix warnings related to printf format strings

[Tsukasa OI]

On 2022/10/12 21:38, Andrew Burgess via Gdb-patches wrote:
> This commit is a follow on to:
> 
>   commit 182421c9d2eea8c4877d983a2124e591f0aca710
>   Date:   Tue Oct 11 15:02:08 2022 +0100
> 
>       sim/ppc: fixes for arguments to printf style functions
> 
> where commit 182421c9d2ee addressed issues with printf format
> arguments that were causing the compiler to give an error, this commit
> addresses issues that caused the compiler to emit a warning.
> 
> This commit is mostly either changing the format string to match the
> argument, or in some cases, excess, unused arguments are removed.
> ---
>  sim/ppc/igen.c      |  2 +-
>  sim/ppc/ld-cache.c  |  4 ++--
>  sim/ppc/ld-decode.c |  2 +-
>  sim/ppc/ld-insn.c   | 41 +++++++++++++++--------------------------
>  4 files changed, 19 insertions(+), 30 deletions(-)
> 
> diff --git a/sim/ppc/igen.c b/sim/ppc/igen.c
> index 6a6dbc30f31..27b48638276 100644
> --- a/sim/ppc/igen.c
> +++ b/sim/ppc/igen.c
> @@ -442,7 +442,7 @@ main(int argc,
>  	        code |= generate_with_icache;
>                  break;
>                default:
> -                error (NULL, "Expecting -Ggen-icache or -Ggen-icache=<N>\n");
> +		error ("Expecting -Ggen-icache or -Ggen-icache=<N>\n");
>                }
>            }
>  	}
> diff --git a/sim/ppc/ld-cache.c b/sim/ppc/ld-cache.c
> index f57f7db650a..b30b1f4d898 100644
> --- a/sim/ppc/ld-cache.c
> +++ b/sim/ppc/ld-cache.c
> @@ -88,13 +88,13 @@ static void
>  dump_cache_rule(cache_table* rule,
>  		int indent)
>  {
> -  dumpf(indent, "((cache_table*)0x%x\n", rule);
> +  dumpf(indent, "((cache_table*)0x%p\n", rule);

Replacing "0x%p" with "%p" is recommended (as I submitted earlier).

Thanks,
Tsukasa

>    dumpf(indent, " (type %s)\n", i2name(rule->type, cache_type_map));
>    dumpf(indent, " (field_name \"%s\")\n", rule->field_name);
>    dumpf(indent, " (derived_name \"%s\")\n", rule->derived_name);
>    dumpf(indent, " (type-def \"%s\")\n", rule->type_def);
>    dumpf(indent, " (expression \"%s\")\n", rule->expression);
> -  dumpf(indent, " (next 0x%x)\n", rule->next);
> +  dumpf(indent, " (next 0x%p)\n", rule->next);
>    dumpf(indent, " )\n");
>  }
>  
> diff --git a/sim/ppc/ld-decode.c b/sim/ppc/ld-decode.c
> index 68d9f5f4f52..83ff1216b30 100644
> --- a/sim/ppc/ld-decode.c
> +++ b/sim/ppc/ld-decode.c
> @@ -120,7 +120,7 @@ dump_decode_rule(decode_table *rule,
>      dumpf(indent, " (special_mask 0x%x)\n", rule->special_mask);
>      dumpf(indent, " (special_value 0x%x)\n", rule->special_value);
>      dumpf(indent, " (special_constant 0x%x)\n", rule->special_constant);
> -    dumpf(indent, " (next 0x%x)\n", rule->next);
> +    dumpf(indent, " (next 0x%p)\n", rule->next);
>    }
>    dumpf(indent, " )\n");
>  }
> diff --git a/sim/ppc/ld-insn.c b/sim/ppc/ld-insn.c
> index 3910af3fdf6..c89c81c1073 100644
> --- a/sim/ppc/ld-insn.c
> +++ b/sim/ppc/ld-insn.c
> @@ -827,29 +827,18 @@ static void
>  dump_insn_field(insn_field *field,
>  		int indent)
>  {
> -
> -  printf("(insn_field*)0x%lx\n", (unsigned long)field);
> -
> -  dumpf(indent, "(first %d)\n", field->first);
> -
> -  dumpf(indent, "(last %d)\n", field->last);
> -
> -  dumpf(indent, "(width %d)\n", field->width);
> -
> +  printf ("(insn_field*)0x%p\n", field);
> +  dumpf (indent, "(first %d)\n", field->first);
> +  dumpf (indent, "(last %d)\n", field->last);
> +  dumpf (indent, "(width %d)\n", field->width);
>    if (field->is_int)
> -    dumpf(indent, "(is_int %d)\n", field->val_int);
> -
> +    dumpf (indent, "(is_int %d)\n", field->val_int);
>    if (field->is_slash)
> -    dumpf(indent, "(is_slash)\n");
> -
> +    dumpf (indent, "(is_slash)\n");
>    if (field->is_string)
> -    dumpf(indent, "(is_string `%s')\n", field->val_string);
> -  
> -  dumpf(indent, "(next 0x%x)\n", field->next);
> -  
> -  dumpf(indent, "(prev 0x%x)\n", field->prev);
> -  
> -
> +    dumpf (indent, "(is_string `%s')\n", field->val_string);
> +  dumpf (indent, "(next 0x%p)\n", field->next);
> +  dumpf (indent, "(prev 0x%p)\n", field->prev);
>  }
>  
>  static void
> @@ -860,13 +849,13 @@ dump_insn_fields(insn_fields *fields,
>  
>    printf("(insn_fields*)%p\n", fields);
>  
> -  dumpf(indent, "(first 0x%x)\n", fields->first);
> -  dumpf(indent, "(last 0x%x)\n", fields->last);
> +  dumpf(indent, "(first 0x%p)\n", fields->first);
> +  dumpf(indent, "(last 0x%p)\n", fields->last);
>  
>    dumpf(indent, "(value 0x%x)\n", fields->value);
>  
>    for (i = 0; i < insn_bit_size; i++) {
> -    dumpf(indent, "(bits[%d] ", i, fields->bits[i]);
> +    dumpf(indent, "(bits[%d]", i);
>      dump_insn_field(fields->bits[i], indent+1);
>      dumpf(indent, " )\n");
>    }
> @@ -961,16 +950,16 @@ dump_insn_table(insn_table *table,
>      dump_opcode_field(table->opcode, indent+1, 1);
>      dumpf(indent, " )\n");
>  
> -    dumpf(indent, "(nr_entries %d)\n", table->entries);
> +    dumpf(indent, "(nr_entries %d)\n", table->nr_entries);
>      dumpf(indent, "(entries ");
>      dump_insn_table(table->entries, indent+1, table->nr_entries);
>      dumpf(indent, " )\n");
>  
> -    dumpf(indent, "(sibling ", table->sibling);
> +    dumpf(indent, "(sibling ");
>      dump_insn_table(table->sibling, indent+1, levels-1);
>      dumpf(indent, " )\n");
>  
> -    dumpf(indent, "(parent ", table->parent);
> +    dumpf(indent, "(parent ");
>      dump_insn_table(table->parent, indent+1, 0);
>      dumpf(indent, " )\n");
>  

Re: [PATCH 2/5] sim/ppc: fix warnings related to printf format strings

[Andrew Burgess]

Tsukasa OI <research_trasio@irq.a4lg.com> writes:

> On 2022/10/12 21:38, Andrew Burgess via Gdb-patches wrote:
>> This commit is a follow on to:
>> 
>>   commit 182421c9d2eea8c4877d983a2124e591f0aca710
>>   Date:   Tue Oct 11 15:02:08 2022 +0100
>> 
>>       sim/ppc: fixes for arguments to printf style functions
>> 
>> where commit 182421c9d2ee addressed issues with printf format
>> arguments that were causing the compiler to give an error, this commit
>> addresses issues that caused the compiler to emit a warning.
>> 
>> This commit is mostly either changing the format string to match the
>> argument, or in some cases, excess, unused arguments are removed.
>> ---
>>  sim/ppc/igen.c      |  2 +-
>>  sim/ppc/ld-cache.c  |  4 ++--
>>  sim/ppc/ld-decode.c |  2 +-
>>  sim/ppc/ld-insn.c   | 41 +++++++++++++++--------------------------
>>  4 files changed, 19 insertions(+), 30 deletions(-)
>> 
>> diff --git a/sim/ppc/igen.c b/sim/ppc/igen.c
>> index 6a6dbc30f31..27b48638276 100644
>> --- a/sim/ppc/igen.c
>> +++ b/sim/ppc/igen.c
>> @@ -442,7 +442,7 @@ main(int argc,
>>  	        code |= generate_with_icache;
>>                  break;
>>                default:
>> -                error (NULL, "Expecting -Ggen-icache or -Ggen-icache=<N>\n");
>> +		error ("Expecting -Ggen-icache or -Ggen-icache=<N>\n");
>>                }
>>            }
>>  	}
>> diff --git a/sim/ppc/ld-cache.c b/sim/ppc/ld-cache.c
>> index f57f7db650a..b30b1f4d898 100644
>> --- a/sim/ppc/ld-cache.c
>> +++ b/sim/ppc/ld-cache.c
>> @@ -88,13 +88,13 @@ static void
>>  dump_cache_rule(cache_table* rule,
>>  		int indent)
>>  {
>> -  dumpf(indent, "((cache_table*)0x%x\n", rule);
>> +  dumpf(indent, "((cache_table*)0x%p\n", rule);
>
> Replacing "0x%p" with "%p" is recommended (as I submitted earlier).

Thanks.  I've made this change locally for now.  I'll wait to see if
there's any more feedback over the next couple of days.

Thanks,
Andrew

>
> Thanks,
> Tsukasa
>
>>    dumpf(indent, " (type %s)\n", i2name(rule->type, cache_type_map));
>>    dumpf(indent, " (field_name \"%s\")\n", rule->field_name);
>>    dumpf(indent, " (derived_name \"%s\")\n", rule->derived_name);
>>    dumpf(indent, " (type-def \"%s\")\n", rule->type_def);
>>    dumpf(indent, " (expression \"%s\")\n", rule->expression);
>> -  dumpf(indent, " (next 0x%x)\n", rule->next);
>> +  dumpf(indent, " (next 0x%p)\n", rule->next);
>>    dumpf(indent, " )\n");
>>  }
>>  
>> diff --git a/sim/ppc/ld-decode.c b/sim/ppc/ld-decode.c
>> index 68d9f5f4f52..83ff1216b30 100644
>> --- a/sim/ppc/ld-decode.c
>> +++ b/sim/ppc/ld-decode.c
>> @@ -120,7 +120,7 @@ dump_decode_rule(decode_table *rule,
>>      dumpf(indent, " (special_mask 0x%x)\n", rule->special_mask);
>>      dumpf(indent, " (special_value 0x%x)\n", rule->special_value);
>>      dumpf(indent, " (special_constant 0x%x)\n", rule->special_constant);
>> -    dumpf(indent, " (next 0x%x)\n", rule->next);
>> +    dumpf(indent, " (next 0x%p)\n", rule->next);
>>    }
>>    dumpf(indent, " )\n");
>>  }
>> diff --git a/sim/ppc/ld-insn.c b/sim/ppc/ld-insn.c
>> index 3910af3fdf6..c89c81c1073 100644
>> --- a/sim/ppc/ld-insn.c
>> +++ b/sim/ppc/ld-insn.c
>> @@ -827,29 +827,18 @@ static void
>>  dump_insn_field(insn_field *field,
>>  		int indent)
>>  {
>> -
>> -  printf("(insn_field*)0x%lx\n", (unsigned long)field);
>> -
>> -  dumpf(indent, "(first %d)\n", field->first);
>> -
>> -  dumpf(indent, "(last %d)\n", field->last);
>> -
>> -  dumpf(indent, "(width %d)\n", field->width);
>> -
>> +  printf ("(insn_field*)0x%p\n", field);
>> +  dumpf (indent, "(first %d)\n", field->first);
>> +  dumpf (indent, "(last %d)\n", field->last);
>> +  dumpf (indent, "(width %d)\n", field->width);
>>    if (field->is_int)
>> -    dumpf(indent, "(is_int %d)\n", field->val_int);
>> -
>> +    dumpf (indent, "(is_int %d)\n", field->val_int);
>>    if (field->is_slash)
>> -    dumpf(indent, "(is_slash)\n");
>> -
>> +    dumpf (indent, "(is_slash)\n");
>>    if (field->is_string)
>> -    dumpf(indent, "(is_string `%s')\n", field->val_string);
>> -  
>> -  dumpf(indent, "(next 0x%x)\n", field->next);
>> -  
>> -  dumpf(indent, "(prev 0x%x)\n", field->prev);
>> -  
>> -
>> +    dumpf (indent, "(is_string `%s')\n", field->val_string);
>> +  dumpf (indent, "(next 0x%p)\n", field->next);
>> +  dumpf (indent, "(prev 0x%p)\n", field->prev);
>>  }
>>  
>>  static void
>> @@ -860,13 +849,13 @@ dump_insn_fields(insn_fields *fields,
>>  
>>    printf("(insn_fields*)%p\n", fields);
>>  
>> -  dumpf(indent, "(first 0x%x)\n", fields->first);
>> -  dumpf(indent, "(last 0x%x)\n", fields->last);
>> +  dumpf(indent, "(first 0x%p)\n", fields->first);
>> +  dumpf(indent, "(last 0x%p)\n", fields->last);
>>  
>>    dumpf(indent, "(value 0x%x)\n", fields->value);
>>  
>>    for (i = 0; i < insn_bit_size; i++) {
>> -    dumpf(indent, "(bits[%d] ", i, fields->bits[i]);
>> +    dumpf(indent, "(bits[%d]", i);
>>      dump_insn_field(fields->bits[i], indent+1);
>>      dumpf(indent, " )\n");
>>    }
>> @@ -961,16 +950,16 @@ dump_insn_table(insn_table *table,
>>      dump_opcode_field(table->opcode, indent+1, 1);
>>      dumpf(indent, " )\n");
>>  
>> -    dumpf(indent, "(nr_entries %d)\n", table->entries);
>> +    dumpf(indent, "(nr_entries %d)\n", table->nr_entries);
>>      dumpf(indent, "(entries ");
>>      dump_insn_table(table->entries, indent+1, table->nr_entries);
>>      dumpf(indent, " )\n");
>>  
>> -    dumpf(indent, "(sibling ", table->sibling);
>> +    dumpf(indent, "(sibling ");
>>      dump_insn_table(table->sibling, indent+1, levels-1);
>>      dumpf(indent, " )\n");
>>  
>> -    dumpf(indent, "(parent ", table->parent);
>> +    dumpf(indent, "(parent ");
>>      dump_insn_table(table->parent, indent+1, 0);
>>      dumpf(indent, " )\n");
>>  

Re: [PATCH 2/5] sim/ppc: fix warnings related to printf format strings

[Mike Frysinger]

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

On 12 Oct 2022 13:38, Andrew Burgess via Gdb-patches wrote:
> --- a/sim/ppc/igen.c
> +++ b/sim/ppc/igen.c
> @@ -442,7 +442,7 @@ main(int argc,
>  	        code |= generate_with_icache;
>                  break;
>                default:
> -                error (NULL, "Expecting -Ggen-icache or -Ggen-icache=<N>\n");
> +		error ("Expecting -Ggen-icache or -Ggen-icache=<N>\n");

the file is using inconsistent whitespace now

> --- a/sim/ppc/ld-cache.c
> +++ b/sim/ppc/ld-cache.c
> @@ -88,13 +88,13 @@ static void
>  dump_cache_rule(cache_table* rule,
>  		int indent)
>  {
> -  dumpf(indent, "((cache_table*)0x%x\n", rule);
> +  dumpf(indent, "((cache_table*)0x%p\n", rule);

you don't want 0x with %p as %p already includes it

comes up multiple times in this patch
-mike

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

Re: [PATCH 2/5] sim/ppc: fix warnings related to printf format strings

[Andrew Burgess]

Mike Frysinger <vapier@gentoo.org> writes:

> On 12 Oct 2022 13:38, Andrew Burgess via Gdb-patches wrote:
>> --- a/sim/ppc/igen.c
>> +++ b/sim/ppc/igen.c
>> @@ -442,7 +442,7 @@ main(int argc,
>>  	        code |= generate_with_icache;
>>                  break;
>>                default:
>> -                error (NULL, "Expecting -Ggen-icache or -Ggen-icache=<N>\n");
>> +		error ("Expecting -Ggen-icache or -Ggen-icache=<N>\n");
>
> the file is using inconsistent whitespace now
>
>> --- a/sim/ppc/ld-cache.c
>> +++ b/sim/ppc/ld-cache.c
>> @@ -88,13 +88,13 @@ static void
>>  dump_cache_rule(cache_table* rule,
>>  		int indent)
>>  {
>> -  dumpf(indent, "((cache_table*)0x%x\n", rule);
>> +  dumpf(indent, "((cache_table*)0x%p\n", rule);
>
> you don't want 0x with %p as %p already includes it
>
> comes up multiple times in this patch

Tsukasa OI already pointed this out:

  https://sourceware.org/pipermail/gdb-patches/2022-October/192598.html

The version I pushed fixed this error.

Thanks,
Andrew

[PATCH 3/5] sim/ppc: mark device_error function as ATTRIBUTE_NORETURN

[Andrew Burgess]

The device_error function always ends up calling the error function,
which is itself marked as ATTRIBUTE_NORETURN, so it makes sense that
device_error should also be marked ATTRIBUTE_NORETURN.

Doing this resolves a few warnings from hw_ide.c about possibly
uninitialized variables - the variables are only uninitialized after
passing through a call to device_error, which obviously means the
variables are never really used uninitialized, the simulation will
terminate with the device_error call.
---
 sim/ppc/device.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sim/ppc/device.h b/sim/ppc/device.h
index bd539095160..65c85e4ddd3 100644
--- a/sim/ppc/device.h
+++ b/sim/ppc/device.h
@@ -729,7 +729,7 @@ EXTERN_DEVICE\
 (void) device_error
 (device *me,
  const char *fmt,
- ...) ATTRIBUTE_PRINTF_2;
+ ...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF_2;
 
 INLINE_DEVICE\
 (int) device_trace
-- 
2.25.4

[PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings

[Andrew Burgess]

When building the erc32 simulator I get a few warnings like this:

  /tmp/build/sim/../../src/sim/erc32/exec.c:1377:21: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
   1377 |   sregs->fs[rd] = *((float32 *) & ddata[0]);
        |                    ~^~~~~~~~~~~~~~~~~~~~~~~

The type of '& ddata[0]' will be 'uint32_t *', which is what triggers
the warning.

This commit uses an intermediate pointer of type 'char *' when
performing the type-punning, which is well-defined behaviour, and will
silence the above warning.

With this change, I now see no warnings when compiling exec.c, which
means that the line in Makefile.in that disables -Werror can be
removed.

There should be no change in behaviour after this commit.
---
 sim/erc32/Makefile.in |  3 ---
 sim/erc32/exec.c      | 12 +++++++++---
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/sim/erc32/Makefile.in b/sim/erc32/Makefile.in
index 786ae1dcc7b..41830aab726 100644
--- a/sim/erc32/Makefile.in
+++ b/sim/erc32/Makefile.in
@@ -32,9 +32,6 @@ SIM_EXTRA_CLEAN = clean-sis
 # behaviour of UART interrupt routines ...
 SIM_EXTRA_CFLAGS += -DFAST_UART -I$(srcroot)
 
-# Some modules don't build cleanly yet.
-exec.o: SIM_WERROR_CFLAGS =
-
 ## COMMON_POST_CONFIG_FRAG
 
 # `sis' doesn't need interf.o.
diff --git a/sim/erc32/exec.c b/sim/erc32/exec.c
index ef93692e7a2..af9ad9ea9ab 100644
--- a/sim/erc32/exec.c
+++ b/sim/erc32/exec.c
@@ -1345,7 +1345,8 @@ dispatch_instruction(struct pstate *sregs)
 	    if (mexc) {
 		sregs->trap = TRAP_DEXC;
 	    } else {
-		sregs->fs[rd] = *((float32 *) & data);
+		char *ptr = (char *) &data;
+		sregs->fs[rd] = *((float32 *) ptr);
 	    }
 	    break;
 	case LDDF:
@@ -1371,13 +1372,18 @@ dispatch_instruction(struct pstate *sregs)
 	    if (mexc) {
 		sregs->trap = TRAP_DEXC;
 	    } else {
+		char *ptr;
+
 		rd &= 0x1E;
 		sregs->flrd = rd;
-		sregs->fs[rd] = *((float32 *) & ddata[0]);
+
+		ptr = (char *) &ddata[0];
+		sregs->fs[rd] = *((float32 *) ptr);
 #ifdef STAT
 		sregs->nload++;	/* Double load counts twice */
 #endif
-		sregs->fs[rd + 1] = *((float32 *) & ddata[1]);
+		ptr = (char *) &ddata[1];
+		sregs->fs[rd + 1] = *((float32 *) ptr);
 		sregs->ltime = ebase.simtime + sregs->icnt + FLSTHOLD +
 			       sregs->hold + sregs->fhold;
 	    }
-- 
2.25.4

Re: [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings

[Pedro Alves]

On 2022-10-12 1:38 p.m., Andrew Burgess via Gdb-patches wrote:
> When building the erc32 simulator I get a few warnings like this:
> 
>   /tmp/build/sim/../../src/sim/erc32/exec.c:1377:21: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
>    1377 |   sregs->fs[rd] = *((float32 *) & ddata[0]);
>         |                    ~^~~~~~~~~~~~~~~~~~~~~~~
> 
> The type of '& ddata[0]' will be 'uint32_t *', which is what triggers
> the warning.
> 
> This commit uses an intermediate pointer of type 'char *' when
> performing the type-punning, which is well-defined behaviour, and will
> silence the above warning.

I'm afraid that's not correct.  That's still undefined behavior, it's just silencing
the warning.  The end result is still aliasing float32 and uint32_t objects, and risks
generating bogus code.  The char escape hatch only works if you access the object
representation via a character type.  Here, the patch is still accessing the object
representation of a uint32_t object via a floa32 type.

Here's an old article explaining strict aliasing (just one that I found via a quick google):

  https://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html

This scenario is the "CASTING TO CHAR*" one in that article.

> @@ -1345,7 +1345,8 @@ dispatch_instruction(struct pstate *sregs)
>  	    if (mexc) {
>  		sregs->trap = TRAP_DEXC;
>  	    } else {
> -		sregs->fs[rd] = *((float32 *) & data);
> +		char *ptr = (char *) &data;
> +		sregs->fs[rd] = *((float32 *) ptr);

The best IMHO is to do the type punning via a union, like e.g.:

  union { float32 f; uint32_t i; } u;
  u.i = data;
  sregs->fs[rd] = u.f;

See here in the C11 standard:

 https://port70.net/~nsz/c/c11/n1570.html#note95

and also the documentation of -fstrict-aliasing at:

  https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html

Pedro Alves

Re: [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings

[Lancelot SIX]

On Wed, Oct 12, 2022 at 03:11:27PM +0100, Pedro Alves wrote:
> On 2022-10-12 1:38 p.m., Andrew Burgess via Gdb-patches wrote:
> > When building the erc32 simulator I get a few warnings like this:
> > 
> >   /tmp/build/sim/../../src/sim/erc32/exec.c:1377:21: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
> >    1377 |   sregs->fs[rd] = *((float32 *) & ddata[0]);
> >         |                    ~^~~~~~~~~~~~~~~~~~~~~~~
> > 
> > The type of '& ddata[0]' will be 'uint32_t *', which is what triggers
> > the warning.
> > 
> > This commit uses an intermediate pointer of type 'char *' when
> > performing the type-punning, which is well-defined behaviour, and will
> > silence the above warning.
> 
> I'm afraid that's not correct.  That's still undefined behavior, it's just silencing
> the warning.  The end result is still aliasing float32 and uint32_t objects, and risks
> generating bogus code.  The char escape hatch only works if you access the object
> representation via a character type.  Here, the patch is still accessing the object
> representation of a uint32_t object via a floa32 type.
> 
> Here's an old article explaining strict aliasing (just one that I found via a quick google):
> 
>   https://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html
> 
> This scenario is the "CASTING TO CHAR*" one in that article.
> 
> > @@ -1345,7 +1345,8 @@ dispatch_instruction(struct pstate *sregs)
> >  	    if (mexc) {
> >  		sregs->trap = TRAP_DEXC;
> >  	    } else {
> > -		sregs->fs[rd] = *((float32 *) & data);
> > +		char *ptr = (char *) &data;
> > +		sregs->fs[rd] = *((float32 *) ptr);
> 
> The best IMHO is to do the type punning via a union, like e.g.:
> 
>   union { float32 f; uint32_t i; } u;
>   u.i = data;
>   sregs->fs[rd] = u.f;
> 
> See here in the C11 standard:
> 
>  https://port70.net/~nsz/c/c11/n1570.html#note95
> 
> and also the documentation of -fstrict-aliasing at:
> 
>   https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
> 

Hi,

Another well defined (at least to my knowledge) solution to this problem
is memcpy.  You could do something like:

  memcpy (&sregt->fs[rd], ddata, sizeof (float32));

I tend to find this more straightforward than the type punning version,
but I would be happy with either.

Best,
Lancelot.

> Pedro Alves

Re: [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings

[Andrew Burgess]

Lancelot SIX <lsix@lancelotsix.com> writes:

> On Wed, Oct 12, 2022 at 03:11:27PM +0100, Pedro Alves wrote:
>> On 2022-10-12 1:38 p.m., Andrew Burgess via Gdb-patches wrote:
>> > When building the erc32 simulator I get a few warnings like this:
>> > 
>> >   /tmp/build/sim/../../src/sim/erc32/exec.c:1377:21: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
>> >    1377 |   sregs->fs[rd] = *((float32 *) & ddata[0]);
>> >         |                    ~^~~~~~~~~~~~~~~~~~~~~~~
>> > 
>> > The type of '& ddata[0]' will be 'uint32_t *', which is what triggers
>> > the warning.
>> > 
>> > This commit uses an intermediate pointer of type 'char *' when
>> > performing the type-punning, which is well-defined behaviour, and will
>> > silence the above warning.
>> 
>> I'm afraid that's not correct.  That's still undefined behavior, it's just silencing
>> the warning.  The end result is still aliasing float32 and uint32_t objects, and risks
>> generating bogus code.  The char escape hatch only works if you access the object
>> representation via a character type.  Here, the patch is still accessing the object
>> representation of a uint32_t object via a floa32 type.
>> 
>> Here's an old article explaining strict aliasing (just one that I found via a quick google):
>> 
>>   https://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html
>> 
>> This scenario is the "CASTING TO CHAR*" one in that article.
>> 
>> > @@ -1345,7 +1345,8 @@ dispatch_instruction(struct pstate *sregs)
>> >  	    if (mexc) {
>> >  		sregs->trap = TRAP_DEXC;
>> >  	    } else {
>> > -		sregs->fs[rd] = *((float32 *) & data);
>> > +		char *ptr = (char *) &data;
>> > +		sregs->fs[rd] = *((float32 *) ptr);
>> 
>> The best IMHO is to do the type punning via a union, like e.g.:
>> 
>>   union { float32 f; uint32_t i; } u;
>>   u.i = data;
>>   sregs->fs[rd] = u.f;
>> 
>> See here in the C11 standard:
>> 
>>  https://port70.net/~nsz/c/c11/n1570.html#note95
>> 
>> and also the documentation of -fstrict-aliasing at:
>> 
>>   https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
>> 
>
> Hi,
>
> Another well defined (at least to my knowledge) solution to this problem
> is memcpy.  You could do something like:
>
>   memcpy (&sregt->fs[rd], ddata, sizeof (float32));
>
> I tend to find this more straightforward than the type punning version,
> but I would be happy with either.
>

Pedro, Lancelot, thanks for taking the time to give really useful
feedback.

In the end I went with the memcpy approach.  I ran a few tests with GCC,
Clang, and ICC, and in each case the code generated at -O0 was either
identical, or pretty much identical when using memcpy vs using a union.
When switching to -O2 the code was identical in all cases I checked.

Thoughts?

Thanks,
Andrew

---

commit d04acbda1f2a191193772fc9416cf5b29f0702ce
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Oct 12 11:45:53 2022 +0100

    sim/erc32: avoid dereferencing type-punned pointer warnings
    
    When building the erc32 simulator I get a few warnings like this:
    
      /tmp/build/sim/../../src/sim/erc32/exec.c:1377:21: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
       1377 |   sregs->fs[rd] = *((float32 *) & ddata[0]);
            |                    ~^~~~~~~~~~~~~~~~~~~~~~~
    
    The type of '& ddata[0]' will be 'uint32_t *', which is what triggers
    the warning.
    
    This commit makes use of memcpy when performing the type-punning,
    which resolves the above warnings.
    
    With this change, I now see no warnings when compiling exec.c, which
    means that the line in Makefile.in that disables -Werror can be
    removed.
    
    There should be no change in behaviour after this commit.

diff --git a/sim/erc32/Makefile.in b/sim/erc32/Makefile.in
index 786ae1dcc7b..41830aab726 100644
--- a/sim/erc32/Makefile.in
+++ b/sim/erc32/Makefile.in
@@ -32,9 +32,6 @@ SIM_EXTRA_CLEAN = clean-sis
 # behaviour of UART interrupt routines ...
 SIM_EXTRA_CFLAGS += -DFAST_UART -I$(srcroot)
 
-# Some modules don't build cleanly yet.
-exec.o: SIM_WERROR_CFLAGS =
-
 ## COMMON_POST_CONFIG_FRAG
 
 # `sis' doesn't need interf.o.
diff --git a/sim/erc32/exec.c b/sim/erc32/exec.c
index ef93692e7a2..26d48c0e46e 100644
--- a/sim/erc32/exec.c
+++ b/sim/erc32/exec.c
@@ -1345,7 +1345,7 @@ dispatch_instruction(struct pstate *sregs)
 	    if (mexc) {
 		sregs->trap = TRAP_DEXC;
 	    } else {
-		sregs->fs[rd] = *((float32 *) & data);
+		memcpy (&sregs->fs[rd], &data, sizeof (sregs->fs[rd]));
 	    }
 	    break;
 	case LDDF:
@@ -1373,11 +1373,12 @@ dispatch_instruction(struct pstate *sregs)
 	    } else {
 		rd &= 0x1E;
 		sregs->flrd = rd;
-		sregs->fs[rd] = *((float32 *) & ddata[0]);
+		memcpy (&sregs->fs[rd], &ddata[0], sizeof (sregs->fs[rd]));
 #ifdef STAT
 		sregs->nload++;	/* Double load counts twice */
 #endif
-		sregs->fs[rd + 1] = *((float32 *) & ddata[1]);
+		memcpy (&sregs->fs[rd + 1], &ddata[1],
+			sizeof (sregs->fs[rd + 1]));
 		sregs->ltime = ebase.simtime + sregs->icnt + FLSTHOLD +
 			       sregs->hold + sregs->fhold;
 	    }

Re: [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings

[Pedro Alves]

Hi,

On 2022-10-13 11:35 a.m., Andrew Burgess wrote:
> Lancelot SIX <lsix@lancelotsix.com> writes:

>>
>> Another well defined (at least to my knowledge) solution to this problem
>> is memcpy.  You could do something like:
>>
>>   memcpy (&sregt->fs[rd], ddata, sizeof (float32));
>>
>> I tend to find this more straightforward than the type punning version,
>> but I would be happy with either.

Yes, memcpy is fine too.

> Pedro, Lancelot, thanks for taking the time to give really useful
> feedback.
> 
> In the end I went with the memcpy approach.  I ran a few tests with GCC,
> Clang, and ICC, and in each case the code generated at -O0 was either
> identical, or pretty much identical when using memcpy vs using a union.
> When switching to -O2 the code was identical in all cases I checked.
> 
> Thoughts?

LGTM.

Pedro Alves

Re: [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings

[Mike Frysinger]

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

On 12 Oct 2022 13:38, Andrew Burgess via Gdb-patches wrote:
> When building the erc32 simulator I get a few warnings like this:
> 
>   /tmp/build/sim/../../src/sim/erc32/exec.c:1377:21: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
>    1377 |   sregs->fs[rd] = *((float32 *) & ddata[0]);
>         |                    ~^~~~~~~~~~~~~~~~~~~~~~~
> 
> The type of '& ddata[0]' will be 'uint32_t *', which is what triggers
> the warning.
> 
> This commit uses an intermediate pointer of type 'char *' when
> performing the type-punning, which is well-defined behaviour, and will
> silence the above warning.

this is kind of a poor fix.  if we can't arrange for the pointers to be
the right type, at least a memcpy would be better.
-mike

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

Re: [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings

[Andrew Burgess]

Mike Frysinger <vapier@gentoo.org> writes:

> On 12 Oct 2022 13:38, Andrew Burgess via Gdb-patches wrote:
>> When building the erc32 simulator I get a few warnings like this:
>> 
>>   /tmp/build/sim/../../src/sim/erc32/exec.c:1377:21: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
>>    1377 |   sregs->fs[rd] = *((float32 *) & ddata[0]);
>>         |                    ~^~~~~~~~~~~~~~~~~~~~~~~
>> 
>> The type of '& ddata[0]' will be 'uint32_t *', which is what triggers
>> the warning.
>> 
>> This commit uses an intermediate pointer of type 'char *' when
>> performing the type-punning, which is well-defined behaviour, and will
>> silence the above warning.
>
> this is kind of a poor fix.  if we can't arrange for the pointers to be
> the right type, at least a memcpy would be better.

This was already discussed in this thread:

  https://sourceware.org/pipermail/gdb-patches/2022-October/192604.html

The version that got pushed:

  https://sourceware.org/pipermail/gdb-patches/2022-October/192647.html

does make use of memcpy.

Thanks,
Andrew

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

[Andrew Burgess]

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(-)

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;
 	    
-- 
2.25.4

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

[Mike Frysinger]

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

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

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

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

[Andrew Burgess]

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

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

[Mike Frysinger]

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

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

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

Re: [PATCH 0/5] Silence some build warnings in various simulators

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> This series silences some simulator build warnings.

These all seem fine to me, after the updates to patch #4 and the tweak
in patch #2.  Thanks.

Tom

Re: [PATCH 0/5] Silence some build warnings in various simulators

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> This series silences some simulator build warnings.
>
> These all seem fine to me, after the updates to patch #4 and the tweak
> in patch #2.  Thanks.

Thanks, I pushed this series.

Andrew