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