public inbox for
 help / color / mirror / Atom feed
From: Mike Frysinger <>
To: Sam James <>
Cc: Mark Wielaard <>,
Subject: Re: [PATCH] sim: common, microblaze, mn10300: handle signal.h defining REC_PC.
Date: Sun, 15 Jan 2023 03:07:20 -0500	[thread overview]
Message-ID: <Y8O0ODu8PTsRBe12@vapier> (raw)
In-Reply-To: <>

[-- 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 ( 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:
> 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.;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.

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

  reply	other threads:[~2023-01-15  8:07 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-14 23:28 Mark Wielaard
2023-01-15  0:22 ` Mike Frysinger
2023-01-15  0:58   ` Sam James
2023-01-15  8:07     ` Mike Frysinger [this message]
2023-01-15 17:48   ` Mark Wielaard

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y8O0ODu8PTsRBe12@vapier \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).