* [PATCH 0/5] Silence some build warnings in various simulators @ 2022-10-12 12:38 Andrew Burgess 2022-10-12 12:38 ` [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c Andrew Burgess ` (5 more replies) 0 siblings, 6 replies; 25+ messages in thread From: Andrew Burgess @ 2022-10-12 12:38 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c 2022-10-12 12:38 [PATCH 0/5] Silence some build warnings in various simulators Andrew Burgess @ 2022-10-12 12:38 ` Andrew Burgess 2022-10-23 12:30 ` Mike Frysinger 2022-10-12 12:38 ` [PATCH 2/5] sim/ppc: fix warnings related to printf format strings Andrew Burgess ` (4 subsequent siblings) 5 siblings, 1 reply; 25+ messages in thread From: Andrew Burgess @ 2022-10-12 12:38 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c 2022-10-12 12:38 ` [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c Andrew Burgess @ 2022-10-23 12:30 ` Mike Frysinger 2022-10-24 15:57 ` Andrew Burgess 0 siblings, 1 reply; 25+ messages in thread From: Mike Frysinger @ 2022-10-23 12:30 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches [-- 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 --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c 2022-10-23 12:30 ` Mike Frysinger @ 2022-10-24 15:57 ` Andrew Burgess 2022-10-24 15:59 ` Mike Frysinger 0 siblings, 1 reply; 25+ messages in thread From: Andrew Burgess @ 2022-10-24 15:57 UTC (permalink / raw) To: Mike Frysinger; +Cc: gdb-patches 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); ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c 2022-10-24 15:57 ` Andrew Burgess @ 2022-10-24 15:59 ` Mike Frysinger 2022-10-27 15:53 ` Andrew Burgess 0 siblings, 1 reply; 25+ messages in thread From: Mike Frysinger @ 2022-10-24 15:59 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches [-- 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 --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c 2022-10-24 15:59 ` Mike Frysinger @ 2022-10-27 15:53 ` Andrew Burgess 0 siblings, 0 replies; 25+ messages in thread From: Andrew Burgess @ 2022-10-27 15:53 UTC (permalink / raw) To: Mike Frysinger; +Cc: gdb-patches 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); ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/5] sim/ppc: fix warnings related to printf format strings 2022-10-12 12:38 [PATCH 0/5] Silence some build warnings in various simulators Andrew Burgess 2022-10-12 12:38 ` [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c Andrew Burgess @ 2022-10-12 12:38 ` Andrew Burgess 2022-10-12 12:46 ` Tsukasa OI 2022-10-23 12:20 ` Mike Frysinger 2022-10-12 12:38 ` [PATCH 3/5] sim/ppc: mark device_error function as ATTRIBUTE_NORETURN Andrew Burgess ` (3 subsequent siblings) 5 siblings, 2 replies; 25+ messages in thread From: Andrew Burgess @ 2022-10-12 12:38 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/5] sim/ppc: fix warnings related to printf format strings 2022-10-12 12:38 ` [PATCH 2/5] sim/ppc: fix warnings related to printf format strings Andrew Burgess @ 2022-10-12 12:46 ` Tsukasa OI 2022-10-12 13:50 ` Andrew Burgess 2022-10-23 12:20 ` Mike Frysinger 1 sibling, 1 reply; 25+ messages in thread From: Tsukasa OI @ 2022-10-12 12:46 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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"); > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/5] sim/ppc: fix warnings related to printf format strings 2022-10-12 12:46 ` Tsukasa OI @ 2022-10-12 13:50 ` Andrew Burgess 0 siblings, 0 replies; 25+ messages in thread From: Andrew Burgess @ 2022-10-12 13:50 UTC (permalink / raw) To: Tsukasa OI, gdb-patches 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"); >> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/5] sim/ppc: fix warnings related to printf format strings 2022-10-12 12:38 ` [PATCH 2/5] sim/ppc: fix warnings related to printf format strings Andrew Burgess 2022-10-12 12:46 ` Tsukasa OI @ 2022-10-23 12:20 ` Mike Frysinger 2022-10-24 15:41 ` Andrew Burgess 1 sibling, 1 reply; 25+ messages in thread From: Mike Frysinger @ 2022-10-23 12:20 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches [-- 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 --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/5] sim/ppc: fix warnings related to printf format strings 2022-10-23 12:20 ` Mike Frysinger @ 2022-10-24 15:41 ` Andrew Burgess 0 siblings, 0 replies; 25+ messages in thread From: Andrew Burgess @ 2022-10-24 15:41 UTC (permalink / raw) To: Mike Frysinger; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/5] sim/ppc: mark device_error function as ATTRIBUTE_NORETURN 2022-10-12 12:38 [PATCH 0/5] Silence some build warnings in various simulators Andrew Burgess 2022-10-12 12:38 ` [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c Andrew Burgess 2022-10-12 12:38 ` [PATCH 2/5] sim/ppc: fix warnings related to printf format strings Andrew Burgess @ 2022-10-12 12:38 ` Andrew Burgess 2022-10-12 12:38 ` [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings Andrew Burgess ` (2 subsequent siblings) 5 siblings, 0 replies; 25+ messages in thread From: Andrew Burgess @ 2022-10-12 12:38 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings 2022-10-12 12:38 [PATCH 0/5] Silence some build warnings in various simulators Andrew Burgess ` (2 preceding siblings ...) 2022-10-12 12:38 ` [PATCH 3/5] sim/ppc: mark device_error function as ATTRIBUTE_NORETURN Andrew Burgess @ 2022-10-12 12:38 ` Andrew Burgess 2022-10-12 14:11 ` Pedro Alves 2022-10-23 12:34 ` Mike Frysinger 2022-10-12 12:38 ` [PATCH 5/5] sim/iq2000: silence pointer-sign warnings Andrew Burgess 2022-10-14 17:50 ` [PATCH 0/5] Silence some build warnings in various simulators Tom Tromey 5 siblings, 2 replies; 25+ messages in thread From: Andrew Burgess @ 2022-10-12 12:38 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings 2022-10-12 12:38 ` [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings Andrew Burgess @ 2022-10-12 14:11 ` Pedro Alves 2022-10-12 17:02 ` Lancelot SIX 2022-10-23 12:34 ` Mike Frysinger 1 sibling, 1 reply; 25+ messages in thread From: Pedro Alves @ 2022-10-12 14:11 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings 2022-10-12 14:11 ` Pedro Alves @ 2022-10-12 17:02 ` Lancelot SIX 2022-10-13 10:35 ` Andrew Burgess 0 siblings, 1 reply; 25+ messages in thread From: Lancelot SIX @ 2022-10-12 17:02 UTC (permalink / raw) To: Pedro Alves; +Cc: Andrew Burgess, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings 2022-10-12 17:02 ` Lancelot SIX @ 2022-10-13 10:35 ` Andrew Burgess 2022-10-13 10:49 ` Pedro Alves 0 siblings, 1 reply; 25+ messages in thread From: Andrew Burgess @ 2022-10-13 10:35 UTC (permalink / raw) To: Lancelot SIX, Pedro Alves; +Cc: gdb-patches 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; } ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings 2022-10-13 10:35 ` Andrew Burgess @ 2022-10-13 10:49 ` Pedro Alves 0 siblings, 0 replies; 25+ messages in thread From: Pedro Alves @ 2022-10-13 10:49 UTC (permalink / raw) To: Andrew Burgess, Lancelot SIX; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings 2022-10-12 12:38 ` [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings Andrew Burgess 2022-10-12 14:11 ` Pedro Alves @ 2022-10-23 12:34 ` Mike Frysinger 2022-10-24 15:42 ` Andrew Burgess 1 sibling, 1 reply; 25+ messages in thread From: Mike Frysinger @ 2022-10-23 12:34 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches [-- 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 --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings 2022-10-23 12:34 ` Mike Frysinger @ 2022-10-24 15:42 ` Andrew Burgess 0 siblings, 0 replies; 25+ messages in thread From: Andrew Burgess @ 2022-10-24 15:42 UTC (permalink / raw) To: Mike Frysinger; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 5/5] sim/iq2000: silence pointer-sign warnings 2022-10-12 12:38 [PATCH 0/5] Silence some build warnings in various simulators Andrew Burgess ` (3 preceding siblings ...) 2022-10-12 12:38 ` [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings Andrew Burgess @ 2022-10-12 12:38 ` Andrew Burgess 2022-10-23 12:32 ` Mike Frysinger 2022-10-14 17:50 ` [PATCH 0/5] Silence some build warnings in various simulators Tom Tromey 5 siblings, 1 reply; 25+ messages in thread From: Andrew Burgess @ 2022-10-12 12:38 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/5] sim/iq2000: silence pointer-sign warnings 2022-10-12 12:38 ` [PATCH 5/5] sim/iq2000: silence pointer-sign warnings Andrew Burgess @ 2022-10-23 12:32 ` Mike Frysinger 2022-10-24 15:45 ` Andrew Burgess 0 siblings, 1 reply; 25+ messages in thread From: Mike Frysinger @ 2022-10-23 12:32 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches [-- 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 --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/5] sim/iq2000: silence pointer-sign warnings 2022-10-23 12:32 ` Mike Frysinger @ 2022-10-24 15:45 ` Andrew Burgess 2022-10-26 8:51 ` Mike Frysinger 0 siblings, 1 reply; 25+ messages in thread From: Andrew Burgess @ 2022-10-24 15:45 UTC (permalink / raw) To: Mike Frysinger; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/5] sim/iq2000: silence pointer-sign warnings 2022-10-24 15:45 ` Andrew Burgess @ 2022-10-26 8:51 ` Mike Frysinger 0 siblings, 0 replies; 25+ messages in thread From: Mike Frysinger @ 2022-10-26 8:51 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches [-- 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 --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/5] Silence some build warnings in various simulators 2022-10-12 12:38 [PATCH 0/5] Silence some build warnings in various simulators Andrew Burgess ` (4 preceding siblings ...) 2022-10-12 12:38 ` [PATCH 5/5] sim/iq2000: silence pointer-sign warnings Andrew Burgess @ 2022-10-14 17:50 ` Tom Tromey 2022-10-19 13:34 ` Andrew Burgess 5 siblings, 1 reply; 25+ messages in thread From: Tom Tromey @ 2022-10-14 17:50 UTC (permalink / raw) To: Andrew Burgess via Gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/5] Silence some build warnings in various simulators 2022-10-14 17:50 ` [PATCH 0/5] Silence some build warnings in various simulators Tom Tromey @ 2022-10-19 13:34 ` Andrew Burgess 0 siblings, 0 replies; 25+ messages in thread From: Andrew Burgess @ 2022-10-19 13:34 UTC (permalink / raw) To: Tom Tromey, Andrew Burgess via Gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2022-10-27 15:53 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-12 12:38 [PATCH 0/5] Silence some build warnings in various simulators Andrew Burgess 2022-10-12 12:38 ` [PATCH 1/5] sim/cgen: mask uninitialized variable warning in cgen-run.c Andrew Burgess 2022-10-23 12:30 ` Mike Frysinger 2022-10-24 15:57 ` Andrew Burgess 2022-10-24 15:59 ` Mike Frysinger 2022-10-27 15:53 ` Andrew Burgess 2022-10-12 12:38 ` [PATCH 2/5] sim/ppc: fix warnings related to printf format strings Andrew Burgess 2022-10-12 12:46 ` Tsukasa OI 2022-10-12 13:50 ` Andrew Burgess 2022-10-23 12:20 ` Mike Frysinger 2022-10-24 15:41 ` Andrew Burgess 2022-10-12 12:38 ` [PATCH 3/5] sim/ppc: mark device_error function as ATTRIBUTE_NORETURN Andrew Burgess 2022-10-12 12:38 ` [PATCH 4/5] sim/erc32: avoid dereferencing type-punned pointer warnings Andrew Burgess 2022-10-12 14:11 ` Pedro Alves 2022-10-12 17:02 ` Lancelot SIX 2022-10-13 10:35 ` Andrew Burgess 2022-10-13 10:49 ` Pedro Alves 2022-10-23 12:34 ` Mike Frysinger 2022-10-24 15:42 ` Andrew Burgess 2022-10-12 12:38 ` [PATCH 5/5] sim/iq2000: silence pointer-sign warnings Andrew Burgess 2022-10-23 12:32 ` Mike Frysinger 2022-10-24 15:45 ` Andrew Burgess 2022-10-26 8:51 ` Mike Frysinger 2022-10-14 17:50 ` [PATCH 0/5] Silence some build warnings in various simulators Tom Tromey 2022-10-19 13:34 ` Andrew Burgess
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).