[PATCH] sim: common, microblaze, mn10300: handle signal.h defining REC_PC.

[PATCH] sim: common, microblaze, mn10300: handle signal.h defining REC_PC.

[Mark Wielaard]

signal.h isn't needed in microblaze and mn10300 interp.c
so don't include it.

In common sim-events.c, sim-signal.c, nrun.c and dv-sockser.c we
do need signal.h, but check whether REG_PC is defined (and then
undefine it) before including the sim headers.

It breaks the build on sparc because signal.h indirectly
includes /usr/include/sys/ucontext.h and defines REG_PC,
which is also defined in microblaze-opcm.h
---
 sim/common/dv-sockser.c | 6 ++++++
 sim/common/nrun.c       | 6 ++++++
 sim/common/sim-events.c | 6 ++++++
 sim/common/sim-signal.c | 6 ++++++
 sim/microblaze/interp.c | 1 -
 sim/mn10300/interp.c    | 2 --
 6 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/sim/common/dv-sockser.c b/sim/common/dv-sockser.c
index fba2775f2e8..db6ab8fc4be 100644
--- a/sim/common/dv-sockser.c
+++ b/sim/common/dv-sockser.c
@@ -39,6 +39,12 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <sys/time.h>
 #include <sys/types.h>
 
+/* signal.h might indirectly have defined some REG_ macros
+   which are also defined in the sim headers.  */
+#ifdef REG_PC
+#undef REG_PC
+#endif
+
 #include "sim-main.h"
 #include "sim-assert.h"
 #include "sim-options.h"
diff --git a/sim/common/nrun.c b/sim/common/nrun.c
index 4c011627bb8..6ad2b19fb0d 100644
--- a/sim/common/nrun.c
+++ b/sim/common/nrun.c
@@ -29,6 +29,12 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 #include "bfd.h"
 #include "environ.h"
 
+/* signal.h might indirectly have defined some REG_ macros
+   which are also defined in the sim headers.  */
+#ifdef REG_PC
+#undef REG_PC
+#endif
+
 #include "sim-main.h"
 #include "sim-signal.h"
 #include "sim/callback.h"
diff --git a/sim/common/sim-events.c b/sim/common/sim-events.c
index e2afe2be6c5..ce3a5dc1652 100644
--- a/sim/common/sim-events.c
+++ b/sim/common/sim-events.c
@@ -33,6 +33,12 @@
 
 #include "libiberty.h"
 
+/* signal.h might indirectly have defined some REG_ macros
+   which are also defined in the sim headers.  */
+#ifdef REG_PC
+#undef REG_PC
+#endif
+
 #include "sim-main.h"
 #include "sim-assert.h"
 #include "sim-cpu.h"
diff --git a/sim/common/sim-signal.c b/sim/common/sim-signal.c
index 9c4e261fa21..94d5d9d51c9 100644
--- a/sim/common/sim-signal.c
+++ b/sim/common/sim-signal.c
@@ -22,6 +22,12 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include <signal.h>
 
+/* signal.h might indirectly have defined some REG_ macros
+   which are also defined in the sim headers.  */
+#ifdef REG_PC
+#undef REG_PC
+#endif
+
 #include "sim-main.h"
 #include "sim-signal.h"
 
diff --git a/sim/microblaze/interp.c b/sim/microblaze/interp.c
index a4f505e77a8..f53c1d7d65b 100644
--- a/sim/microblaze/interp.c
+++ b/sim/microblaze/interp.c
@@ -19,7 +19,6 @@
 /* This must come before any other includes.  */
 #include "defs.h"
 
-#include <signal.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
diff --git a/sim/mn10300/interp.c b/sim/mn10300/interp.c
index 2915551253f..07c3b8c900f 100644
--- a/sim/mn10300/interp.c
+++ b/sim/mn10300/interp.c
@@ -1,8 +1,6 @@
 /* This must come before any other includes.  */
 #include "defs.h"
 
-#include <signal.h>
-
 #include "sim-main.h"
 #include "sim-options.h"
 #include "sim-hw.h"
-- 
2.31.1

Re: [PATCH] sim: common, microblaze, mn10300: handle signal.h defining REC_PC.

[Mike Frysinger]

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

On 15 Jan 2023 00:28, Mark Wielaard wrote:
> signal.h isn't needed in microblaze and mn10300 interp.c
> so don't include it.

these changes are fine to merge if you want to split it out

> In common sim-events.c, sim-signal.c, nrun.c and dv-sockser.c we
> do need signal.h, but check whether REG_PC is defined (and then
> undefine it) before including the sim headers.
> 
> It breaks the build on sparc because signal.h indirectly
> includes /usr/include/sys/ucontext.h and defines REG_PC,
> which is also defined in microblaze-opcm.h

i don't think this is correct.  none of the files quoted use REG_PC,
so undefining a random symbol in them doesn't make sense.  nothing in
sim/common/ uses REG_PC for that matter.
-mike

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

Re: [PATCH] sim: common, microblaze, mn10300: handle signal.h defining REC_PC.

[Sam James]

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



> On 15 Jan 2023, at 00:22, Mike Frysinger <vapier@gentoo.org> wrote:
> 
> On 15 Jan 2023 00:28, Mark Wielaard wrote:
>> signal.h isn't needed in microblaze and mn10300 interp.c
>> so don't include it.
> 
> these changes are fine to merge if you want to split it out
> 
>> In common sim-events.c, sim-signal.c, nrun.c and dv-sockser.c we
>> do need signal.h, but check whether REG_PC is defined (and then
>> undefine it) before including the sim headers.
>> 
>> It breaks the build on sparc because signal.h indirectly
>> includes /usr/include/sys/ucontext.h and defines REG_PC,
>> which is also defined in microblaze-opcm.h
> 
> i don't think this is correct.  none of the files quoted use REG_PC,
> so undefining a random symbol in them doesn't make sense.  nothing in
> sim/common/ uses REG_PC for that matter.

The original error (https://builder.sourceware.org/buildbot/#/builders/229/builds/3) is:
```
In file included from ../../binutils-gdb/sim/mn10300/sim-main.h:41,
from ../../binutils-gdb/sim/common/dv-sockser.c:42:
../../binutils-gdb/sim/mn10300/mn10300-sim.h:68: error: "REG_PC" redefined [-Werror]
68 | #define REG_PC 9
|
In file included from /usr/include/signal.h:316,
from ../gnulib/import/signal.h:52,
from ../../binutils-gdb/sim/common/dv-sockser.c:29:
/usr/include/sys/ucontext.h:111: note: this is the location of the previous definition
111 | # define REG_PC (1)
|
```

There's history of just ducking this in other projects, and I can't really blame them:
https://patchwork.kernel.org/project/qemu-devel/patch/1490272961-1128-1-git-send-email-peter.maydell@linaro.org/

Overall, we have:
```
$ grep -rsin "#define.*REG_PC"
sim/mn10300/mn10300_sim.h:57:#define PC (State.regs[REG_PC])
sim/mn10300/mn10300_sim.h:74:#define REG_PC 9
gas/config/tc-arm.c:744:#define REG_PC  15
gprofng/libcollector/unwind.c:111:#define GET_PC(ctx) (((ucontext_t*)ctx)->uc_mcontext.gregs[REG_PC])
opcodes/microblaze-opcm.h:77:#define REG_PC_MASK 0x8000
opcodes/microblaze-opcm.h:101:#define REG_PC  32 /* PC.  */
include/opcode/cris.h:35:#define REG_PC (15)
include/opcode/cris.h:143:#define BDAP_PC_LOW     (BDAP_INDIR_LOW + REG_PC)
```

What do you prefer?

1. Rename all the REG_* (ugly)
2. #undef hack in each of the consumers where there's a #define for it?
3. What Mark did in some misc. top-level sim place
4. Beg every vendor to change their ucontext.h
5. Something else?

thanks,
sam


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: [PATCH] sim: common, microblaze, mn10300: handle signal.h defining REC_PC.

[Mike Frysinger]

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

On 15 Jan 2023 00:58, Sam James wrote:
> > On 15 Jan 2023, at 00:22, Mike Frysinger wrote:
> > On 15 Jan 2023 00:28, Mark Wielaard wrote:
> >> In common sim-events.c, sim-signal.c, nrun.c and dv-sockser.c we
> >> do need signal.h, but check whether REG_PC is defined (and then
> >> undefine it) before including the sim headers.
> >> 
> >> It breaks the build on sparc because signal.h indirectly
> >> includes /usr/include/sys/ucontext.h and defines REG_PC,
> >> which is also defined in microblaze-opcm.h
> > 
> > i don't think this is correct.  none of the files quoted use REG_PC,
> > so undefining a random symbol in them doesn't make sense.  nothing in
> > sim/common/ uses REG_PC for that matter.
> 
> The original error (https://builder.sourceware.org/buildbot/#/builders/229/builds/3) is:
> ```
> In file included from ../../binutils-gdb/sim/mn10300/sim-main.h:41,
> from ../../binutils-gdb/sim/common/dv-sockser.c:42:
> ../../binutils-gdb/sim/mn10300/mn10300-sim.h:68: error: "REG_PC" redefined [-Werror]
> 68 | #define REG_PC 9
> |
> In file included from /usr/include/signal.h:316,
> from ../gnulib/import/signal.h:52,
> from ../../binutils-gdb/sim/common/dv-sockser.c:29:
> /usr/include/sys/ucontext.h:111: note: this is the location of the previous definition
> 111 | # define REG_PC (1)
> |
> ```

the arch sim-main.h shouldn't be bleeding random arch-specific headers
into common headers.  there's a todo in mn10300/sim-main.h about this
with a breadcrumb for how to clean it up.  i've cleaned up most of the
arches at this point (~18), but i've got ~10 to go, and haven't gotten
around to them yet.  basically the cgen & igen based arches.

> There's history of just ducking this in other projects, and I can't really blame them:
> https://patchwork.kernel.org/project/qemu-devel/patch/1490272961-1128-1-git-send-email-peter.maydell@linaro.org/
> 
> Overall, we have:
> ```
> $ grep -rsin "#define.*REG_PC"
> sim/mn10300/mn10300_sim.h:57:#define PC (State.regs[REG_PC])
> sim/mn10300/mn10300_sim.h:74:#define REG_PC 9
> gas/config/tc-arm.c:744:#define REG_PC  15
> gprofng/libcollector/unwind.c:111:#define GET_PC(ctx) (((ucontext_t*)ctx)->uc_mcontext.gregs[REG_PC])
> opcodes/microblaze-opcm.h:77:#define REG_PC_MASK 0x8000
> opcodes/microblaze-opcm.h:101:#define REG_PC  32 /* PC.  */
> include/opcode/cris.h:35:#define REG_PC (15)
> include/opcode/cris.h:143:#define BDAP_PC_LOW     (BDAP_INDIR_LOW + REG_PC)
> ```

there's more conflicts than REG_PC.  it depends on the host & target, and
what set of headers happened to be included.  the REG_* namespace is a
mess, and it's kind of a self-inflicted (i.e. GNU) problem.  ptrace.h is
another place where it sometimes comes up.  REG_R# is a common conflict.

> What do you prefer?
> 
> 1. Rename all the REG_* (ugly)
> 2. #undef hack in each of the consumers where there's a #define for it?
> 3. What Mark did in some misc. top-level sim place
> 4. Beg every vendor to change their ucontext.h
> 5. Something else?

splattering #undef boilerplate around the codebase isn't going to happen.
it's not maintainable.  cleaning up the remaining sim-main.h headers is a
known todo that probably makes the issue go away enough for the sim.

somewhat ironically, i think the current state is due to portability issues
with signal.h.  the only reason sim is building with GNUisms enabled is to
get access to the strsignal prototype in string.h.  POSIX didn't adopt the
function until 2008 edition.
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=2232061b1ccf68bb1e46c95cab6f531831d72aa5

i bet we could drop AC_USE_SYSTEM_EXTENSIONS from the sim now since it's
been in POSIX for more than a decade, and even when i landed that patch,
it was for "old" systems, which makes them double old at this point.

if we find a setup that still lacks strsignal support, we can add it to
our local gnulib/ instead.  although i think gnulib/ enables the system
extensions too, so maybe dropping it from sim wouldn't help as much as
i would like.
-mike

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

Re: [PATCH] sim: common, microblaze, mn10300: handle signal.h defining REC_PC.

[Mark Wielaard]

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

Hi Mike,

On Sat, Jan 14, 2023 at 07:22:29PM -0500, Mike Frysinger wrote:
> On 15 Jan 2023 00:28, Mark Wielaard wrote:
> > signal.h isn't needed in microblaze and mn10300 interp.c
> > so don't include it.
> 
> these changes are fine to merge if you want to split it out

Thanks. I pushed the attached for just those two changed.

> > In common sim-events.c, sim-signal.c, nrun.c and dv-sockser.c we
> > do need signal.h, but check whether REG_PC is defined (and then
> > undefine it) before including the sim headers.
> > 
> > It breaks the build on sparc because signal.h indirectly
> > includes /usr/include/sys/ucontext.h and defines REG_PC,
> > which is also defined in microblaze-opcm.h
> 
> i don't think this is correct.  none of the files quoted use REG_PC,
> so undefining a random symbol in them doesn't make sense.  nothing in
> sim/common/ uses REG_PC for that matter.

I see what you mean. I'll try to come up with another fix for this
part.

Cheers,

Mark

[-- Attachment #2: 0001-sim-microblaze-mn10300-remove-signal.h-include-in-in.patch --]
[-- Type: text/plain, Size: 1080 bytes --]

From ad6adc6657192a2bec1d721f4e2e7743db4c1da0 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Sat, 14 Jan 2023 22:54:20 +0100
Subject: [PATCH] sim: microblaze, mn10300: remove signal.h include in interp.c

signal.h isn't needed in microblaze and mn10300 interp.c
so don't include it.
---
 sim/microblaze/interp.c | 1 -
 sim/mn10300/interp.c    | 2 --
 2 files changed, 3 deletions(-)

diff --git a/sim/microblaze/interp.c b/sim/microblaze/interp.c
index a4f505e77a8..f53c1d7d65b 100644
--- a/sim/microblaze/interp.c
+++ b/sim/microblaze/interp.c
@@ -19,7 +19,6 @@
 /* This must come before any other includes.  */
 #include "defs.h"
 
-#include <signal.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
diff --git a/sim/mn10300/interp.c b/sim/mn10300/interp.c
index 2915551253f..07c3b8c900f 100644
--- a/sim/mn10300/interp.c
+++ b/sim/mn10300/interp.c
@@ -1,8 +1,6 @@
 /* This must come before any other includes.  */
 #include "defs.h"
 
-#include <signal.h>
-
 #include "sim-main.h"
 #include "sim-options.h"
 #include "sim-hw.h"
-- 
2.31.1