public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/4] sim/common: Suppress warnings if built with Clang
@ 2022-09-13 12:57 Tsukasa OI
  2022-09-13 12:57 ` [PATCH 1/4] sim: Add ATTRIBUTE_PRINTF Tsukasa OI
                   ` (4 more replies)
  0 siblings, 5 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-09-13 12:57 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Hello,

[Common Background: Building GNU Binutils / GDB with Clang 15.0.0]

I'm now testing to build GNU Binutils / GDB with latest Clang (15.0.0) and
found some errors by default (when Binutils / GDB is not configured with
"--disable-werror").

While the best compiler to build GNU Binutils / GDB is GNU GCC, testing
other compilers are helpful to discover underlying problems and modernize
Binutils / GDB, even if building entire Binutils / GDB with the latest Clang
is unrealistic.  To be sure, I'm not going to finish "porting for Clang".
I will take low-hanging fruits and...

1.  make building with Clang easier and/or
2.  fix code issues (or non-issues) discovered as Clang warnings.

I made four patchsets in which, applying them all makes it possible to
build GNU Binutils / GDB with Clang (without help of --disable-werrors) for
many (but not all) targets including i386 and RISC-V with Ubuntu 22.04 LTS
(x86_64) host.  At least, I think they fix all (at minimum, most of) arch-
independent parts which prevents building with the latest version of Clang.

This is the one of them.


[About this Patchset]

This patchset contains four minor fixes to the files under sim/common/.
Each contains minor fixes to suppress Clang warnings.  Especially, adding
ATTRIBUTE_PRINTF (PATCH 1/4) is important for better static analysis,
even from GNU toolchain side.

For the rest, see commit descriptions for each patch.


Thanks,
Tsukasa




Tsukasa OI (4):
  sim: Add ATTRIBUTE_PRINTF
  sim: Remove self-assignments
  sim: Make WITH_{TRACE,PROFILE}-based macros bool
  sim: Suppress non-literal printf warning

 sim/common/hw-tree.c     |  2 --
 sim/common/sim-cpu.h     |  3 ++-
 sim/common/sim-hw.c      |  3 +++
 sim/common/sim-profile.h | 12 ++++++------
 sim/common/sim-trace.h   |  4 ++--
 5 files changed, 13 insertions(+), 11 deletions(-)


base-commit: 8fa9bc6a030c9a41eb8cf6f0f66043e02005b291
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/4] sim: Add ATTRIBUTE_PRINTF
  2022-09-13 12:57 [PATCH 0/4] sim/common: Suppress warnings if built with Clang Tsukasa OI
@ 2022-09-13 12:57 ` Tsukasa OI
  2022-09-13 12:57 ` [PATCH 2/4] sim: Remove self-assignments Tsukasa OI
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-09-13 12:57 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Clang generates a warning if the format parameter of a printf-like function
is not a literal.  To avoid warnings on printf-like wrapper, it requires
__attribute__((format)) and we have ATTRIBUTE_PRINTF macro.  Because of the
default configuration ("-Werror"), it causes a build failure.

This commit adds ATTRIBUTE_PRINTF for a printf-like function.

sim/ChangeLog:

	* common/sim-cpu.h (sim_io_eprintf_cpu): Add ATTRIBUTE_PRINTF.
---
 sim/common/sim-cpu.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sim/common/sim-cpu.h b/sim/common/sim-cpu.h
index d8b2b9c494a..024eb31d79a 100644
--- a/sim/common/sim-cpu.h
+++ b/sim/common/sim-cpu.h
@@ -137,7 +137,8 @@ extern sim_cpu *sim_cpu_lookup (SIM_DESC, const char *);
 /* Return prefix to use in cpu specific messages.  */
 extern const char *sim_cpu_msg_prefix (sim_cpu *);
 /* Cover fn to sim_io_eprintf.  */
-extern void sim_io_eprintf_cpu (sim_cpu *, const char *, ...);
+extern void sim_io_eprintf_cpu (sim_cpu *, const char *, ...)
+    ATTRIBUTE_PRINTF (2, 3);
 
 /* Get/set a pc value.  */
 #define CPU_PC_GET(cpu) ((* CPU_PC_FETCH (cpu)) (cpu))
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 2/4] sim: Remove self-assignments
  2022-09-13 12:57 [PATCH 0/4] sim/common: Suppress warnings if built with Clang Tsukasa OI
  2022-09-13 12:57 ` [PATCH 1/4] sim: Add ATTRIBUTE_PRINTF Tsukasa OI
@ 2022-09-13 12:57 ` Tsukasa OI
  2022-09-13 12:57 ` [PATCH 3/4] sim: Make WITH_{TRACE,PROFILE}-based macros bool Tsukasa OI
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-09-13 12:57 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Clang generates a warning if a redundant self-assignment is detected.
Because of default configuration ("-Werror"), it causes a build failure on
the default configuration (without "--disable-werror").

This commit removes two redundant self-assignments.

sim/ChangeLog:

	* common/hw-tree.c (split_find_device): Remove redundant
	self-assignments.
---
 sim/common/hw-tree.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/sim/common/hw-tree.c b/sim/common/hw-tree.c
index 56319333d76..8bb5ac77545 100644
--- a/sim/common/hw-tree.c
+++ b/sim/common/hw-tree.c
@@ -335,7 +335,6 @@ split_find_device (struct hw *current,
       else if (strncmp (spec->path, "./", strlen ("./")) == 0)
 	{
 	  /* cd ./... */
-	  current = current;
 	  spec->path += strlen ("./");
 	}
       else if (strncmp (spec->path, "../", strlen ("../")) == 0)
@@ -348,7 +347,6 @@ split_find_device (struct hw *current,
       else if (strcmp (spec->path, ".") == 0)
 	{
 	  /* cd . */
-	  current = current;
 	  spec->path += strlen (".");
 	}
       else if (strcmp (spec->path, "..") == 0)
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 3/4] sim: Make WITH_{TRACE,PROFILE}-based macros bool
  2022-09-13 12:57 [PATCH 0/4] sim/common: Suppress warnings if built with Clang Tsukasa OI
  2022-09-13 12:57 ` [PATCH 1/4] sim: Add ATTRIBUTE_PRINTF Tsukasa OI
  2022-09-13 12:57 ` [PATCH 2/4] sim: Remove self-assignments Tsukasa OI
@ 2022-09-13 12:57 ` Tsukasa OI
  2022-10-05 11:38   ` Andrew Burgess
  2022-09-13 12:57 ` [PATCH 4/4] sim: Suppress non-literal printf warning Tsukasa OI
  2022-09-25  8:42 ` [PATCH v2 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
  4 siblings, 1 reply; 36+ messages in thread
From: Tsukasa OI @ 2022-09-13 12:57 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Clang generates a warning if an ambiguous expression (possibly a bitwise
operation (& or |) but a logical operator (&& or ||) is used) is detected.
Because of the default configuration ("-Werror"), it causes a build failure.

This is caused by predicate macros that use (base_variable & flag).
Clang considers them as regular integer values (not boolean) and
generates that warning.

This commit makes Clang to think those predicate macros to be boolean.

sim/ChangeLog:

	* common/sim-profile.h (WITH_PROFILE_INSN_P, WITH_PROFILE_MEMORY_P,
	WITH_PROFILE_MODEL_P, WITH_PROFILE_SCACHE_P, WITH_PROFILE_PC_P,
	WITH_PROFILE_CORE_P): Make predicate macros boolean.
	* common/sim-trace.h (WITH_TRACE_P, WITH_TRACE_ANY_P): Likewise.
---
 sim/common/sim-profile.h | 12 ++++++------
 sim/common/sim-trace.h   |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/sim/common/sim-profile.h b/sim/common/sim-profile.h
index 61d5039e10d..acf0f7d74e2 100644
--- a/sim/common/sim-profile.h
+++ b/sim/common/sim-profile.h
@@ -87,12 +87,12 @@ SIM_RC sim_profile_set_option (SIM_DESC sd_, const char *name_, int idx_,
 #define PROFILE_core   (1 << PROFILE_CORE_IDX)
 
 /* Preprocessor macros to simplify tests of WITH_PROFILE.  */
-#define WITH_PROFILE_INSN_P (WITH_PROFILE & PROFILE_insn)
-#define WITH_PROFILE_MEMORY_P (WITH_PROFILE & PROFILE_memory)
-#define WITH_PROFILE_MODEL_P (WITH_PROFILE & PROFILE_model)
-#define WITH_PROFILE_SCACHE_P (WITH_PROFILE & PROFILE_scache)
-#define WITH_PROFILE_PC_P (WITH_PROFILE & PROFILE_pc)
-#define WITH_PROFILE_CORE_P (WITH_PROFILE & PROFILE_core)
+#define WITH_PROFILE_INSN_P   (!!(WITH_PROFILE & PROFILE_insn))
+#define WITH_PROFILE_MEMORY_P (!!(WITH_PROFILE & PROFILE_memory))
+#define WITH_PROFILE_MODEL_P  (!!(WITH_PROFILE & PROFILE_model))
+#define WITH_PROFILE_SCACHE_P (!!(WITH_PROFILE & PROFILE_scache))
+#define WITH_PROFILE_PC_P     (!!(WITH_PROFILE & PROFILE_pc))
+#define WITH_PROFILE_CORE_P   (!!(WITH_PROFILE & PROFILE_core))
 
 /* If MAX_TARGET_MODES isn't defined, we can't do memory profiling.
    ??? It is intended that this is a temporary occurrence.  Normally
diff --git a/sim/common/sim-trace.h b/sim/common/sim-trace.h
index d08810d9fcc..bc3e690a4cc 100644
--- a/sim/common/sim-trace.h
+++ b/sim/common/sim-trace.h
@@ -125,10 +125,10 @@ enum {
 #define TRACE_debug    (1 << TRACE_DEBUG_IDX)
 
 /* Return non-zero if tracing of idx is enabled (compiled in).  */
-#define WITH_TRACE_P(idx)	(WITH_TRACE & (1 << idx))
+#define WITH_TRACE_P(idx)	(!!(WITH_TRACE & (1 << idx)))
 
 /* Preprocessor macros to simplify tests of WITH_TRACE.  */
-#define WITH_TRACE_ANY_P	(WITH_TRACE)
+#define WITH_TRACE_ANY_P	(!!(WITH_TRACE))
 #define WITH_TRACE_INSN_P	WITH_TRACE_P (TRACE_INSN_IDX)
 #define WITH_TRACE_DISASM_P	WITH_TRACE_P (TRACE_DISASM_IDX)
 #define WITH_TRACE_DECODE_P	WITH_TRACE_P (TRACE_DECODE_IDX)
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 4/4] sim: Suppress non-literal printf warning
  2022-09-13 12:57 [PATCH 0/4] sim/common: Suppress warnings if built with Clang Tsukasa OI
                   ` (2 preceding siblings ...)
  2022-09-13 12:57 ` [PATCH 3/4] sim: Make WITH_{TRACE,PROFILE}-based macros bool Tsukasa OI
@ 2022-09-13 12:57 ` Tsukasa OI
  2022-10-05 11:45   ` Andrew Burgess
  2022-09-25  8:42 ` [PATCH v2 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
  4 siblings, 1 reply; 36+ messages in thread
From: Tsukasa OI @ 2022-09-13 12:57 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Clang generates a warning if the format parameter of a printf-like function
is not a literal.  However, on hw_vabort, it's unavoidable to use non-
literal as a format string (unless we make huge redesign).

We have "include/diagnostics.h" to suppress certain warnings only when
necessary.  Because DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL can suppress
warnings when the format parameter of a printf-like function is not a
literal, this commit adds this (only where necessary) to suppress this
error with "-Werror", the default configuration.

sim/ChangeLog:

	* common/sim-hw.c (hw_vabort): Suppress non-literal printf warning
	by using DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL.
---
 sim/common/sim-hw.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c
index cece5638bc9..36f355d2262 100644
--- a/sim/common/sim-hw.c
+++ b/sim/common/sim-hw.c
@@ -425,10 +425,13 @@ hw_vabort (struct hw *me,
   strcat (msg, ": ");
   strcat (msg, fmt);
   /* report the problem */
+  DIAGNOSTIC_PUSH
+  DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
   sim_engine_vabort (hw_system (me),
 		     STATE_HW (hw_system (me))->cpu,
 		     STATE_HW (hw_system (me))->cia,
 		     msg, ap);
+  DIAGNOSTIC_POP
 }
 
 void
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v2 0/5] sim: Suppress warnings if built with Clang
  2022-09-13 12:57 [PATCH 0/4] sim/common: Suppress warnings if built with Clang Tsukasa OI
                   ` (3 preceding siblings ...)
  2022-09-13 12:57 ` [PATCH 4/4] sim: Suppress non-literal printf warning Tsukasa OI
@ 2022-09-25  8:42 ` Tsukasa OI
  2022-09-25  8:42   ` [PATCH v2 1/5] sim: Remove self-assignments Tsukasa OI
                     ` (5 more replies)
  4 siblings, 6 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-09-25  8:42 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Hello,

This is the version 2 patchset to suppress Clang compiler warnings
(causes a build failure due to default -Werror).


[Background]

When we build Binutils and GDB with Clang, it causes a build failure due to
warnings generated by Clang and the default -Werror configuration.

I finally managed to make ALL ARCHITECTURE ENABLED Binutils and GDB
-Werror-free on Clang 15.0.0 (note that this does not necessarily mean
warning-free) and this patchset is a part of it (common simulator part
except the printf-like functions).

Full Clang 15.0.0 -Werror-free branch is available at:
<https://github.com/a4lg/binutils-gdb/tree/clang-nowarn-dev>


[About this Patchset]

This patchset contains four five fixes to the files under sim{,/common}.
Each contains minor fixes to suppress Clang / GCC warnings.

Each change is detailed in the each patch.


[Changes: v1 -> v2]

-   Commit messages are improved
-   PATCH v1 1/4 is Moved to another patchset
-   PATCH v1 2-4/4 -> PATCH v2 1-3/5
-   New: Check known getopt definition on sim (PATCH v2 4/5)
    This test is already performed on binutils, gas, gprof and ld and this
    commit does the same to sim.
-   New: Initialize pbb_br_* by default (PATCH v2 5/5)
    It did not generate warnings on Clang but GCC creates some warnings
    (although those are not reported as errors).
    This is due to uninitialized pbb_br_* variables and this commit fixes
    the issue by initializing them by a harmless value.

Thanks,
Tsukasa




Tsukasa OI (5):
  sim: Remove self-assignments
  sim: Make WITH_{TRACE,PROFILE}-based macros bool
  sim: Suppress non-literal printf warning
  sim: Check known getopt definition existence
  sim: Initialize pbb_br_* by default

 sim/common/genmloop.sh   |  8 ++++----
 sim/common/hw-tree.c     |  2 --
 sim/common/sim-hw.c      |  3 +++
 sim/common/sim-profile.h | 12 ++++++------
 sim/common/sim-trace.h   |  4 ++--
 sim/config.h.in          |  3 +++
 sim/configure            | 32 ++++++++++++++++++++++++++++++++
 sim/configure.ac         | 10 ++++++++++
 8 files changed, 60 insertions(+), 14 deletions(-)


base-commit: 58d69206b8173b9d027a6c65f56cdaf045ae6e64
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v2 1/5] sim: Remove self-assignments
  2022-09-25  8:42 ` [PATCH v2 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
@ 2022-09-25  8:42   ` Tsukasa OI
  2022-09-25  8:42   ` [PATCH v2 2/5] sim: Make WITH_{TRACE,PROFILE}-based macros bool Tsukasa OI
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-09-25  8:42 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Clang generates a warning if there is a redundant self-assignment
("-Wself-assign").  On the default configuration, it causes a build failure
(unless "--disable-werror" is specified).

This commit removes two redundant self-assignments.

sim/ChangeLog:

	* common/hw-tree.c (split_find_device): Remove redundant
	self-assignments.
---
 sim/common/hw-tree.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/sim/common/hw-tree.c b/sim/common/hw-tree.c
index 56319333d76..8bb5ac77545 100644
--- a/sim/common/hw-tree.c
+++ b/sim/common/hw-tree.c
@@ -335,7 +335,6 @@ split_find_device (struct hw *current,
       else if (strncmp (spec->path, "./", strlen ("./")) == 0)
 	{
 	  /* cd ./... */
-	  current = current;
 	  spec->path += strlen ("./");
 	}
       else if (strncmp (spec->path, "../", strlen ("../")) == 0)
@@ -348,7 +347,6 @@ split_find_device (struct hw *current,
       else if (strcmp (spec->path, ".") == 0)
 	{
 	  /* cd . */
-	  current = current;
 	  spec->path += strlen (".");
 	}
       else if (strcmp (spec->path, "..") == 0)
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v2 2/5] sim: Make WITH_{TRACE,PROFILE}-based macros bool
  2022-09-25  8:42 ` [PATCH v2 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
  2022-09-25  8:42   ` [PATCH v2 1/5] sim: Remove self-assignments Tsukasa OI
@ 2022-09-25  8:42   ` Tsukasa OI
  2022-09-25  8:42   ` [PATCH v2 3/5] sim: Suppress non-literal printf warning Tsukasa OI
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-09-25  8:42 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Clang generates a warning if there is an ambiguous expression (possibly a
bitwise operation (& or |), but a logical operator (&& or ||) is used;
"-Wconstant-logical-operand").  On the default configuration, it causes a
build failure (unless "--disable-werror" is specified).

This is caused by predicate macros that use the form (base_variable & flag).
Clang considers them as regular integer values (not boolean) and
generates that warning.

This commit makes Clang think those predicate macros to be boolean.

sim/ChangeLog:

	* common/sim-profile.h (WITH_PROFILE_INSN_P,
	WITH_PROFILE_MEMORY_P, WITH_PROFILE_MODEL_P,
	WITH_PROFILE_SCACHE_P, WITH_PROFILE_PC_P, WITH_PROFILE_CORE_P):
	Make predicate macros boolean.
	* common/sim-trace.h (WITH_TRACE_P, WITH_TRACE_ANY_P): Likewise.
---
 sim/common/sim-profile.h | 12 ++++++------
 sim/common/sim-trace.h   |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/sim/common/sim-profile.h b/sim/common/sim-profile.h
index 61d5039e10d..acf0f7d74e2 100644
--- a/sim/common/sim-profile.h
+++ b/sim/common/sim-profile.h
@@ -87,12 +87,12 @@ SIM_RC sim_profile_set_option (SIM_DESC sd_, const char *name_, int idx_,
 #define PROFILE_core   (1 << PROFILE_CORE_IDX)
 
 /* Preprocessor macros to simplify tests of WITH_PROFILE.  */
-#define WITH_PROFILE_INSN_P (WITH_PROFILE & PROFILE_insn)
-#define WITH_PROFILE_MEMORY_P (WITH_PROFILE & PROFILE_memory)
-#define WITH_PROFILE_MODEL_P (WITH_PROFILE & PROFILE_model)
-#define WITH_PROFILE_SCACHE_P (WITH_PROFILE & PROFILE_scache)
-#define WITH_PROFILE_PC_P (WITH_PROFILE & PROFILE_pc)
-#define WITH_PROFILE_CORE_P (WITH_PROFILE & PROFILE_core)
+#define WITH_PROFILE_INSN_P   (!!(WITH_PROFILE & PROFILE_insn))
+#define WITH_PROFILE_MEMORY_P (!!(WITH_PROFILE & PROFILE_memory))
+#define WITH_PROFILE_MODEL_P  (!!(WITH_PROFILE & PROFILE_model))
+#define WITH_PROFILE_SCACHE_P (!!(WITH_PROFILE & PROFILE_scache))
+#define WITH_PROFILE_PC_P     (!!(WITH_PROFILE & PROFILE_pc))
+#define WITH_PROFILE_CORE_P   (!!(WITH_PROFILE & PROFILE_core))
 
 /* If MAX_TARGET_MODES isn't defined, we can't do memory profiling.
    ??? It is intended that this is a temporary occurrence.  Normally
diff --git a/sim/common/sim-trace.h b/sim/common/sim-trace.h
index d08810d9fcc..bc3e690a4cc 100644
--- a/sim/common/sim-trace.h
+++ b/sim/common/sim-trace.h
@@ -125,10 +125,10 @@ enum {
 #define TRACE_debug    (1 << TRACE_DEBUG_IDX)
 
 /* Return non-zero if tracing of idx is enabled (compiled in).  */
-#define WITH_TRACE_P(idx)	(WITH_TRACE & (1 << idx))
+#define WITH_TRACE_P(idx)	(!!(WITH_TRACE & (1 << idx)))
 
 /* Preprocessor macros to simplify tests of WITH_TRACE.  */
-#define WITH_TRACE_ANY_P	(WITH_TRACE)
+#define WITH_TRACE_ANY_P	(!!(WITH_TRACE))
 #define WITH_TRACE_INSN_P	WITH_TRACE_P (TRACE_INSN_IDX)
 #define WITH_TRACE_DISASM_P	WITH_TRACE_P (TRACE_DISASM_IDX)
 #define WITH_TRACE_DECODE_P	WITH_TRACE_P (TRACE_DECODE_IDX)
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v2 3/5] sim: Suppress non-literal printf warning
  2022-09-25  8:42 ` [PATCH v2 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
  2022-09-25  8:42   ` [PATCH v2 1/5] sim: Remove self-assignments Tsukasa OI
  2022-09-25  8:42   ` [PATCH v2 2/5] sim: Make WITH_{TRACE,PROFILE}-based macros bool Tsukasa OI
@ 2022-09-25  8:42   ` Tsukasa OI
  2022-09-25  8:42   ` [PATCH v2 4/5] sim: Check known getopt definition existence Tsukasa OI
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-09-25  8:42 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Clang generates a warning if the format string of a printf-like function is
not a literal ("-Wformat-nonliteral").  On the default configuration, it
causes a build failure (unless "--disable-werror" is specified).

However, non-literal format string is completely safe here.

We have "include/diagnostics.h" to suppress certain warnings only when
necessary.  Because DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL can suppress
warnings when the format string of a printf-like function is not a literal,
this commit adds this macro (only where necessary) to suppress this warning.

sim/ChangeLog:

	* common/sim-hw.c (hw_vabort): Suppress non-literal printf warning
	by using DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL.
---
 sim/common/sim-hw.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c
index cece5638bc9..36f355d2262 100644
--- a/sim/common/sim-hw.c
+++ b/sim/common/sim-hw.c
@@ -425,10 +425,13 @@ hw_vabort (struct hw *me,
   strcat (msg, ": ");
   strcat (msg, fmt);
   /* report the problem */
+  DIAGNOSTIC_PUSH
+  DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
   sim_engine_vabort (hw_system (me),
 		     STATE_HW (hw_system (me))->cpu,
 		     STATE_HW (hw_system (me))->cia,
 		     msg, ap);
+  DIAGNOSTIC_POP
 }
 
 void
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v2 4/5] sim: Check known getopt definition existence
  2022-09-25  8:42 ` [PATCH v2 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
                     ` (2 preceding siblings ...)
  2022-09-25  8:42   ` [PATCH v2 3/5] sim: Suppress non-literal printf warning Tsukasa OI
@ 2022-09-25  8:42   ` Tsukasa OI
  2022-09-25  8:42   ` [PATCH v2 5/5] sim: Initialize pbb_br_* by default Tsukasa OI
  2022-10-06  6:43   ` [PATCH v3 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
  5 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-09-25  8:42 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Clang generates a warning if there is a function declaration/definition
with zero arguments.  Such declarations/definitions without a prototype (an
argument list) are deprecated forms of indefinite arguments
("-Wdeprecated-non-prototype").  On the default configuration, it causes a
build failure (unless "--disable-werror" is specified).

include/getopt.h defines some getopt function definitions but one of them
has a form "extern int getopt ();".  If this form is selected in
include/getopt.h, Clang generates a warning and the build fails by default.

In really old environments, this getopt definition with no arguments is
necessary (because the definition may change between environments).
However, this definition is now a cause of problems on modern environments.

A good news is, this definition is not always selected (e.g. if used by
binutils/*.c).  This is because configuration scripts of binutils, gas,
gprof and ld tries to find known definition of getopt function is used and
defines HAVE_DECL_GETOPT macro.  If this macro is defined when getopt.h is
included, a good form of getopt is used and Clang won't generate warnings.

This commit adds a portion of ld/configure.ac to find the known getopt
definition.  If we could find one (and we *will* in most environments),
we don't need to rely on the deprecated definition.

sim/ChangeLog:

	* configure.ac: Find the known getopt definition in <unistd.h>.
	* configure: Regenerate.
	* config.h.in: Likewise.
---
 sim/config.h.in  |  3 +++
 sim/configure    | 32 ++++++++++++++++++++++++++++++++
 sim/configure.ac | 10 ++++++++++
 3 files changed, 45 insertions(+)

diff --git a/sim/config.h.in b/sim/config.h.in
index 8e84759df04..5091dc32a1b 100644
--- a/sim/config.h.in
+++ b/sim/config.h.in
@@ -41,6 +41,9 @@
 /* Define to 1 if you have the `chmod' function. */
 #undef HAVE_CHMOD
 
+/* Is the prototype for getopt in <unistd.h> in the expected format? */
+#undef HAVE_DECL_GETOPT
+
 /* Define to 1 if you have the declaration of `tzname', and to 0 if you don't.
    */
 #undef HAVE_DECL_TZNAME
diff --git a/sim/configure b/sim/configure
index b31c2f5d8f3..495f1b21ce9 100755
--- a/sim/configure
+++ b/sim/configure
@@ -16298,6 +16298,38 @@ $as_echo "${WARN_CFLAGS} ${WERROR_CFLAGS}" >&6; }
 fi
 
 
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for a known getopt prototype in unistd.h" >&5
+$as_echo_n "checking for a known getopt prototype in unistd.h... " >&6; }
+if ${ld_cv_decl_getopt_unistd_h+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+#include <unistd.h>
+int
+main ()
+{
+extern int getopt (int, char *const*, const char *);
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  ld_cv_decl_getopt_unistd_h=yes
+else
+  ld_cv_decl_getopt_unistd_h=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+fi
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ld_cv_decl_getopt_unistd_h" >&5
+$as_echo "$ld_cv_decl_getopt_unistd_h" >&6; }
+if test $ld_cv_decl_getopt_unistd_h = yes; then
+
+$as_echo "#define HAVE_DECL_GETOPT 1" >>confdefs.h
+
+fi
+
 
 
 
diff --git a/sim/configure.ac b/sim/configure.ac
index 66a1020efe0..c24c676d4da 100644
--- a/sim/configure.ac
+++ b/sim/configure.ac
@@ -177,6 +177,16 @@ SIM_AC_OPTION_STDIO
 SIM_AC_OPTION_TRACE
 SIM_AC_OPTION_WARNINGS
 
+AC_MSG_CHECKING(for a known getopt prototype in unistd.h)
+AC_CACHE_VAL(ld_cv_decl_getopt_unistd_h,
+[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([#include <unistd.h>], [extern int getopt (int, char *const*, const char *);])],
+ld_cv_decl_getopt_unistd_h=yes, ld_cv_decl_getopt_unistd_h=no)])
+AC_MSG_RESULT($ld_cv_decl_getopt_unistd_h)
+if test $ld_cv_decl_getopt_unistd_h = yes; then
+  AC_DEFINE([HAVE_DECL_GETOPT], 1,
+	    [Is the prototype for getopt in <unistd.h> in the expected format?])
+fi
+
 dnl These are unfortunate.  They are conditionally called by other sim macros
 dnl but always used by common/Make-common.in.  So we have to subst here even
 dnl when the rest of the code is in the respective macros.  Once we merge the
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v2 5/5] sim: Initialize pbb_br_* by default
  2022-09-25  8:42 ` [PATCH v2 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
                     ` (3 preceding siblings ...)
  2022-09-25  8:42   ` [PATCH v2 4/5] sim: Check known getopt definition existence Tsukasa OI
@ 2022-09-25  8:42   ` Tsukasa OI
  2022-10-06  6:43   ` [PATCH v3 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
  5 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-09-25  8:42 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

On the files generated by sim/common/genmloop.sh, variables pbb_br_type and
pbb_br_npc are declared uninitialized and passed to other functions in some
cases.  Despite that those are harmless, they will generate GCC warnings
("-Wmaybe-uninitialized").

This commit ensures that pbb_br_type and pbb_br_npc variables are
initialized to a harmless value.

sim/ChangeLog:

	* common/genmloop.sh: Initialize pbb_br_type and pbb_br_npc
	by default.
---
 sim/common/genmloop.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sim/common/genmloop.sh b/sim/common/genmloop.sh
index 1bbeb615b05..5f6456d7159 100755
--- a/sim/common/genmloop.sh
+++ b/sim/common/genmloop.sh
@@ -1167,8 +1167,8 @@ void
   SEM_PC vpc;
 #if WITH_SEM_SWITCH_FULL
   /* For communication between cti's and cti-chain.  */
-  SEM_BRANCH_TYPE pbb_br_type;
-  PCADDR pbb_br_npc;
+  SEM_BRANCH_TYPE pbb_br_type = SEM_BRANCH_UNTAKEN;
+  PCADDR pbb_br_npc = 0;
 #endif
 
 EOF
@@ -1259,8 +1259,8 @@ void
   SEM_PC vpc;
 #if WITH_SEM_SWITCH_FAST
   /* For communication between cti's and cti-chain.  */
-  SEM_BRANCH_TYPE pbb_br_type;
-  PCADDR pbb_br_npc;
+  SEM_BRANCH_TYPE pbb_br_type = SEM_BRANCH_UNTAKEN;
+  PCADDR pbb_br_npc = 0;
 #endif
 
 EOF
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/4] sim: Make WITH_{TRACE,PROFILE}-based macros bool
  2022-09-13 12:57 ` [PATCH 3/4] sim: Make WITH_{TRACE,PROFILE}-based macros bool Tsukasa OI
@ 2022-10-05 11:38   ` Andrew Burgess
  2022-10-06  5:33     ` Tsukasa OI
  0 siblings, 1 reply; 36+ messages in thread
From: Andrew Burgess @ 2022-10-05 11:38 UTC (permalink / raw)
  To: Tsukasa OI, Tsukasa OI, Mike Frysinger; +Cc: gdb-patches

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

> Clang generates a warning if an ambiguous expression (possibly a bitwise
> operation (& or |) but a logical operator (&& or ||) is used) is detected.
> Because of the default configuration ("-Werror"), it causes a build failure.
>
> This is caused by predicate macros that use (base_variable & flag).
> Clang considers them as regular integer values (not boolean) and
> generates that warning.
>
> This commit makes Clang to think those predicate macros to be boolean.
>
> sim/ChangeLog:
>
> 	* common/sim-profile.h (WITH_PROFILE_INSN_P, WITH_PROFILE_MEMORY_P,
> 	WITH_PROFILE_MODEL_P, WITH_PROFILE_SCACHE_P, WITH_PROFILE_PC_P,
> 	WITH_PROFILE_CORE_P): Make predicate macros boolean.
> 	* common/sim-trace.h (WITH_TRACE_P, WITH_TRACE_ANY_P): Likewise.
> ---
>  sim/common/sim-profile.h | 12 ++++++------
>  sim/common/sim-trace.h   |  4 ++--
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/sim/common/sim-profile.h b/sim/common/sim-profile.h
> index 61d5039e10d..acf0f7d74e2 100644
> --- a/sim/common/sim-profile.h
> +++ b/sim/common/sim-profile.h
> @@ -87,12 +87,12 @@ SIM_RC sim_profile_set_option (SIM_DESC sd_, const char *name_, int idx_,
>  #define PROFILE_core   (1 << PROFILE_CORE_IDX)
>  
>  /* Preprocessor macros to simplify tests of WITH_PROFILE.  */
> -#define WITH_PROFILE_INSN_P (WITH_PROFILE & PROFILE_insn)
> -#define WITH_PROFILE_MEMORY_P (WITH_PROFILE & PROFILE_memory)
> -#define WITH_PROFILE_MODEL_P (WITH_PROFILE & PROFILE_model)
> -#define WITH_PROFILE_SCACHE_P (WITH_PROFILE & PROFILE_scache)
> -#define WITH_PROFILE_PC_P (WITH_PROFILE & PROFILE_pc)
> -#define WITH_PROFILE_CORE_P (WITH_PROFILE & PROFILE_core)
> +#define WITH_PROFILE_INSN_P   (!!(WITH_PROFILE & PROFILE_insn))
> +#define WITH_PROFILE_MEMORY_P (!!(WITH_PROFILE & PROFILE_memory))
> +#define WITH_PROFILE_MODEL_P  (!!(WITH_PROFILE & PROFILE_model))
> +#define WITH_PROFILE_SCACHE_P (!!(WITH_PROFILE & PROFILE_scache))
> +#define WITH_PROFILE_PC_P     (!!(WITH_PROFILE & PROFILE_pc))
> +#define WITH_PROFILE_CORE_P   (!!(WITH_PROFILE & PROFILE_core))

Rather than using this '!!' trick, which I'm really not a fan of, could
we just do:

  #define WITH_PROFILE_CORE_P   ((WITH_PROFILE & PROFILE_core) != 0)

I feel this is an accurate representation of the question we're actually
asking.

Thanks,
Andrew


>  
>  /* If MAX_TARGET_MODES isn't defined, we can't do memory profiling.
>     ??? It is intended that this is a temporary occurrence.  Normally
> diff --git a/sim/common/sim-trace.h b/sim/common/sim-trace.h
> index d08810d9fcc..bc3e690a4cc 100644
> --- a/sim/common/sim-trace.h
> +++ b/sim/common/sim-trace.h
> @@ -125,10 +125,10 @@ enum {
>  #define TRACE_debug    (1 << TRACE_DEBUG_IDX)
>  
>  /* Return non-zero if tracing of idx is enabled (compiled in).  */
> -#define WITH_TRACE_P(idx)	(WITH_TRACE & (1 << idx))
> +#define WITH_TRACE_P(idx)	(!!(WITH_TRACE & (1 << idx)))
>  
>  /* Preprocessor macros to simplify tests of WITH_TRACE.  */
> -#define WITH_TRACE_ANY_P	(WITH_TRACE)
> +#define WITH_TRACE_ANY_P	(!!(WITH_TRACE))
>  #define WITH_TRACE_INSN_P	WITH_TRACE_P (TRACE_INSN_IDX)
>  #define WITH_TRACE_DISASM_P	WITH_TRACE_P (TRACE_DISASM_IDX)
>  #define WITH_TRACE_DECODE_P	WITH_TRACE_P (TRACE_DECODE_IDX)
> -- 
> 2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 4/4] sim: Suppress non-literal printf warning
  2022-09-13 12:57 ` [PATCH 4/4] sim: Suppress non-literal printf warning Tsukasa OI
@ 2022-10-05 11:45   ` Andrew Burgess
  2022-10-06  5:39     ` Tsukasa OI
  0 siblings, 1 reply; 36+ messages in thread
From: Andrew Burgess @ 2022-10-05 11:45 UTC (permalink / raw)
  To: Tsukasa OI, Tsukasa OI, Mike Frysinger; +Cc: gdb-patches

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

> Clang generates a warning if the format parameter of a printf-like function
> is not a literal.  However, on hw_vabort, it's unavoidable to use non-
> literal as a format string (unless we make huge redesign).
>
> We have "include/diagnostics.h" to suppress certain warnings only when
> necessary.  Because DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL can suppress
> warnings when the format parameter of a printf-like function is not a
> literal, this commit adds this (only where necessary) to suppress this
> error with "-Werror", the default configuration.
>
> sim/ChangeLog:
>
> 	* common/sim-hw.c (hw_vabort): Suppress non-literal printf warning
> 	by using DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL.
> ---
>  sim/common/sim-hw.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c
> index cece5638bc9..36f355d2262 100644
> --- a/sim/common/sim-hw.c
> +++ b/sim/common/sim-hw.c
> @@ -425,10 +425,13 @@ hw_vabort (struct hw *me,
>    strcat (msg, ": ");
>    strcat (msg, fmt);
>    /* report the problem */
> +  DIAGNOSTIC_PUSH
> +  DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
>    sim_engine_vabort (hw_system (me),
>  		     STATE_HW (hw_system (me))->cpu,
>  		     STATE_HW (hw_system (me))->cia,
>  		     msg, ap);
> +  DIAGNOSTIC_POP

Rather than disabling diagnostics, I'd like to propose the patch below
which expands FMT and AP within sim-hw.c, then passes the expanded
string through to sim_engine_abort.  What do you think of this?

My motivation is to avoid disabling diagnostics as much as possible.

As far as I can tell the host_callback_struct::evprintf_filtered
callback is just the standard printf API, so using vsnprintf should
expand everything correctly.

Thanks,
Andrew

---

diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c
index cece5638bc9..7bfe91e4ae2 100644
--- a/sim/common/sim-hw.c
+++ b/sim/common/sim-hw.c
@@ -408,8 +408,11 @@ hw_vabort (struct hw *me,
 	   const char *fmt,
 	   va_list ap)
 {
+  int len;
   const char *name;
   char *msg;
+  va_list cpy;
+
   /* find an identity */
   if (me != NULL && hw_path (me) != NULL && hw_path (me) [0] != '\0')
     name = hw_path (me);
@@ -419,16 +422,19 @@ hw_vabort (struct hw *me,
     name = hw_family (me);
   else
     name = "device";
-  /* construct an updated format string */
-  msg = alloca (strlen (name) + strlen (": ") + strlen (fmt) + 1);
-  strcpy (msg, name);
-  strcat (msg, ": ");
-  strcat (msg, fmt);
+
+  /* Expand FMT and AP into MSG buffer.  */
+  va_copy (cpy, ap);
+  len = vsnprintf (NULL, 0, fmt, cpy) + 1;
+  va_end (cpy);
+  msg = alloca (len);
+  vsnprintf (msg, len, fmt, ap);
+
   /* report the problem */
-  sim_engine_vabort (hw_system (me),
-		     STATE_HW (hw_system (me))->cpu,
-		     STATE_HW (hw_system (me))->cia,
-		     msg, ap);
+  sim_engine_abort (hw_system (me),
+		    STATE_HW (hw_system (me))->cpu,
+		    STATE_HW (hw_system (me))->cia,
+		    "%s: %s", name, msg);
 }
 
 void


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/4] sim: Make WITH_{TRACE,PROFILE}-based macros bool
  2022-10-05 11:38   ` Andrew Burgess
@ 2022-10-06  5:33     ` Tsukasa OI
  0 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-10-06  5:33 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

On 2022/10/05 20:38, Andrew Burgess wrote:
> Tsukasa OI <research_trasio@irq.a4lg.com> writes:
> 
>> Clang generates a warning if an ambiguous expression (possibly a bitwise
>> operation (& or |) but a logical operator (&& or ||) is used) is detected.
>> Because of the default configuration ("-Werror"), it causes a build failure.
>>
>> This is caused by predicate macros that use (base_variable & flag).
>> Clang considers them as regular integer values (not boolean) and
>> generates that warning.
>>
>> This commit makes Clang to think those predicate macros to be boolean.
>>
>> sim/ChangeLog:
>>
>> 	* common/sim-profile.h (WITH_PROFILE_INSN_P, WITH_PROFILE_MEMORY_P,
>> 	WITH_PROFILE_MODEL_P, WITH_PROFILE_SCACHE_P, WITH_PROFILE_PC_P,
>> 	WITH_PROFILE_CORE_P): Make predicate macros boolean.
>> 	* common/sim-trace.h (WITH_TRACE_P, WITH_TRACE_ANY_P): Likewise.
>> ---
>>  sim/common/sim-profile.h | 12 ++++++------
>>  sim/common/sim-trace.h   |  4 ++--
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/sim/common/sim-profile.h b/sim/common/sim-profile.h
>> index 61d5039e10d..acf0f7d74e2 100644
>> --- a/sim/common/sim-profile.h
>> +++ b/sim/common/sim-profile.h
>> @@ -87,12 +87,12 @@ SIM_RC sim_profile_set_option (SIM_DESC sd_, const char *name_, int idx_,
>>  #define PROFILE_core   (1 << PROFILE_CORE_IDX)
>>  
>>  /* Preprocessor macros to simplify tests of WITH_PROFILE.  */
>> -#define WITH_PROFILE_INSN_P (WITH_PROFILE & PROFILE_insn)
>> -#define WITH_PROFILE_MEMORY_P (WITH_PROFILE & PROFILE_memory)
>> -#define WITH_PROFILE_MODEL_P (WITH_PROFILE & PROFILE_model)
>> -#define WITH_PROFILE_SCACHE_P (WITH_PROFILE & PROFILE_scache)
>> -#define WITH_PROFILE_PC_P (WITH_PROFILE & PROFILE_pc)
>> -#define WITH_PROFILE_CORE_P (WITH_PROFILE & PROFILE_core)
>> +#define WITH_PROFILE_INSN_P   (!!(WITH_PROFILE & PROFILE_insn))
>> +#define WITH_PROFILE_MEMORY_P (!!(WITH_PROFILE & PROFILE_memory))
>> +#define WITH_PROFILE_MODEL_P  (!!(WITH_PROFILE & PROFILE_model))
>> +#define WITH_PROFILE_SCACHE_P (!!(WITH_PROFILE & PROFILE_scache))
>> +#define WITH_PROFILE_PC_P     (!!(WITH_PROFILE & PROFILE_pc))
>> +#define WITH_PROFILE_CORE_P   (!!(WITH_PROFILE & PROFILE_core))
> 
> Rather than using this '!!' trick, which I'm really not a fan of, could
> we just do:
> 
>   #define WITH_PROFILE_CORE_P   ((WITH_PROFILE & PROFILE_core) != 0)
> 
> I feel this is an accurate representation of the question we're actually
> asking.
> 
> Thanks,
> Andrew

That will work.  That's my habit from old time.  I'll fix that and
resubmit the patchset.

Thanks,
Tsukasa

> 
> 
>>  
>>  /* If MAX_TARGET_MODES isn't defined, we can't do memory profiling.
>>     ??? It is intended that this is a temporary occurrence.  Normally
>> diff --git a/sim/common/sim-trace.h b/sim/common/sim-trace.h
>> index d08810d9fcc..bc3e690a4cc 100644
>> --- a/sim/common/sim-trace.h
>> +++ b/sim/common/sim-trace.h
>> @@ -125,10 +125,10 @@ enum {
>>  #define TRACE_debug    (1 << TRACE_DEBUG_IDX)
>>  
>>  /* Return non-zero if tracing of idx is enabled (compiled in).  */
>> -#define WITH_TRACE_P(idx)	(WITH_TRACE & (1 << idx))
>> +#define WITH_TRACE_P(idx)	(!!(WITH_TRACE & (1 << idx)))
>>  
>>  /* Preprocessor macros to simplify tests of WITH_TRACE.  */
>> -#define WITH_TRACE_ANY_P	(WITH_TRACE)
>> +#define WITH_TRACE_ANY_P	(!!(WITH_TRACE))
>>  #define WITH_TRACE_INSN_P	WITH_TRACE_P (TRACE_INSN_IDX)
>>  #define WITH_TRACE_DISASM_P	WITH_TRACE_P (TRACE_DISASM_IDX)
>>  #define WITH_TRACE_DECODE_P	WITH_TRACE_P (TRACE_DECODE_IDX)
>> -- 
>> 2.34.1
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 4/4] sim: Suppress non-literal printf warning
  2022-10-05 11:45   ` Andrew Burgess
@ 2022-10-06  5:39     ` Tsukasa OI
  2022-10-23 12:22       ` Mike Frysinger
  0 siblings, 1 reply; 36+ messages in thread
From: Tsukasa OI @ 2022-10-06  5:39 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

On 2022/10/05 20:45, Andrew Burgess wrote:
> Tsukasa OI <research_trasio@irq.a4lg.com> writes:
> 
>> Clang generates a warning if the format parameter of a printf-like function
>> is not a literal.  However, on hw_vabort, it's unavoidable to use non-
>> literal as a format string (unless we make huge redesign).
>>
>> We have "include/diagnostics.h" to suppress certain warnings only when
>> necessary.  Because DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL can suppress
>> warnings when the format parameter of a printf-like function is not a
>> literal, this commit adds this (only where necessary) to suppress this
>> error with "-Werror", the default configuration.
>>
>> sim/ChangeLog:
>>
>> 	* common/sim-hw.c (hw_vabort): Suppress non-literal printf warning
>> 	by using DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL.
>> ---
>>  sim/common/sim-hw.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c
>> index cece5638bc9..36f355d2262 100644
>> --- a/sim/common/sim-hw.c
>> +++ b/sim/common/sim-hw.c
>> @@ -425,10 +425,13 @@ hw_vabort (struct hw *me,
>>    strcat (msg, ": ");
>>    strcat (msg, fmt);
>>    /* report the problem */
>> +  DIAGNOSTIC_PUSH
>> +  DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
>>    sim_engine_vabort (hw_system (me),
>>  		     STATE_HW (hw_system (me))->cpu,
>>  		     STATE_HW (hw_system (me))->cia,
>>  		     msg, ap);
>> +  DIAGNOSTIC_POP
> 
> Rather than disabling diagnostics, I'd like to propose the patch below
> which expands FMT and AP within sim-hw.c, then passes the expanded
> string through to sim_engine_abort.  What do you think of this?

Ah, It took a while to understand but makes sense to me.

I just needed to add ATTRIBUTE_PRINTF (2, 0) to suppress "-Werror
-Wformat-nonliteral" but I prefer to use your patch instead.

> 
> My motivation is to avoid disabling diagnostics as much as possible.

I support your opinion as possible.  I sometimes disable some warnings
intentionally but it's because I thought disabling the warning is the
only viable solution.  In this case, it wasn't.

Thanks,
Tsukasa

> 
> As far as I can tell the host_callback_struct::evprintf_filtered
> callback is just the standard printf API, so using vsnprintf should
> expand everything correctly.
> 
> Thanks,
> Andrew
> 
> ---
> 
> diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c
> index cece5638bc9..7bfe91e4ae2 100644
> --- a/sim/common/sim-hw.c
> +++ b/sim/common/sim-hw.c
> @@ -408,8 +408,11 @@ hw_vabort (struct hw *me,
>  	   const char *fmt,
>  	   va_list ap)
>  {
> +  int len;
>    const char *name;
>    char *msg;
> +  va_list cpy;
> +
>    /* find an identity */
>    if (me != NULL && hw_path (me) != NULL && hw_path (me) [0] != '\0')
>      name = hw_path (me);
> @@ -419,16 +422,19 @@ hw_vabort (struct hw *me,
>      name = hw_family (me);
>    else
>      name = "device";
> -  /* construct an updated format string */
> -  msg = alloca (strlen (name) + strlen (": ") + strlen (fmt) + 1);
> -  strcpy (msg, name);
> -  strcat (msg, ": ");
> -  strcat (msg, fmt);
> +
> +  /* Expand FMT and AP into MSG buffer.  */
> +  va_copy (cpy, ap);
> +  len = vsnprintf (NULL, 0, fmt, cpy) + 1;
> +  va_end (cpy);
> +  msg = alloca (len);
> +  vsnprintf (msg, len, fmt, ap);
> +
>    /* report the problem */
> -  sim_engine_vabort (hw_system (me),
> -		     STATE_HW (hw_system (me))->cpu,
> -		     STATE_HW (hw_system (me))->cia,
> -		     msg, ap);
> +  sim_engine_abort (hw_system (me),
> +		    STATE_HW (hw_system (me))->cpu,
> +		    STATE_HW (hw_system (me))->cia,
> +		    "%s: %s", name, msg);
>  }
>  
>  void
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v3 0/5] sim: Suppress warnings if built with Clang
  2022-09-25  8:42 ` [PATCH v2 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
                     ` (4 preceding siblings ...)
  2022-09-25  8:42   ` [PATCH v2 5/5] sim: Initialize pbb_br_* by default Tsukasa OI
@ 2022-10-06  6:43   ` Tsukasa OI
  2022-10-06  6:43     ` [PATCH v3 1/5] sim: Remove self-assignments Tsukasa OI
                       ` (6 more replies)
  5 siblings, 7 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-10-06  6:43 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Hello,

This is the version 2 patchset to suppress Clang compiler warnings
(causes a build failure due to default -Werror).


[Background]

When we build Binutils and GDB with Clang, it causes a build failure due to
warnings generated by Clang and the default -Werror configuration.

I MOSTLY managed to make ALL ARCHITECTURE ENABLED Binutils and GDB
-Werror-free on Clang 15.0.0 (note that this does not necessarily mean
warning-free) and this patchset is a part of it (common simulator parts).

-   It still requires -Wno-implicit-function-declaration on the
    LatticeMicro32 and M32R simulators.  Except them, most of Binutils / GDB
    components can be built with Clang by default with my tree.
-   I noticed that the assembler is not tested enough (because just with
    --enable-targets=all, it builds only host assembler).

Full Clang 15.0.0 -Werror-free branch in development is available at:
<https://github.com/a4lg/binutils-gdb/tree/clang-nowarn-dev>

Tested configuration:
-   Ubuntu 22.04.1 LTS (x86_64)
-   LLVM / Clang 15.0.0 (built from source)
-   Configuration examples:
    $srcdir/configure \
        --enable-targets=all \
        --enable-multilib --enable-ld --enable-gold --enable-nls \
        CC=clang CXX=clang++ CCLD=clang CXXLD=clang++ \
        CFLAGS='  -O2 -g -Wno-implicit-function-declaration' \
        CXXFLAGS='-O2 -g -Wno-implicit-function-declaration'
    $srcdir/configure \
        --target=riscv64-unknown-linux-gnu \
        --enable-multilib --enable-ld --enable-gold --enable-nls \
        CC=clang CXX=clang++ CCLD=clang CXXLD=clang++ \
        CFLAGS='-O2 -g' CXXFLAGS='-O2 -g'


[About this Patchset]

This patchset contains four five fixes to the files under sim{,/common}.
Each contains minor fixes to suppress Clang / GCC warnings.

Each change is detailed in the each patch.


[Changes: v2 -> v3]

-   PATCH 2/5: Rewritten predicate macros from (!!x) to (x != 0).
    (This is based on the feedback by Andrew Burgess).
-   PATCH 3/5: Replaced with a patch by Andrew Burgess
    (slightly modified by me).
-   PATCH 4/5: Fix cached variable from ld_cv_* to sim_cv_*.
-   Removed ChangeLog from commit messages
    (based on the feedback [to another patchset by me] by Bruno Larsen).


[Changes: v1 -> v2]

-   Commit messages are improved
-   PATCH v1 1/4 is Moved to another patchset
-   PATCH v1 2-4/4 -> PATCH v2 1-3/5
-   New: Check known getopt definition on sim (PATCH v2 4/5)
    This test is already performed on binutils, gas, gprof and ld and this
    commit does the same to sim.
-   New: Initialize pbb_br_* by default (PATCH v2 5/5)
    It did not generate warnings on Clang but GCC creates some warnings
    (although those are not reported as errors).
    This is due to uninitialized pbb_br_* variables and this commit fixes
    the issue by initializing them by a harmless value.

Thanks,
Tsukasa




Tsukasa OI (5):
  sim: Remove self-assignments
  sim: Make WITH_{TRACE,PROFILE}-based macros bool
  sim: Suppress non-literal printf warning
  sim: Check known getopt definition existence
  sim: Initialize pbb_br_* by default

 sim/common/genmloop.sh   |  8 ++++----
 sim/common/hw-tree.c     |  2 --
 sim/common/sim-hw.c      | 26 ++++++++++++++++----------
 sim/common/sim-profile.h | 12 ++++++------
 sim/common/sim-trace.h   |  4 ++--
 sim/config.h.in          |  3 +++
 sim/configure            | 32 ++++++++++++++++++++++++++++++++
 sim/configure.ac         | 10 ++++++++++
 8 files changed, 73 insertions(+), 24 deletions(-)


base-commit: a13886e2198beb78b81c59839043b021ce6df78a
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v3 1/5] sim: Remove self-assignments
  2022-10-06  6:43   ` [PATCH v3 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
@ 2022-10-06  6:43     ` Tsukasa OI
  2022-10-11 14:21       ` Andrew Burgess
  2022-10-06  6:43     ` [PATCH v3 2/5] sim: Make WITH_{TRACE,PROFILE}-based macros bool Tsukasa OI
                       ` (5 subsequent siblings)
  6 siblings, 1 reply; 36+ messages in thread
From: Tsukasa OI @ 2022-10-06  6:43 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Clang generates a warning if there is a redundant self-assignment
("-Wself-assign").  On the default configuration, it causes a build failure
(unless "--disable-werror" is specified).

This commit removes two redundant self-assignments.
---
 sim/common/hw-tree.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/sim/common/hw-tree.c b/sim/common/hw-tree.c
index 56319333d76..8bb5ac77545 100644
--- a/sim/common/hw-tree.c
+++ b/sim/common/hw-tree.c
@@ -335,7 +335,6 @@ split_find_device (struct hw *current,
       else if (strncmp (spec->path, "./", strlen ("./")) == 0)
 	{
 	  /* cd ./... */
-	  current = current;
 	  spec->path += strlen ("./");
 	}
       else if (strncmp (spec->path, "../", strlen ("../")) == 0)
@@ -348,7 +347,6 @@ split_find_device (struct hw *current,
       else if (strcmp (spec->path, ".") == 0)
 	{
 	  /* cd . */
-	  current = current;
 	  spec->path += strlen (".");
 	}
       else if (strcmp (spec->path, "..") == 0)
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v3 2/5] sim: Make WITH_{TRACE,PROFILE}-based macros bool
  2022-10-06  6:43   ` [PATCH v3 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
  2022-10-06  6:43     ` [PATCH v3 1/5] sim: Remove self-assignments Tsukasa OI
@ 2022-10-06  6:43     ` Tsukasa OI
  2022-10-06  6:43     ` [PATCH v3 3/5] sim: Suppress non-literal printf warning Tsukasa OI
                       ` (4 subsequent siblings)
  6 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-10-06  6:43 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Clang generates a warning if there is an ambiguous expression (possibly a
bitwise operation (& or |), but a logical operator (&& or ||) is used;
"-Wconstant-logical-operand").  On the default configuration, it causes a
build failure (unless "--disable-werror" is specified).

This is caused by predicate macros that use the form (base_variable & flag).
Clang considers them as regular integer values (not boolean) and
generates that warning.

This commit makes Clang think those predicate macros to be boolean.
---
 sim/common/sim-profile.h | 12 ++++++------
 sim/common/sim-trace.h   |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/sim/common/sim-profile.h b/sim/common/sim-profile.h
index 61d5039e10d..d072621a514 100644
--- a/sim/common/sim-profile.h
+++ b/sim/common/sim-profile.h
@@ -87,12 +87,12 @@ SIM_RC sim_profile_set_option (SIM_DESC sd_, const char *name_, int idx_,
 #define PROFILE_core   (1 << PROFILE_CORE_IDX)
 
 /* Preprocessor macros to simplify tests of WITH_PROFILE.  */
-#define WITH_PROFILE_INSN_P (WITH_PROFILE & PROFILE_insn)
-#define WITH_PROFILE_MEMORY_P (WITH_PROFILE & PROFILE_memory)
-#define WITH_PROFILE_MODEL_P (WITH_PROFILE & PROFILE_model)
-#define WITH_PROFILE_SCACHE_P (WITH_PROFILE & PROFILE_scache)
-#define WITH_PROFILE_PC_P (WITH_PROFILE & PROFILE_pc)
-#define WITH_PROFILE_CORE_P (WITH_PROFILE & PROFILE_core)
+#define WITH_PROFILE_INSN_P   ((WITH_PROFILE & PROFILE_insn) != 0)
+#define WITH_PROFILE_MEMORY_P ((WITH_PROFILE & PROFILE_memory) != 0)
+#define WITH_PROFILE_MODEL_P  ((WITH_PROFILE & PROFILE_model) != 0)
+#define WITH_PROFILE_SCACHE_P ((WITH_PROFILE & PROFILE_scache) != 0)
+#define WITH_PROFILE_PC_P     ((WITH_PROFILE & PROFILE_pc) != 0)
+#define WITH_PROFILE_CORE_P   ((WITH_PROFILE & PROFILE_core) != 0)
 
 /* If MAX_TARGET_MODES isn't defined, we can't do memory profiling.
    ??? It is intended that this is a temporary occurrence.  Normally
diff --git a/sim/common/sim-trace.h b/sim/common/sim-trace.h
index d08810d9fcc..8d4ab130815 100644
--- a/sim/common/sim-trace.h
+++ b/sim/common/sim-trace.h
@@ -125,10 +125,10 @@ enum {
 #define TRACE_debug    (1 << TRACE_DEBUG_IDX)
 
 /* Return non-zero if tracing of idx is enabled (compiled in).  */
-#define WITH_TRACE_P(idx)	(WITH_TRACE & (1 << idx))
+#define WITH_TRACE_P(idx)	((WITH_TRACE & (1 << idx)) != 0)
 
 /* Preprocessor macros to simplify tests of WITH_TRACE.  */
-#define WITH_TRACE_ANY_P	(WITH_TRACE)
+#define WITH_TRACE_ANY_P	(WITH_TRACE != 0)
 #define WITH_TRACE_INSN_P	WITH_TRACE_P (TRACE_INSN_IDX)
 #define WITH_TRACE_DISASM_P	WITH_TRACE_P (TRACE_DISASM_IDX)
 #define WITH_TRACE_DECODE_P	WITH_TRACE_P (TRACE_DECODE_IDX)
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v3 3/5] sim: Suppress non-literal printf warning
  2022-10-06  6:43   ` [PATCH v3 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
  2022-10-06  6:43     ` [PATCH v3 1/5] sim: Remove self-assignments Tsukasa OI
  2022-10-06  6:43     ` [PATCH v3 2/5] sim: Make WITH_{TRACE,PROFILE}-based macros bool Tsukasa OI
@ 2022-10-06  6:43     ` Tsukasa OI
  2022-10-11 14:22       ` Andrew Burgess
  2022-10-06  6:43     ` [PATCH v3 4/5] sim: Check known getopt definition existence Tsukasa OI
                       ` (3 subsequent siblings)
  6 siblings, 1 reply; 36+ messages in thread
From: Tsukasa OI @ 2022-10-06  6:43 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Clang generates a warning if the format string of a printf-like function is
not a literal ("-Wformat-nonliteral"). On the default configuration, it
causes a build failure (unless "--disable-werror" is specified).

To avoid this warning, this commit now uses vsnprintf to format error
message and pass the message to sim_engine_abort function with another
printf-style formatting.

This patch is mostly authored by Andrew Burgess and slightly modified by
Tsukasa OI.

Co-authored-by: Andrew Burgess <aburgess@redhat.com>
Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
---
 sim/common/sim-hw.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c
index cece5638bc9..c70770893c9 100644
--- a/sim/common/sim-hw.c
+++ b/sim/common/sim-hw.c
@@ -403,13 +403,16 @@ sim_hw_io_write_buffer (struct sim_state *sd,
 \f
 /* Abort the simulation specifying HW as the reason */
 
-void
+void ATTRIBUTE_PRINTF (2, 0)
 hw_vabort (struct hw *me,
 	   const char *fmt,
 	   va_list ap)
 {
+  int len;
   const char *name;
   char *msg;
+  va_list cpy;
+
   /* find an identity */
   if (me != NULL && hw_path (me) != NULL && hw_path (me) [0] != '\0')
     name = hw_path (me);
@@ -419,16 +422,19 @@ hw_vabort (struct hw *me,
     name = hw_family (me);
   else
     name = "device";
-  /* construct an updated format string */
-  msg = alloca (strlen (name) + strlen (": ") + strlen (fmt) + 1);
-  strcpy (msg, name);
-  strcat (msg, ": ");
-  strcat (msg, fmt);
+
+  /* Expand FMT and AP into MSG buffer.  */
+  va_copy (cpy, ap);
+  len = vsnprintf (NULL, 0, fmt, cpy) + 1;
+  va_end (cpy);
+  msg = alloca (len);
+  vsnprintf (msg, len, fmt, ap);
+
   /* report the problem */
-  sim_engine_vabort (hw_system (me),
-		     STATE_HW (hw_system (me))->cpu,
-		     STATE_HW (hw_system (me))->cia,
-		     msg, ap);
+  sim_engine_abort (hw_system (me),
+		    STATE_HW (hw_system (me))->cpu,
+		    STATE_HW (hw_system (me))->cia,
+		    "%s: %s", name, msg);
 }
 
 void
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v3 4/5] sim: Check known getopt definition existence
  2022-10-06  6:43   ` [PATCH v3 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
                       ` (2 preceding siblings ...)
  2022-10-06  6:43     ` [PATCH v3 3/5] sim: Suppress non-literal printf warning Tsukasa OI
@ 2022-10-06  6:43     ` Tsukasa OI
  2022-10-12 16:28       ` Tom de Vries
  2022-10-23 12:16       ` Mike Frysinger
  2022-10-06  6:43     ` [PATCH v3 5/5] sim: Initialize pbb_br_* by default Tsukasa OI
                       ` (2 subsequent siblings)
  6 siblings, 2 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-10-06  6:43 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

Clang generates a warning if there is a function declaration/definition
with zero arguments.  Such declarations/definitions without a prototype (an
argument list) are deprecated forms of indefinite arguments
("-Wdeprecated-non-prototype").  On the default configuration, it causes a
build failure (unless "--disable-werror" is specified).

include/getopt.h defines some getopt function definitions but one of them
has a form "extern int getopt ();".  If this form is selected in
include/getopt.h, Clang generates a warning and the build fails by default.

In really old environments, this getopt definition with no arguments is
necessary (because the definition may change between environments).
However, this definition is now a cause of problems on modern environments.

A good news is, this definition is not always selected (e.g. if used by
binutils/*.c).  This is because configuration scripts of binutils, gas,
gprof and ld tries to find known definition of getopt function is used and
defines HAVE_DECL_GETOPT macro.  If this macro is defined when getopt.h is
included, a good form of getopt is used and Clang won't generate warnings.

This commit adds a modified portion of ld/configure.ac to find the known
getopt definition.  If we could find one (and we *will* in most modern
environments), we don't need to rely on the deprecated definition.
---
 sim/config.h.in  |  3 +++
 sim/configure    | 32 ++++++++++++++++++++++++++++++++
 sim/configure.ac | 10 ++++++++++
 3 files changed, 45 insertions(+)

diff --git a/sim/config.h.in b/sim/config.h.in
index 84c363c0aec..9a94b289e46 100644
--- a/sim/config.h.in
+++ b/sim/config.h.in
@@ -41,6 +41,9 @@
 /* Define to 1 if you have the `chmod' function. */
 #undef HAVE_CHMOD
 
+/* Is the prototype for getopt in <unistd.h> in the expected format? */
+#undef HAVE_DECL_GETOPT
+
 /* Define to 1 if you have the declaration of `tzname', and to 0 if you don't.
    */
 #undef HAVE_DECL_TZNAME
diff --git a/sim/configure b/sim/configure
index 75d1935df38..dac7f085be1 100755
--- a/sim/configure
+++ b/sim/configure
@@ -16428,6 +16428,38 @@ $as_echo "${WARN_CFLAGS} ${WERROR_CFLAGS}" >&6; }
 fi
 
 
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for a known getopt prototype in unistd.h" >&5
+$as_echo_n "checking for a known getopt prototype in unistd.h... " >&6; }
+if ${sim_cv_decl_getopt_unistd_h+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+#include <unistd.h>
+int
+main ()
+{
+extern int getopt (int, char *const*, const char *);
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  sim_cv_decl_getopt_unistd_h=yes
+else
+  sim_cv_decl_getopt_unistd_h=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+fi
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $sim_cv_decl_getopt_unistd_h" >&5
+$as_echo "$sim_cv_decl_getopt_unistd_h" >&6; }
+if test $sim_cv_decl_getopt_unistd_h = yes; then
+
+$as_echo "#define HAVE_DECL_GETOPT 1" >>confdefs.h
+
+fi
+
 
 
 
diff --git a/sim/configure.ac b/sim/configure.ac
index 66a1020efe0..be0cfdbea32 100644
--- a/sim/configure.ac
+++ b/sim/configure.ac
@@ -177,6 +177,16 @@ SIM_AC_OPTION_STDIO
 SIM_AC_OPTION_TRACE
 SIM_AC_OPTION_WARNINGS
 
+AC_MSG_CHECKING(for a known getopt prototype in unistd.h)
+AC_CACHE_VAL(sim_cv_decl_getopt_unistd_h,
+[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([#include <unistd.h>], [extern int getopt (int, char *const*, const char *);])],
+sim_cv_decl_getopt_unistd_h=yes, sim_cv_decl_getopt_unistd_h=no)])
+AC_MSG_RESULT($sim_cv_decl_getopt_unistd_h)
+if test $sim_cv_decl_getopt_unistd_h = yes; then
+  AC_DEFINE([HAVE_DECL_GETOPT], 1,
+	    [Is the prototype for getopt in <unistd.h> in the expected format?])
+fi
+
 dnl These are unfortunate.  They are conditionally called by other sim macros
 dnl but always used by common/Make-common.in.  So we have to subst here even
 dnl when the rest of the code is in the respective macros.  Once we merge the
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v3 5/5] sim: Initialize pbb_br_* by default
  2022-10-06  6:43   ` [PATCH v3 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
                       ` (3 preceding siblings ...)
  2022-10-06  6:43     ` [PATCH v3 4/5] sim: Check known getopt definition existence Tsukasa OI
@ 2022-10-06  6:43     ` Tsukasa OI
  2022-10-11 14:20     ` [PATCH v3 0/5] sim: Suppress warnings if built with Clang Andrew Burgess
  2022-10-11 16:40     ` Tom de Vries
  6 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-10-06  6:43 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

On the files generated by sim/common/genmloop.sh, variables pbb_br_type and
pbb_br_npc are declared uninitialized and passed to other functions in some
cases.  Despite that those are harmless, they will generate GCC warnings
("-Wmaybe-uninitialized").

This commit ensures that pbb_br_type and pbb_br_npc variables are
initialized to a harmless value.
---
 sim/common/genmloop.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sim/common/genmloop.sh b/sim/common/genmloop.sh
index 1bbeb615b05..5f6456d7159 100755
--- a/sim/common/genmloop.sh
+++ b/sim/common/genmloop.sh
@@ -1167,8 +1167,8 @@ void
   SEM_PC vpc;
 #if WITH_SEM_SWITCH_FULL
   /* For communication between cti's and cti-chain.  */
-  SEM_BRANCH_TYPE pbb_br_type;
-  PCADDR pbb_br_npc;
+  SEM_BRANCH_TYPE pbb_br_type = SEM_BRANCH_UNTAKEN;
+  PCADDR pbb_br_npc = 0;
 #endif
 
 EOF
@@ -1259,8 +1259,8 @@ void
   SEM_PC vpc;
 #if WITH_SEM_SWITCH_FAST
   /* For communication between cti's and cti-chain.  */
-  SEM_BRANCH_TYPE pbb_br_type;
-  PCADDR pbb_br_npc;
+  SEM_BRANCH_TYPE pbb_br_type = SEM_BRANCH_UNTAKEN;
+  PCADDR pbb_br_npc = 0;
 #endif
 
 EOF
-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 0/5] sim: Suppress warnings if built with Clang
  2022-10-06  6:43   ` [PATCH v3 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
                       ` (4 preceding siblings ...)
  2022-10-06  6:43     ` [PATCH v3 5/5] sim: Initialize pbb_br_* by default Tsukasa OI
@ 2022-10-11 14:20     ` Andrew Burgess
  2022-10-11 16:40     ` Tom de Vries
  6 siblings, 0 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-10-11 14:20 UTC (permalink / raw)
  To: Tsukasa OI, Tsukasa OI, Mike Frysinger; +Cc: gdb-patches

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

> Hello,
>
> This is the version 2 patchset to suppress Clang compiler warnings
> (causes a build failure due to default -Werror).
>
>
> [Background]
>
> When we build Binutils and GDB with Clang, it causes a build failure due to
> warnings generated by Clang and the default -Werror configuration.
>
> I MOSTLY managed to make ALL ARCHITECTURE ENABLED Binutils and GDB
> -Werror-free on Clang 15.0.0 (note that this does not necessarily mean
> warning-free) and this patchset is a part of it (common simulator parts).
>
> -   It still requires -Wno-implicit-function-declaration on the
>     LatticeMicro32 and M32R simulators.  Except them, most of Binutils / GDB
>     components can be built with Clang by default with my tree.
> -   I noticed that the assembler is not tested enough (because just with
>     --enable-targets=all, it builds only host assembler).
>
> Full Clang 15.0.0 -Werror-free branch in development is available at:
> <https://github.com/a4lg/binutils-gdb/tree/clang-nowarn-dev>
>
> Tested configuration:
> -   Ubuntu 22.04.1 LTS (x86_64)
> -   LLVM / Clang 15.0.0 (built from source)
> -   Configuration examples:
>     $srcdir/configure \
>         --enable-targets=all \
>         --enable-multilib --enable-ld --enable-gold --enable-nls \
>         CC=clang CXX=clang++ CCLD=clang CXXLD=clang++ \
>         CFLAGS='  -O2 -g -Wno-implicit-function-declaration' \
>         CXXFLAGS='-O2 -g -Wno-implicit-function-declaration'
>     $srcdir/configure \
>         --target=riscv64-unknown-linux-gnu \
>         --enable-multilib --enable-ld --enable-gold --enable-nls \
>         CC=clang CXX=clang++ CCLD=clang CXXLD=clang++ \
>         CFLAGS='-O2 -g' CXXFLAGS='-O2 -g'
>
>
> [About this Patchset]
>
> This patchset contains four five fixes to the files under sim{,/common}.
> Each contains minor fixes to suppress Clang / GCC warnings.
>
> Each change is detailed in the each patch.
>
>
> [Changes: v2 -> v3]
>
> -   PATCH 2/5: Rewritten predicate macros from (!!x) to (x != 0).
>     (This is based on the feedback by Andrew Burgess).
> -   PATCH 3/5: Replaced with a patch by Andrew Burgess
>     (slightly modified by me).
> -   PATCH 4/5: Fix cached variable from ld_cv_* to sim_cv_*.
> -   Removed ChangeLog from commit messages
>     (based on the feedback [to another patchset by me] by Bruno Larsen).
>
>
> [Changes: v1 -> v2]
>
> -   Commit messages are improved
> -   PATCH v1 1/4 is Moved to another patchset
> -   PATCH v1 2-4/4 -> PATCH v2 1-3/5
> -   New: Check known getopt definition on sim (PATCH v2 4/5)
>     This test is already performed on binutils, gas, gprof and ld and this
>     commit does the same to sim.
> -   New: Initialize pbb_br_* by default (PATCH v2 5/5)
>     It did not generate warnings on Clang but GCC creates some warnings
>     (although those are not reported as errors).
>     This is due to uninitialized pbb_br_* variables and this commit fixes
>     the issue by initializing them by a harmless value.

Thanks for the updated patches.  I reviewed these and pushed them with a
couple of minor updates, I'll follow up to each patch with the changes I
made.

Thanks,
Andrew


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 1/5] sim: Remove self-assignments
  2022-10-06  6:43     ` [PATCH v3 1/5] sim: Remove self-assignments Tsukasa OI
@ 2022-10-11 14:21       ` Andrew Burgess
  2022-10-11 14:29         ` Tsukasa OI
  0 siblings, 1 reply; 36+ messages in thread
From: Andrew Burgess @ 2022-10-11 14:21 UTC (permalink / raw)
  To: Tsukasa OI, Tsukasa OI, Mike Frysinger; +Cc: gdb-patches

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

> Clang generates a warning if there is a redundant self-assignment
> ("-Wself-assign").  On the default configuration, it causes a build failure
> (unless "--disable-werror" is specified).
>
> This commit removes two redundant self-assignments.

I pushed this patch, but I found a couple more of these in ppc/tree.c
which I included in this commit.

Thanks,
Andrew

---

commit 5294d882ebb5b10f8a246f2c51ffef05622ef16c
Author: Tsukasa OI <research_trasio@irq.a4lg.com>
Date:   Sun Sep 25 08:42:02 2022 +0000

    sim: Remove self-assignments
    
    Clang generates a warning if there is a redundant self-assignment
    ("-Wself-assign").  On the default configuration, it causes a build failure
    (unless "--disable-werror" is specified).
    
    This commit removes redundant self-assignments from two files.

diff --git a/sim/common/hw-tree.c b/sim/common/hw-tree.c
index 56319333d76..8bb5ac77545 100644
--- a/sim/common/hw-tree.c
+++ b/sim/common/hw-tree.c
@@ -335,7 +335,6 @@ split_find_device (struct hw *current,
       else if (strncmp (spec->path, "./", strlen ("./")) == 0)
 	{
 	  /* cd ./... */
-	  current = current;
 	  spec->path += strlen ("./");
 	}
       else if (strncmp (spec->path, "../", strlen ("../")) == 0)
@@ -348,7 +347,6 @@ split_find_device (struct hw *current,
       else if (strcmp (spec->path, ".") == 0)
 	{
 	  /* cd . */
-	  current = current;
 	  spec->path += strlen (".");
 	}
       else if (strcmp (spec->path, "..") == 0)
diff --git a/sim/ppc/tree.c b/sim/ppc/tree.c
index 6d20665505e..05532bb47ee 100644
--- a/sim/ppc/tree.c
+++ b/sim/ppc/tree.c
@@ -306,7 +306,6 @@ split_find_device(device *current,
     }
     else if (strncmp(spec->path, "./", strlen("./")) == 0) {
       /* cd ./... */
-      current = current;
       spec->path += strlen("./");
     }
     else if (strncmp(spec->path, "../", strlen("../")) == 0) {
@@ -317,7 +316,6 @@ split_find_device(device *current,
     }
     else if (strcmp(spec->path, ".") == 0) {
       /* cd . */
-      current = current;
       spec->path += strlen(".");
     }
     else if (strcmp(spec->path, "..") == 0) {


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 3/5] sim: Suppress non-literal printf warning
  2022-10-06  6:43     ` [PATCH v3 3/5] sim: Suppress non-literal printf warning Tsukasa OI
@ 2022-10-11 14:22       ` Andrew Burgess
  0 siblings, 0 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-10-11 14:22 UTC (permalink / raw)
  To: Tsukasa OI, Tsukasa OI, Mike Frysinger; +Cc: gdb-patches

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

> Clang generates a warning if the format string of a printf-like function is
> not a literal ("-Wformat-nonliteral"). On the default configuration, it
> causes a build failure (unless "--disable-werror" is specified).
>
> To avoid this warning, this commit now uses vsnprintf to format error
> message and pass the message to sim_engine_abort function with another
> printf-style formatting.
>
> This patch is mostly authored by Andrew Burgess and slightly modified by
> Tsukasa OI.
>
> Co-authored-by: Andrew Burgess <aburgess@redhat.com>
> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
> ---
>  sim/common/sim-hw.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c
> index cece5638bc9..c70770893c9 100644
> --- a/sim/common/sim-hw.c
> +++ b/sim/common/sim-hw.c
> @@ -403,13 +403,16 @@ sim_hw_io_write_buffer (struct sim_state *sd,
>  \f
>  /* Abort the simulation specifying HW as the reason */
>  
> -void
> +void ATTRIBUTE_PRINTF (2, 0)
>  hw_vabort (struct hw *me,
>  	   const char *fmt,
>  	   va_list ap)

I've pushed this patch, but I moved the ATTRIBUTE_PRINTF to the function
declaration.

Thanks,
Andrew

---

commit 96894c19ad2b91db76b9b606d48c56ad354b4801
Author: Tsukasa OI <research_trasio@irq.a4lg.com>
Date:   Thu Oct 6 06:43:51 2022 +0000

    sim: Suppress non-literal printf warning
    
    Clang generates a warning if the format string of a printf-like function is
    not a literal ("-Wformat-nonliteral"). On the default configuration, it
    causes a build failure (unless "--disable-werror" is specified).
    
    To avoid this warning, this commit now uses vsnprintf to format error
    message and pass the message to sim_engine_abort function with another
    printf-style formatting.
    
    This patch is mostly authored by Andrew Burgess and slightly modified by
    Tsukasa OI.
    
    Co-authored-by: Andrew Burgess <aburgess@redhat.com>
    Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>

diff --git a/sim/common/hw-device.h b/sim/common/hw-device.h
index 7a36f2f518c..451f88723de 100644
--- a/sim/common/hw-device.h
+++ b/sim/common/hw-device.h
@@ -437,10 +437,8 @@ void hw_abort
  const char *fmt,
  ...) ATTRIBUTE_PRINTF (2, 3) ATTRIBUTE_NORETURN;
 
-void hw_vabort
-(struct hw *me,
- const char *fmt,
- va_list ap) ATTRIBUTE_NORETURN;
+extern void hw_vabort (struct hw *me, const char *fmt, va_list ap)
+  ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (2, 0);
 
 void hw_halt
 (struct hw *me,
diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c
index cece5638bc9..7bfe91e4ae2 100644
--- a/sim/common/sim-hw.c
+++ b/sim/common/sim-hw.c
@@ -408,8 +408,11 @@ hw_vabort (struct hw *me,
 	   const char *fmt,
 	   va_list ap)
 {
+  int len;
   const char *name;
   char *msg;
+  va_list cpy;
+
   /* find an identity */
   if (me != NULL && hw_path (me) != NULL && hw_path (me) [0] != '\0')
     name = hw_path (me);
@@ -419,16 +422,19 @@ hw_vabort (struct hw *me,
     name = hw_family (me);
   else
     name = "device";
-  /* construct an updated format string */
-  msg = alloca (strlen (name) + strlen (": ") + strlen (fmt) + 1);
-  strcpy (msg, name);
-  strcat (msg, ": ");
-  strcat (msg, fmt);
+
+  /* Expand FMT and AP into MSG buffer.  */
+  va_copy (cpy, ap);
+  len = vsnprintf (NULL, 0, fmt, cpy) + 1;
+  va_end (cpy);
+  msg = alloca (len);
+  vsnprintf (msg, len, fmt, ap);
+
   /* report the problem */
-  sim_engine_vabort (hw_system (me),
-		     STATE_HW (hw_system (me))->cpu,
-		     STATE_HW (hw_system (me))->cia,
-		     msg, ap);
+  sim_engine_abort (hw_system (me),
+		    STATE_HW (hw_system (me))->cpu,
+		    STATE_HW (hw_system (me))->cia,
+		    "%s: %s", name, msg);
 }
 
 void


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 1/5] sim: Remove self-assignments
  2022-10-11 14:21       ` Andrew Burgess
@ 2022-10-11 14:29         ` Tsukasa OI
  0 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-10-11 14:29 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

On 2022/10/11 23:21, Andrew Burgess wrote:
> Tsukasa OI <research_trasio@irq.a4lg.com> writes:
> 
>> Clang generates a warning if there is a redundant self-assignment
>> ("-Wself-assign").  On the default configuration, it causes a build failure
>> (unless "--disable-werror" is specified).
>>
>> This commit removes two redundant self-assignments.
> 
> I pushed this patch, but I found a couple more of these in ppc/tree.c
> which I included in this commit.
> 
> Thanks,
> Andrew

My another (upcoming) patchset included exact changes to sim/ppc/tree.c
but thanks for adding it anyway.

Thanks,
Tsukasa

> 
> ---
> 
> commit 5294d882ebb5b10f8a246f2c51ffef05622ef16c
> Author: Tsukasa OI <research_trasio@irq.a4lg.com>
> Date:   Sun Sep 25 08:42:02 2022 +0000
> 
>     sim: Remove self-assignments
>     
>     Clang generates a warning if there is a redundant self-assignment
>     ("-Wself-assign").  On the default configuration, it causes a build failure
>     (unless "--disable-werror" is specified).
>     
>     This commit removes redundant self-assignments from two files.
> 
> diff --git a/sim/common/hw-tree.c b/sim/common/hw-tree.c
> index 56319333d76..8bb5ac77545 100644
> --- a/sim/common/hw-tree.c
> +++ b/sim/common/hw-tree.c
> @@ -335,7 +335,6 @@ split_find_device (struct hw *current,
>        else if (strncmp (spec->path, "./", strlen ("./")) == 0)
>  	{
>  	  /* cd ./... */
> -	  current = current;
>  	  spec->path += strlen ("./");
>  	}
>        else if (strncmp (spec->path, "../", strlen ("../")) == 0)
> @@ -348,7 +347,6 @@ split_find_device (struct hw *current,
>        else if (strcmp (spec->path, ".") == 0)
>  	{
>  	  /* cd . */
> -	  current = current;
>  	  spec->path += strlen (".");
>  	}
>        else if (strcmp (spec->path, "..") == 0)
> diff --git a/sim/ppc/tree.c b/sim/ppc/tree.c
> index 6d20665505e..05532bb47ee 100644
> --- a/sim/ppc/tree.c
> +++ b/sim/ppc/tree.c
> @@ -306,7 +306,6 @@ split_find_device(device *current,
>      }
>      else if (strncmp(spec->path, "./", strlen("./")) == 0) {
>        /* cd ./... */
> -      current = current;
>        spec->path += strlen("./");
>      }
>      else if (strncmp(spec->path, "../", strlen("../")) == 0) {
> @@ -317,7 +316,6 @@ split_find_device(device *current,
>      }
>      else if (strcmp(spec->path, ".") == 0) {
>        /* cd . */
> -      current = current;
>        spec->path += strlen(".");
>      }
>      else if (strcmp(spec->path, "..") == 0) {
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 0/5] sim: Suppress warnings if built with Clang
  2022-10-06  6:43   ` [PATCH v3 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
                       ` (5 preceding siblings ...)
  2022-10-11 14:20     ` [PATCH v3 0/5] sim: Suppress warnings if built with Clang Andrew Burgess
@ 2022-10-11 16:40     ` Tom de Vries
  2022-10-11 18:02       ` Tsukasa OI
  6 siblings, 1 reply; 36+ messages in thread
From: Tom de Vries @ 2022-10-11 16:40 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

On 10/6/22 08:43, Tsukasa OI via Gdb-patches wrote:
> Hello,
> 
> This is the version 2 patchset to suppress Clang compiler warnings
> (causes a build failure due to default -Werror).
> 
> 

Hi,

AFAICT, this causes breakage with the build bots, see f.i. 
https://builder.sourceware.org/buildbot/#/builders/74/builds/1357 :
...
   CC     pk_disklabel.o
../../../binutils-gdb/sim/ppc/corefile.c: In function 
‘core_map_find_mapping’:
../../../binutils-gdb/sim/ppc/corefile.c:295:136: error: format ‘%x’ 
expects argument of type ‘unsigned int’, but argument 4 has type ‘cpu *’ 
{aka ‘struct _cpu *’} [-Werror=format=]
   295 |     error("core_find_mapping() - access to unmaped address, 
attach a default map to handle this - addr=0x%x nr_bytes=0x%x 
processor=0x%x cia=0x%x\n",
       | 
                                                                       ~^
       | 
                                                                        |
       | 
 
unsigned int
   296 |           addr, nr_bytes, processor, cia);
       |                           ~~~~~~~~~ 

       |                           |
       |                           cpu * {aka struct _cpu *}
cc1: all warnings being treated as errors
make[3]: *** [Makefile:144: corefile.o] Error 1
...

Thanks,
- Tom

> [Background]
> 
> When we build Binutils and GDB with Clang, it causes a build failure due to
> warnings generated by Clang and the default -Werror configuration.
> 
> I MOSTLY managed to make ALL ARCHITECTURE ENABLED Binutils and GDB
> -Werror-free on Clang 15.0.0 (note that this does not necessarily mean
> warning-free) and this patchset is a part of it (common simulator parts).
> 
> -   It still requires -Wno-implicit-function-declaration on the
>      LatticeMicro32 and M32R simulators.  Except them, most of Binutils / GDB
>      components can be built with Clang by default with my tree.
> -   I noticed that the assembler is not tested enough (because just with
>      --enable-targets=all, it builds only host assembler).
> 
> Full Clang 15.0.0 -Werror-free branch in development is available at:
> <https://github.com/a4lg/binutils-gdb/tree/clang-nowarn-dev>
> 
> Tested configuration:
> -   Ubuntu 22.04.1 LTS (x86_64)
> -   LLVM / Clang 15.0.0 (built from source)
> -   Configuration examples:
>      $srcdir/configure \
>          --enable-targets=all \
>          --enable-multilib --enable-ld --enable-gold --enable-nls \
>          CC=clang CXX=clang++ CCLD=clang CXXLD=clang++ \
>          CFLAGS='  -O2 -g -Wno-implicit-function-declaration' \
>          CXXFLAGS='-O2 -g -Wno-implicit-function-declaration'
>      $srcdir/configure \
>          --target=riscv64-unknown-linux-gnu \
>          --enable-multilib --enable-ld --enable-gold --enable-nls \
>          CC=clang CXX=clang++ CCLD=clang CXXLD=clang++ \
>          CFLAGS='-O2 -g' CXXFLAGS='-O2 -g'
> 
> 
> [About this Patchset]
> 
> This patchset contains four five fixes to the files under sim{,/common}.
> Each contains minor fixes to suppress Clang / GCC warnings.
> 
> Each change is detailed in the each patch.
> 
> 
> [Changes: v2 -> v3]
> 
> -   PATCH 2/5: Rewritten predicate macros from (!!x) to (x != 0).
>      (This is based on the feedback by Andrew Burgess).
> -   PATCH 3/5: Replaced with a patch by Andrew Burgess
>      (slightly modified by me).
> -   PATCH 4/5: Fix cached variable from ld_cv_* to sim_cv_*.
> -   Removed ChangeLog from commit messages
>      (based on the feedback [to another patchset by me] by Bruno Larsen).
> 
> 
> [Changes: v1 -> v2]
> 
> -   Commit messages are improved
> -   PATCH v1 1/4 is Moved to another patchset
> -   PATCH v1 2-4/4 -> PATCH v2 1-3/5
> -   New: Check known getopt definition on sim (PATCH v2 4/5)
>      This test is already performed on binutils, gas, gprof and ld and this
>      commit does the same to sim.
> -   New: Initialize pbb_br_* by default (PATCH v2 5/5)
>      It did not generate warnings on Clang but GCC creates some warnings
>      (although those are not reported as errors).
>      This is due to uninitialized pbb_br_* variables and this commit fixes
>      the issue by initializing them by a harmless value.
> 
> Thanks,
> Tsukasa
> 
> 
> 
> 
> Tsukasa OI (5):
>    sim: Remove self-assignments
>    sim: Make WITH_{TRACE,PROFILE}-based macros bool
>    sim: Suppress non-literal printf warning
>    sim: Check known getopt definition existence
>    sim: Initialize pbb_br_* by default
> 
>   sim/common/genmloop.sh   |  8 ++++----
>   sim/common/hw-tree.c     |  2 --
>   sim/common/sim-hw.c      | 26 ++++++++++++++++----------
>   sim/common/sim-profile.h | 12 ++++++------
>   sim/common/sim-trace.h   |  4 ++--
>   sim/config.h.in          |  3 +++
>   sim/configure            | 32 ++++++++++++++++++++++++++++++++
>   sim/configure.ac         | 10 ++++++++++
>   8 files changed, 73 insertions(+), 24 deletions(-)
> 
> 
> base-commit: a13886e2198beb78b81c59839043b021ce6df78a

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 0/5] sim: Suppress warnings if built with Clang
  2022-10-11 16:40     ` Tom de Vries
@ 2022-10-11 18:02       ` Tsukasa OI
  0 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-10-11 18:02 UTC (permalink / raw)
  To: Tom de Vries, Andrew Burgess; +Cc: gdb-patches

On 2022/10/12 1:40, Tom de Vries wrote:
> On 10/6/22 08:43, Tsukasa OI via Gdb-patches wrote:
>> Hello,
>>
>> This is the version 2 patchset to suppress Clang compiler warnings
>> (causes a build failure due to default -Werror).
>>
>>
> 
> Hi,
> 
> AFAICT, this causes breakage with the build bots, see f.i.
> https://builder.sourceware.org/buildbot/#/builders/74/builds/1357 :
> ...
>   CC     pk_disklabel.o
> ../../../binutils-gdb/sim/ppc/corefile.c: In function
> ‘core_map_find_mapping’:
> ../../../binutils-gdb/sim/ppc/corefile.c:295:136: error: format ‘%x’
> expects argument of type ‘unsigned int’, but argument 4 has type ‘cpu *’
> {aka ‘struct _cpu *’} [-Werror=format=]
>   295 |     error("core_find_mapping() - access to unmaped address,
> attach a default map to handle this - addr=0x%x nr_bytes=0x%x
> processor=0x%x cia=0x%x\n",
>       |
>                                                                       ~^
>       |
>                                                                        |
>       |
> unsigned int
>   296 |           addr, nr_bytes, processor, cia);
>       |                           ~~~~~~~~~
>       |                           |
>       |                           cpu * {aka struct _cpu *}
> cc1: all warnings being treated as errors
> make[3]: *** [Makefile:144: corefile.o] Error 1
> ...
> 
> Thanks,
> - Tom

OK, I've figured out why.

First of all, ATTRIBUTE_PRINTF should be placed on the declaration
(rather than definition) whenever possible (if there's no declaration,
no need to create one) because it provides better analysis even if a
printf-like function is in the different object file.  This is opposite
from what I initially thought (note that I remembered those two words
"definition" and "declaration" incorrectly and be careful when you read
my previous e-mails and commit messages).

Second, when Andrew added ATTRIBUTE_PRINTF on the declaration, it
revealed existing broken printf-like calls and generated some errors,
even on GCC.  I'm working on the fix.

Thanks,
Tsukasa

> 
>> [Background]
>>
>> When we build Binutils and GDB with Clang, it causes a build failure
>> due to
>> warnings generated by Clang and the default -Werror configuration.
>>
>> I MOSTLY managed to make ALL ARCHITECTURE ENABLED Binutils and GDB
>> -Werror-free on Clang 15.0.0 (note that this does not necessarily mean
>> warning-free) and this patchset is a part of it (common simulator parts).
>>
>> -   It still requires -Wno-implicit-function-declaration on the
>>      LatticeMicro32 and M32R simulators.  Except them, most of
>> Binutils / GDB
>>      components can be built with Clang by default with my tree.
>> -   I noticed that the assembler is not tested enough (because just with
>>      --enable-targets=all, it builds only host assembler).
>>
>> Full Clang 15.0.0 -Werror-free branch in development is available at:
>> <https://github.com/a4lg/binutils-gdb/tree/clang-nowarn-dev>
>>
>> Tested configuration:
>> -   Ubuntu 22.04.1 LTS (x86_64)
>> -   LLVM / Clang 15.0.0 (built from source)
>> -   Configuration examples:
>>      $srcdir/configure \
>>          --enable-targets=all \
>>          --enable-multilib --enable-ld --enable-gold --enable-nls \
>>          CC=clang CXX=clang++ CCLD=clang CXXLD=clang++ \
>>          CFLAGS='  -O2 -g -Wno-implicit-function-declaration' \
>>          CXXFLAGS='-O2 -g -Wno-implicit-function-declaration'
>>      $srcdir/configure \
>>          --target=riscv64-unknown-linux-gnu \
>>          --enable-multilib --enable-ld --enable-gold --enable-nls \
>>          CC=clang CXX=clang++ CCLD=clang CXXLD=clang++ \
>>          CFLAGS='-O2 -g' CXXFLAGS='-O2 -g'
>>
>>
>> [About this Patchset]
>>
>> This patchset contains four five fixes to the files under sim{,/common}.
>> Each contains minor fixes to suppress Clang / GCC warnings.
>>
>> Each change is detailed in the each patch.
>>
>>
>> [Changes: v2 -> v3]
>>
>> -   PATCH 2/5: Rewritten predicate macros from (!!x) to (x != 0).
>>      (This is based on the feedback by Andrew Burgess).
>> -   PATCH 3/5: Replaced with a patch by Andrew Burgess
>>      (slightly modified by me).
>> -   PATCH 4/5: Fix cached variable from ld_cv_* to sim_cv_*.
>> -   Removed ChangeLog from commit messages
>>      (based on the feedback [to another patchset by me] by Bruno Larsen).
>>
>>
>> [Changes: v1 -> v2]
>>
>> -   Commit messages are improved
>> -   PATCH v1 1/4 is Moved to another patchset
>> -   PATCH v1 2-4/4 -> PATCH v2 1-3/5
>> -   New: Check known getopt definition on sim (PATCH v2 4/5)
>>      This test is already performed on binutils, gas, gprof and ld and
>> this
>>      commit does the same to sim.
>> -   New: Initialize pbb_br_* by default (PATCH v2 5/5)
>>      It did not generate warnings on Clang but GCC creates some warnings
>>      (although those are not reported as errors).
>>      This is due to uninitialized pbb_br_* variables and this commit
>> fixes
>>      the issue by initializing them by a harmless value.
>>
>> Thanks,
>> Tsukasa
>>
>>
>>
>>
>> Tsukasa OI (5):
>>    sim: Remove self-assignments
>>    sim: Make WITH_{TRACE,PROFILE}-based macros bool
>>    sim: Suppress non-literal printf warning
>>    sim: Check known getopt definition existence
>>    sim: Initialize pbb_br_* by default
>>
>>   sim/common/genmloop.sh   |  8 ++++----
>>   sim/common/hw-tree.c     |  2 --
>>   sim/common/sim-hw.c      | 26 ++++++++++++++++----------
>>   sim/common/sim-profile.h | 12 ++++++------
>>   sim/common/sim-trace.h   |  4 ++--
>>   sim/config.h.in          |  3 +++
>>   sim/configure            | 32 ++++++++++++++++++++++++++++++++
>>   sim/configure.ac         | 10 ++++++++++
>>   8 files changed, 73 insertions(+), 24 deletions(-)
>>
>>
>> base-commit: a13886e2198beb78b81c59839043b021ce6df78a
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/5] sim: Check known getopt definition existence
  2022-10-06  6:43     ` [PATCH v3 4/5] sim: Check known getopt definition existence Tsukasa OI
@ 2022-10-12 16:28       ` Tom de Vries
  2022-10-12 17:03         ` Tsukasa OI
  2022-10-13  9:50         ` Tsukasa OI
  2022-10-23 12:16       ` Mike Frysinger
  1 sibling, 2 replies; 36+ messages in thread
From: Tom de Vries @ 2022-10-12 16:28 UTC (permalink / raw)
  To: Tsukasa OI, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches

On 10/6/22 08:43, Tsukasa OI via Gdb-patches wrote:
> Clang generates a warning if there is a function declaration/definition
> with zero arguments.  Such declarations/definitions without a prototype (an
> argument list) are deprecated forms of indefinite arguments
> ("-Wdeprecated-non-prototype").  On the default configuration, it causes a
> build failure (unless "--disable-werror" is specified).
> 
> include/getopt.h defines some getopt function definitions but one of them
> has a form "extern int getopt ();".  If this form is selected in
> include/getopt.h, Clang generates a warning and the build fails by default.
> 
> In really old environments, this getopt definition with no arguments is
> necessary (because the definition may change between environments).
> However, this definition is now a cause of problems on modern environments.
> 
> A good news is, this definition is not always selected (e.g. if used by
> binutils/*.c).  This is because configuration scripts of binutils, gas,
> gprof and ld tries to find known definition of getopt function is used and
> defines HAVE_DECL_GETOPT macro.  If this macro is defined when getopt.h is
> included, a good form of getopt is used and Clang won't generate warnings.
> 
> This commit adds a modified portion of ld/configure.ac to find the known
> getopt definition.  If we could find one (and we *will* in most modern
> environments), we don't need to rely on the deprecated definition.

I'm guessing this cause the build breakage on buildbot gdb-centos-x86_64 .

https://builder.sourceware.org/buildbot/#/builders/71/builds/1392

Thanks,
- Tom


> ---
>   sim/config.h.in  |  3 +++
>   sim/configure    | 32 ++++++++++++++++++++++++++++++++
>   sim/configure.ac | 10 ++++++++++
>   3 files changed, 45 insertions(+)
> 
> diff --git a/sim/config.h.in b/sim/config.h.in
> index 84c363c0aec..9a94b289e46 100644
> --- a/sim/config.h.in
> +++ b/sim/config.h.in
> @@ -41,6 +41,9 @@
>   /* Define to 1 if you have the `chmod' function. */
>   #undef HAVE_CHMOD
>   
> +/* Is the prototype for getopt in <unistd.h> in the expected format? */
> +#undef HAVE_DECL_GETOPT
> +
>   /* Define to 1 if you have the declaration of `tzname', and to 0 if you don't.
>      */
>   #undef HAVE_DECL_TZNAME
> diff --git a/sim/configure b/sim/configure
> index 75d1935df38..dac7f085be1 100755
> --- a/sim/configure
> +++ b/sim/configure
> @@ -16428,6 +16428,38 @@ $as_echo "${WARN_CFLAGS} ${WERROR_CFLAGS}" >&6; }
>   fi
>   
>   
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for a known getopt prototype in unistd.h" >&5
> +$as_echo_n "checking for a known getopt prototype in unistd.h... " >&6; }
> +if ${sim_cv_decl_getopt_unistd_h+:} false; then :
> +  $as_echo_n "(cached) " >&6
> +else
> +  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
> +/* end confdefs.h.  */
> +#include <unistd.h>
> +int
> +main ()
> +{
> +extern int getopt (int, char *const*, const char *);
> +  ;
> +  return 0;
> +}
> +_ACEOF
> +if ac_fn_c_try_compile "$LINENO"; then :
> +  sim_cv_decl_getopt_unistd_h=yes
> +else
> +  sim_cv_decl_getopt_unistd_h=no
> +fi
> +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
> +fi
> +
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $sim_cv_decl_getopt_unistd_h" >&5
> +$as_echo "$sim_cv_decl_getopt_unistd_h" >&6; }
> +if test $sim_cv_decl_getopt_unistd_h = yes; then
> +
> +$as_echo "#define HAVE_DECL_GETOPT 1" >>confdefs.h
> +
> +fi
> +
>   
>   
>   
> diff --git a/sim/configure.ac b/sim/configure.ac
> index 66a1020efe0..be0cfdbea32 100644
> --- a/sim/configure.ac
> +++ b/sim/configure.ac
> @@ -177,6 +177,16 @@ SIM_AC_OPTION_STDIO
>   SIM_AC_OPTION_TRACE
>   SIM_AC_OPTION_WARNINGS
>   
> +AC_MSG_CHECKING(for a known getopt prototype in unistd.h)
> +AC_CACHE_VAL(sim_cv_decl_getopt_unistd_h,
> +[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([#include <unistd.h>], [extern int getopt (int, char *const*, const char *);])],
> +sim_cv_decl_getopt_unistd_h=yes, sim_cv_decl_getopt_unistd_h=no)])
> +AC_MSG_RESULT($sim_cv_decl_getopt_unistd_h)
> +if test $sim_cv_decl_getopt_unistd_h = yes; then
> +  AC_DEFINE([HAVE_DECL_GETOPT], 1,
> +	    [Is the prototype for getopt in <unistd.h> in the expected format?])
> +fi
> +
>   dnl These are unfortunate.  They are conditionally called by other sim macros
>   dnl but always used by common/Make-common.in.  So we have to subst here even
>   dnl when the rest of the code is in the respective macros.  Once we merge the

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/5] sim: Check known getopt definition existence
  2022-10-12 16:28       ` Tom de Vries
@ 2022-10-12 17:03         ` Tsukasa OI
  2022-10-12 17:08           ` Tom de Vries
  2022-10-13  9:50         ` Tsukasa OI
  1 sibling, 1 reply; 36+ messages in thread
From: Tsukasa OI @ 2022-10-12 17:03 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gdb-patches

Hi Tom,

On 2022/10/13 1:28, Tom de Vries wrote:
> On 10/6/22 08:43, Tsukasa OI via Gdb-patches wrote:
>> Clang generates a warning if there is a function declaration/definition
>> with zero arguments.  Such declarations/definitions without a
>> prototype (an
>> argument list) are deprecated forms of indefinite arguments
>> ("-Wdeprecated-non-prototype").  On the default configuration, it
>> causes a
>> build failure (unless "--disable-werror" is specified).
>>
>> include/getopt.h defines some getopt function definitions but one of them
>> has a form "extern int getopt ();".  If this form is selected in
>> include/getopt.h, Clang generates a warning and the build fails by
>> default.
>>
>> In really old environments, this getopt definition with no arguments is
>> necessary (because the definition may change between environments).
>> However, this definition is now a cause of problems on modern
>> environments.
>>
>> A good news is, this definition is not always selected (e.g. if used by
>> binutils/*.c).  This is because configuration scripts of binutils, gas,
>> gprof and ld tries to find known definition of getopt function is used
>> and
>> defines HAVE_DECL_GETOPT macro.  If this macro is defined when
>> getopt.h is
>> included, a good form of getopt is used and Clang won't generate
>> warnings.
>>
>> This commit adds a modified portion of ld/configure.ac to find the known
>> getopt definition.  If we could find one (and we *will* in most modern
>> environments), we don't need to rely on the deprecated definition.
> 
> I'm guessing this cause the build breakage on buildbot gdb-centos-x86_64 .
> 
> https://builder.sourceware.org/buildbot/#/builders/71/builds/1392
> 
> Thanks,
> - Tom

I didn't see that error and it seems this error is very strange
considering sim/m32c/main.c is the **only** file which should not be
affected by my change as long as <unistd.h> correctly declares getopt.

Adding
#include <getopt.h>
after
#include <sys/types.h>
might change something but I cannot guarantee whether this change would
fix the issue.

Thanks,
Tsukasa

> 
> 
>> ---
>>   sim/config.h.in  |  3 +++
>>   sim/configure    | 32 ++++++++++++++++++++++++++++++++
>>   sim/configure.ac | 10 ++++++++++
>>   3 files changed, 45 insertions(+)
>>
>> diff --git a/sim/config.h.in b/sim/config.h.in
>> index 84c363c0aec..9a94b289e46 100644
>> --- a/sim/config.h.in
>> +++ b/sim/config.h.in
>> @@ -41,6 +41,9 @@
>>   /* Define to 1 if you have the `chmod' function. */
>>   #undef HAVE_CHMOD
>>   +/* Is the prototype for getopt in <unistd.h> in the expected
>> format? */
>> +#undef HAVE_DECL_GETOPT
>> +
>>   /* Define to 1 if you have the declaration of `tzname', and to 0 if
>> you don't.
>>      */
>>   #undef HAVE_DECL_TZNAME
>> diff --git a/sim/configure b/sim/configure
>> index 75d1935df38..dac7f085be1 100755
>> --- a/sim/configure
>> +++ b/sim/configure
>> @@ -16428,6 +16428,38 @@ $as_echo "${WARN_CFLAGS} ${WERROR_CFLAGS}"
>> >&6; }
>>   fi
>>     +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for a known
>> getopt prototype in unistd.h" >&5
>> +$as_echo_n "checking for a known getopt prototype in unistd.h... "
>> >&6; }
>> +if ${sim_cv_decl_getopt_unistd_h+:} false; then :
>> +  $as_echo_n "(cached) " >&6
>> +else
>> +  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
>> +/* end confdefs.h.  */
>> +#include <unistd.h>
>> +int
>> +main ()
>> +{
>> +extern int getopt (int, char *const*, const char *);
>> +  ;
>> +  return 0;
>> +}
>> +_ACEOF
>> +if ac_fn_c_try_compile "$LINENO"; then :
>> +  sim_cv_decl_getopt_unistd_h=yes
>> +else
>> +  sim_cv_decl_getopt_unistd_h=no
>> +fi
>> +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
>> +fi
>> +
>> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result:
>> $sim_cv_decl_getopt_unistd_h" >&5
>> +$as_echo "$sim_cv_decl_getopt_unistd_h" >&6; }
>> +if test $sim_cv_decl_getopt_unistd_h = yes; then
>> +
>> +$as_echo "#define HAVE_DECL_GETOPT 1" >>confdefs.h
>> +
>> +fi
>> +
>>       diff --git a/sim/configure.ac b/sim/configure.ac
>> index 66a1020efe0..be0cfdbea32 100644
>> --- a/sim/configure.ac
>> +++ b/sim/configure.ac
>> @@ -177,6 +177,16 @@ SIM_AC_OPTION_STDIO
>>   SIM_AC_OPTION_TRACE
>>   SIM_AC_OPTION_WARNINGS
>>   +AC_MSG_CHECKING(for a known getopt prototype in unistd.h)
>> +AC_CACHE_VAL(sim_cv_decl_getopt_unistd_h,
>> +[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([#include <unistd.h>], [extern
>> int getopt (int, char *const*, const char *);])],
>> +sim_cv_decl_getopt_unistd_h=yes, sim_cv_decl_getopt_unistd_h=no)])
>> +AC_MSG_RESULT($sim_cv_decl_getopt_unistd_h)
>> +if test $sim_cv_decl_getopt_unistd_h = yes; then
>> +  AC_DEFINE([HAVE_DECL_GETOPT], 1,
>> +        [Is the prototype for getopt in <unistd.h> in the expected
>> format?])
>> +fi
>> +
>>   dnl These are unfortunate.  They are conditionally called by other
>> sim macros
>>   dnl but always used by common/Make-common.in.  So we have to subst
>> here even
>>   dnl when the rest of the code is in the respective macros.  Once we
>> merge the
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/5] sim: Check known getopt definition existence
  2022-10-12 17:03         ` Tsukasa OI
@ 2022-10-12 17:08           ` Tom de Vries
  2022-10-12 17:20             ` Tom de Vries
  0 siblings, 1 reply; 36+ messages in thread
From: Tom de Vries @ 2022-10-12 17:08 UTC (permalink / raw)
  To: Tsukasa OI; +Cc: gdb-patches

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

On 10/12/22 19:03, Tsukasa OI wrote:
> Adding
> #include <getopt.h>
> after
> #include <sys/types.h>
> might change something but I cannot guarantee whether this change would
> fix the issue.

Ack, I'll try attached patch on the try-buildbot.

Thanks,
- Tom

[-- Attachment #2: 0001-sim-m32c-Add-missing-include-getopt.h-in-main.c.patch --]
[-- Type: text/x-patch, Size: 894 bytes --]

sim/m32c: Add missing include getopt.h in main.c

On buildbot gdb-centos-x86_64 we run into:
...
sim/m32c/main.c: In function ‘main’:
sim/m32c/main.c:143:3: error: implicit declaration of function ‘getopt’ \
  [-Werror=implicit-function-declaration]
   while ((o = getopt (argc, argv, "tc:vdm:C")) != -1)
   ^
cc1: all warnings being treated as errors
make[3]: *** [main.o] Error 1
...

Fix this by adding the missing include of getopt.h.

Build on x86_64-linux.

---
 sim/m32c/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sim/m32c/main.c b/sim/m32c/main.c
index 958ca27ab2b..4ec30c7c7d0 100644
--- a/sim/m32c/main.c
+++ b/sim/m32c/main.c
@@ -29,6 +29,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <setjmp.h>
 #include <signal.h>
 #include <sys/types.h>
+#include <getopt.h>
 
 #ifdef HAVE_SYS_SOCKET_H
 #ifdef HAVE_NETINET_IN_H

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/5] sim: Check known getopt definition existence
  2022-10-12 17:08           ` Tom de Vries
@ 2022-10-12 17:20             ` Tom de Vries
  0 siblings, 0 replies; 36+ messages in thread
From: Tom de Vries @ 2022-10-12 17:20 UTC (permalink / raw)
  To: Tsukasa OI; +Cc: gdb-patches

On 10/12/22 19:08, Tom de Vries wrote:
> On 10/12/22 19:03, Tsukasa OI wrote:
>> Adding
>> #include <getopt.h>
>> after
>> #include <sys/types.h>
>> might change something but I cannot guarantee whether this change would
>> fix the issue.
> 
> Ack, I'll try attached patch on the try-buildbot.
> 

I've tried it out, and it still fails the same:
...
/srv/buildbot/worker/gdb-try-centos-x86_64/gdb-build/sim/../../binutils-gdb/sim/m32c/main.c: 
In function ‘main’:
/srv/buildbot/worker/gdb-try-centos-x86_64/gdb-build/sim/../../binutils-gdb/sim/m32c/main.c:144:3: 
error: implicit declaration of function ‘getopt’ 
[-Werror=implicit-function-declaration]
    while ((o = getopt (argc, argv, "tc:vdm:C")) != -1)
    ^
cc1: all warnings being treated as errors
make[3]: *** [main.o] Error 1
make[3]: Leaving directory 
`/srv/buildbot/worker/gdb-try-centos-x86_64/gdb-build/sim/m32c'
...

Thanks,
- Tom

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/5] sim: Check known getopt definition existence
  2022-10-12 16:28       ` Tom de Vries
  2022-10-12 17:03         ` Tsukasa OI
@ 2022-10-13  9:50         ` Tsukasa OI
  1 sibling, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-10-13  9:50 UTC (permalink / raw)
  To: Tom de Vries; +Cc: Nick Clifton, gdb-patches, Binutils

On 2022/10/13 1:28, Tom de Vries wrote:
> On 10/6/22 08:43, Tsukasa OI via Gdb-patches wrote:
>> Clang generates a warning if there is a function declaration/definition
>> with zero arguments.  Such declarations/definitions without a
>> prototype (an
>> argument list) are deprecated forms of indefinite arguments
>> ("-Wdeprecated-non-prototype").  On the default configuration, it
>> causes a
>> build failure (unless "--disable-werror" is specified).
>>
>> include/getopt.h defines some getopt function definitions but one of them
>> has a form "extern int getopt ();".  If this form is selected in
>> include/getopt.h, Clang generates a warning and the build fails by
>> default.
>>
>> In really old environments, this getopt definition with no arguments is
>> necessary (because the definition may change between environments).
>> However, this definition is now a cause of problems on modern
>> environments.
>>
>> A good news is, this definition is not always selected (e.g. if used by
>> binutils/*.c).  This is because configuration scripts of binutils, gas,
>> gprof and ld tries to find known definition of getopt function is used
>> and
>> defines HAVE_DECL_GETOPT macro.  If this macro is defined when
>> getopt.h is
>> included, a good form of getopt is used and Clang won't generate
>> warnings.
>>
>> This commit adds a modified portion of ld/configure.ac to find the known
>> getopt definition.  If we could find one (and we *will* in most modern
>> environments), we don't need to rely on the deprecated definition.
> 
> I'm guessing this cause the build breakage on buildbot gdb-centos-x86_64 .
> 
> https://builder.sourceware.org/buildbot/#/builders/71/builds/1392
> 
> Thanks,
> - Tom

Hi Tom,

I finally found a cause.  First of all, this is reproduced with
following configurations (examples are selected to minimize time to
reproduce):

-   CentOS 7 (x86_64; with GNU libc 2.17)
-   "make all-sim" with following configurations (for example):
    -   --target=m32c-unknown-elf
    -   --target=rl78-unknown-elf

And it was true that my commit (as you pointed out) was the initial bad
commit but the real cause was much complex than I expected.  The reason
I could not initially reproduce the issue was because it required GNU
libc <= 2.25 to reproduce.

An interesting fact is... standard unistd.h on GNU libc <= 2.25 includes
<getopt.h> (with __need_getopt macro defined) but it actually includes
$(srcdir)/include/getopt.h, not getopt.h in GNU libc.  Since my change
defined HAVE_DECL_GETOPT to 1 on GNU libc-based environment and
declaration of the getopt function is suppressed, causing an error.

On GNU libc >= 2.26, unistd.h includes <bits/getopt_posix.h> without
defining __need_getopt and that's why this error is suppressed on newer
GNU libc-based environments.


Possible reason why this bug is missed is, there are not so many getopt
callers (many calls getopt_long or getopt_long_only, not getopt).

True getopt callers on Binutils/GDB/GCC are following:

-   M32C simulator (affected by my change)
-   RL78 simulator (affected by my change)
-   gprofng (getopt declaration not checked by gprofng/configure)

... yes, GCC (entirely) and the most of Binutils components do not
depend on getopt function anymore.  This fact explains why this bug is
not discovered so long.


The true fix to this is going to be applied to include/getopt.h
(Binutils) to detect this include path on GNU libc <= 2.25.

Aside from this, some sim source files need to be changed (to minimize
problems even further).  I'll submit these patchsets soon.

Thanks,
Tsukasa

> 
> 
>> ---
>>   sim/config.h.in  |  3 +++
>>   sim/configure    | 32 ++++++++++++++++++++++++++++++++
>>   sim/configure.ac | 10 ++++++++++
>>   3 files changed, 45 insertions(+)
>>
>> diff --git a/sim/config.h.in b/sim/config.h.in
>> index 84c363c0aec..9a94b289e46 100644
>> --- a/sim/config.h.in
>> +++ b/sim/config.h.in
>> @@ -41,6 +41,9 @@
>>   /* Define to 1 if you have the `chmod' function. */
>>   #undef HAVE_CHMOD
>>   +/* Is the prototype for getopt in <unistd.h> in the expected
>> format? */
>> +#undef HAVE_DECL_GETOPT
>> +
>>   /* Define to 1 if you have the declaration of `tzname', and to 0 if
>> you don't.
>>      */
>>   #undef HAVE_DECL_TZNAME
>> diff --git a/sim/configure b/sim/configure
>> index 75d1935df38..dac7f085be1 100755
>> --- a/sim/configure
>> +++ b/sim/configure
>> @@ -16428,6 +16428,38 @@ $as_echo "${WARN_CFLAGS} ${WERROR_CFLAGS}"
>> >&6; }
>>   fi
>>     +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for a known
>> getopt prototype in unistd.h" >&5
>> +$as_echo_n "checking for a known getopt prototype in unistd.h... "
>> >&6; }
>> +if ${sim_cv_decl_getopt_unistd_h+:} false; then :
>> +  $as_echo_n "(cached) " >&6
>> +else
>> +  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
>> +/* end confdefs.h.  */
>> +#include <unistd.h>
>> +int
>> +main ()
>> +{
>> +extern int getopt (int, char *const*, const char *);
>> +  ;
>> +  return 0;
>> +}
>> +_ACEOF
>> +if ac_fn_c_try_compile "$LINENO"; then :
>> +  sim_cv_decl_getopt_unistd_h=yes
>> +else
>> +  sim_cv_decl_getopt_unistd_h=no
>> +fi
>> +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
>> +fi
>> +
>> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result:
>> $sim_cv_decl_getopt_unistd_h" >&5
>> +$as_echo "$sim_cv_decl_getopt_unistd_h" >&6; }
>> +if test $sim_cv_decl_getopt_unistd_h = yes; then
>> +
>> +$as_echo "#define HAVE_DECL_GETOPT 1" >>confdefs.h
>> +
>> +fi
>> +
>>       diff --git a/sim/configure.ac b/sim/configure.ac
>> index 66a1020efe0..be0cfdbea32 100644
>> --- a/sim/configure.ac
>> +++ b/sim/configure.ac
>> @@ -177,6 +177,16 @@ SIM_AC_OPTION_STDIO
>>   SIM_AC_OPTION_TRACE
>>   SIM_AC_OPTION_WARNINGS
>>   +AC_MSG_CHECKING(for a known getopt prototype in unistd.h)
>> +AC_CACHE_VAL(sim_cv_decl_getopt_unistd_h,
>> +[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([#include <unistd.h>], [extern
>> int getopt (int, char *const*, const char *);])],
>> +sim_cv_decl_getopt_unistd_h=yes, sim_cv_decl_getopt_unistd_h=no)])
>> +AC_MSG_RESULT($sim_cv_decl_getopt_unistd_h)
>> +if test $sim_cv_decl_getopt_unistd_h = yes; then
>> +  AC_DEFINE([HAVE_DECL_GETOPT], 1,
>> +        [Is the prototype for getopt in <unistd.h> in the expected
>> format?])
>> +fi
>> +
>>   dnl These are unfortunate.  They are conditionally called by other
>> sim macros
>>   dnl but always used by common/Make-common.in.  So we have to subst
>> here even
>>   dnl when the rest of the code is in the respective macros.  Once we
>> merge the
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/5] sim: Check known getopt definition existence
  2022-10-06  6:43     ` [PATCH v3 4/5] sim: Check known getopt definition existence Tsukasa OI
  2022-10-12 16:28       ` Tom de Vries
@ 2022-10-23 12:16       ` Mike Frysinger
  2022-10-27  2:02         ` Tsukasa OI
  1 sibling, 1 reply; 36+ messages in thread
From: Mike Frysinger @ 2022-10-23 12:16 UTC (permalink / raw)
  To: Tsukasa OI; +Cc: Andrew Burgess, gdb-patches

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

On 06 Oct 2022 06:43, Tsukasa OI wrote:
> Clang generates a warning if there is a function declaration/definition
> with zero arguments.  Such declarations/definitions without a prototype (an
> argument list) are deprecated forms of indefinite arguments
> ("-Wdeprecated-non-prototype").  On the default configuration, it causes a
> build failure (unless "--disable-werror" is specified).
> 
> include/getopt.h defines some getopt function definitions but one of them
> has a form "extern int getopt ();".  If this form is selected in
> include/getopt.h, Clang generates a warning and the build fails by default.
> 
> In really old environments, this getopt definition with no arguments is
> necessary (because the definition may change between environments).
> However, this definition is now a cause of problems on modern environments.
> 
> A good news is, this definition is not always selected (e.g. if used by
> binutils/*.c).  This is because configuration scripts of binutils, gas,
> gprof and ld tries to find known definition of getopt function is used and
> defines HAVE_DECL_GETOPT macro.  If this macro is defined when getopt.h is
> included, a good form of getopt is used and Clang won't generate warnings.
> 
> This commit adds a modified portion of ld/configure.ac to find the known
> getopt definition.  If we could find one (and we *will* in most modern
> environments), we don't need to rely on the deprecated definition.
> ---
>  sim/config.h.in  |  3 +++
>  sim/configure    | 32 ++++++++++++++++++++++++++++++++
>  sim/configure.ac | 10 ++++++++++

this logic belongs in m4/sim_ac_platform.m4, not configure.ac

should leave a comment above the code too indicating that this logic is
purely for local getopt.h usage, and is copied from other dirs (e.g. the
binutils files).
-mike

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 4/4] sim: Suppress non-literal printf warning
  2022-10-06  5:39     ` Tsukasa OI
@ 2022-10-23 12:22       ` Mike Frysinger
  2022-10-24 10:50         ` Tsukasa OI
  0 siblings, 1 reply; 36+ messages in thread
From: Mike Frysinger @ 2022-10-23 12:22 UTC (permalink / raw)
  To: Tsukasa OI; +Cc: Andrew Burgess, gdb-patches

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

On 06 Oct 2022 14:39, Tsukasa OI via Gdb-patches wrote:
> On 2022/10/05 20:45, Andrew Burgess wrote:
> > Tsukasa OI <research_trasio@irq.a4lg.com> writes:
> >> Clang generates a warning if the format parameter of a printf-like function
> >> is not a literal.  However, on hw_vabort, it's unavoidable to use non-
> >> literal as a format string (unless we make huge redesign).
> >>
> >> We have "include/diagnostics.h" to suppress certain warnings only when
> >> necessary.  Because DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL can suppress
> >> warnings when the format parameter of a printf-like function is not a
> >> literal, this commit adds this (only where necessary) to suppress this
> >> error with "-Werror", the default configuration.
> >>
> >> sim/ChangeLog:
> >>
> >> 	* common/sim-hw.c (hw_vabort): Suppress non-literal printf warning
> >> 	by using DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL.
> >> ---
> >>  sim/common/sim-hw.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c
> >> index cece5638bc9..36f355d2262 100644
> >> --- a/sim/common/sim-hw.c
> >> +++ b/sim/common/sim-hw.c
> >> @@ -425,10 +425,13 @@ hw_vabort (struct hw *me,
> >>    strcat (msg, ": ");
> >>    strcat (msg, fmt);
> >>    /* report the problem */
> >> +  DIAGNOSTIC_PUSH
> >> +  DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
> >>    sim_engine_vabort (hw_system (me),
> >>  		     STATE_HW (hw_system (me))->cpu,
> >>  		     STATE_HW (hw_system (me))->cia,
> >>  		     msg, ap);
> >> +  DIAGNOSTIC_POP
> > 
> > Rather than disabling diagnostics, I'd like to propose the patch below
> > which expands FMT and AP within sim-hw.c, then passes the expanded
> > string through to sim_engine_abort.  What do you think of this?
> 
> Ah, It took a while to understand but makes sense to me.
> 
> I just needed to add ATTRIBUTE_PRINTF (2, 0) to suppress "-Werror
> -Wformat-nonliteral" but I prefer to use your patch instead.

adding ATTRIBUTE_PRINTF doesn't suppress warnings, it fixes them
-mike

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 4/4] sim: Suppress non-literal printf warning
  2022-10-23 12:22       ` Mike Frysinger
@ 2022-10-24 10:50         ` Tsukasa OI
  0 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-10-24 10:50 UTC (permalink / raw)
  To: Mike Frysinger, gdb-patches

On 2022/10/23 21:22, Mike Frysinger wrote:
> On 06 Oct 2022 14:39, Tsukasa OI via Gdb-patches wrote:
>> On 2022/10/05 20:45, Andrew Burgess wrote:
>>> Tsukasa OI <research_trasio@irq.a4lg.com> writes:
>>>> Clang generates a warning if the format parameter of a printf-like function
>>>> is not a literal.  However, on hw_vabort, it's unavoidable to use non-
>>>> literal as a format string (unless we make huge redesign).
>>>>
>>>> We have "include/diagnostics.h" to suppress certain warnings only when
>>>> necessary.  Because DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL can suppress
>>>> warnings when the format parameter of a printf-like function is not a
>>>> literal, this commit adds this (only where necessary) to suppress this
>>>> error with "-Werror", the default configuration.
>>>>
>>>> sim/ChangeLog:
>>>>
>>>> 	* common/sim-hw.c (hw_vabort): Suppress non-literal printf warning
>>>> 	by using DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL.
>>>> ---
>>>>  sim/common/sim-hw.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c
>>>> index cece5638bc9..36f355d2262 100644
>>>> --- a/sim/common/sim-hw.c
>>>> +++ b/sim/common/sim-hw.c
>>>> @@ -425,10 +425,13 @@ hw_vabort (struct hw *me,
>>>>    strcat (msg, ": ");
>>>>    strcat (msg, fmt);
>>>>    /* report the problem */
>>>> +  DIAGNOSTIC_PUSH
>>>> +  DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
>>>>    sim_engine_vabort (hw_system (me),
>>>>  		     STATE_HW (hw_system (me))->cpu,
>>>>  		     STATE_HW (hw_system (me))->cia,
>>>>  		     msg, ap);
>>>> +  DIAGNOSTIC_POP
>>>
>>> Rather than disabling diagnostics, I'd like to propose the patch below
>>> which expands FMT and AP within sim-hw.c, then passes the expanded
>>> string through to sim_engine_abort.  What do you think of this?
>>
>> Ah, It took a while to understand but makes sense to me.
>>
>> I just needed to add ATTRIBUTE_PRINTF (2, 0) to suppress "-Werror
>> -Wformat-nonliteral" but I prefer to use your patch instead.
> 
> adding ATTRIBUTE_PRINTF doesn't suppress warnings, it fixes them
> -mike

Correct.  I'm focusing on fixing "build failures caused by Clang
warnings" and I think I wrote commit messages without considering why
warnings are gone.

Thanks for pointing this out.
Tsukasa

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v3 4/5] sim: Check known getopt definition existence
  2022-10-23 12:16       ` Mike Frysinger
@ 2022-10-27  2:02         ` Tsukasa OI
  0 siblings, 0 replies; 36+ messages in thread
From: Tsukasa OI @ 2022-10-27  2:02 UTC (permalink / raw)
  To: Mike Frysinger, gdb-patches

On 2022/10/23 21:16, Mike Frysinger wrote:
> On 06 Oct 2022 06:43, Tsukasa OI wrote:
>> Clang generates a warning if there is a function declaration/definition
>> with zero arguments.  Such declarations/definitions without a prototype (an
>> argument list) are deprecated forms of indefinite arguments
>> ("-Wdeprecated-non-prototype").  On the default configuration, it causes a
>> build failure (unless "--disable-werror" is specified).
>>
>> include/getopt.h defines some getopt function definitions but one of them
>> has a form "extern int getopt ();".  If this form is selected in
>> include/getopt.h, Clang generates a warning and the build fails by default.
>>
>> In really old environments, this getopt definition with no arguments is
>> necessary (because the definition may change between environments).
>> However, this definition is now a cause of problems on modern environments.
>>
>> A good news is, this definition is not always selected (e.g. if used by
>> binutils/*.c).  This is because configuration scripts of binutils, gas,
>> gprof and ld tries to find known definition of getopt function is used and
>> defines HAVE_DECL_GETOPT macro.  If this macro is defined when getopt.h is
>> included, a good form of getopt is used and Clang won't generate warnings.
>>
>> This commit adds a modified portion of ld/configure.ac to find the known
>> getopt definition.  If we could find one (and we *will* in most modern
>> environments), we don't need to rely on the deprecated definition.
>> ---
>>  sim/config.h.in  |  3 +++
>>  sim/configure    | 32 ++++++++++++++++++++++++++++++++
>>  sim/configure.ac | 10 ++++++++++
> 
> this logic belongs in m4/sim_ac_platform.m4, not configure.ac
> 
> should leave a comment above the code too indicating that this logic is
> purely for local getopt.h usage, and is copied from other dirs (e.g. the
> binutils files).
> -mike

OK, I'll submit a patch to move this portion to
sim/m4/sim_ac_platform.m4 with proper commit message.

Thanks,
Tsukasa

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2022-10-27  2:02 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-13 12:57 [PATCH 0/4] sim/common: Suppress warnings if built with Clang Tsukasa OI
2022-09-13 12:57 ` [PATCH 1/4] sim: Add ATTRIBUTE_PRINTF Tsukasa OI
2022-09-13 12:57 ` [PATCH 2/4] sim: Remove self-assignments Tsukasa OI
2022-09-13 12:57 ` [PATCH 3/4] sim: Make WITH_{TRACE,PROFILE}-based macros bool Tsukasa OI
2022-10-05 11:38   ` Andrew Burgess
2022-10-06  5:33     ` Tsukasa OI
2022-09-13 12:57 ` [PATCH 4/4] sim: Suppress non-literal printf warning Tsukasa OI
2022-10-05 11:45   ` Andrew Burgess
2022-10-06  5:39     ` Tsukasa OI
2022-10-23 12:22       ` Mike Frysinger
2022-10-24 10:50         ` Tsukasa OI
2022-09-25  8:42 ` [PATCH v2 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
2022-09-25  8:42   ` [PATCH v2 1/5] sim: Remove self-assignments Tsukasa OI
2022-09-25  8:42   ` [PATCH v2 2/5] sim: Make WITH_{TRACE,PROFILE}-based macros bool Tsukasa OI
2022-09-25  8:42   ` [PATCH v2 3/5] sim: Suppress non-literal printf warning Tsukasa OI
2022-09-25  8:42   ` [PATCH v2 4/5] sim: Check known getopt definition existence Tsukasa OI
2022-09-25  8:42   ` [PATCH v2 5/5] sim: Initialize pbb_br_* by default Tsukasa OI
2022-10-06  6:43   ` [PATCH v3 0/5] sim: Suppress warnings if built with Clang Tsukasa OI
2022-10-06  6:43     ` [PATCH v3 1/5] sim: Remove self-assignments Tsukasa OI
2022-10-11 14:21       ` Andrew Burgess
2022-10-11 14:29         ` Tsukasa OI
2022-10-06  6:43     ` [PATCH v3 2/5] sim: Make WITH_{TRACE,PROFILE}-based macros bool Tsukasa OI
2022-10-06  6:43     ` [PATCH v3 3/5] sim: Suppress non-literal printf warning Tsukasa OI
2022-10-11 14:22       ` Andrew Burgess
2022-10-06  6:43     ` [PATCH v3 4/5] sim: Check known getopt definition existence Tsukasa OI
2022-10-12 16:28       ` Tom de Vries
2022-10-12 17:03         ` Tsukasa OI
2022-10-12 17:08           ` Tom de Vries
2022-10-12 17:20             ` Tom de Vries
2022-10-13  9:50         ` Tsukasa OI
2022-10-23 12:16       ` Mike Frysinger
2022-10-27  2:02         ` Tsukasa OI
2022-10-06  6:43     ` [PATCH v3 5/5] sim: Initialize pbb_br_* by default Tsukasa OI
2022-10-11 14:20     ` [PATCH v3 0/5] sim: Suppress warnings if built with Clang Andrew Burgess
2022-10-11 16:40     ` Tom de Vries
2022-10-11 18:02       ` Tsukasa OI

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