sim: common, microblaze, mn10300: handle signal.h defining REC_PC.
Commit Message
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(-)
Comments
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
> 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
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
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
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"
@@ -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"
@@ -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"
@@ -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"
@@ -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"
@@ -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>
@@ -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"