Re: [musl] REG_SP Definition for RISC-V

[Rich Felker <dalias@libc.org>]

On Tue, Feb 18, 2020 at 11:17:30AM -0800, Palmer Dabbelt wrote:
> On Tue, 04 Feb 2020 06:26:31 PST (-0800), dalias@libc.org wrote:
> >On Tue, Feb 04, 2020 at 10:03:59AM +0000, Mark Corbin wrote:
> >>On Monday, 3 February 2020 15:24:27 GMT Rich Felker wrote:
> >>> On Mon, Feb 03, 2020 at 03:17:15PM +0000, Mark Corbin wrote:
> >>> > On Monday, 3 February 2020 13:32:25 GMT Rich Felker wrote:
> >>> > > On Mon, Feb 03, 2020 at 11:42:30AM +0000, Mark Corbin wrote:
> >>> > > > Hello
> >>> > > >
> >>> > > > I'm trying to fix a build issue with libsigsegv [1] for RISC-V when
> >>> > > > compiling against musl 1.1.24 (under Buildroot).
> >>> > > >
> >>> > > > The build fails because the array index 'REG_SP' (for indexing into
> >>> > > > uc_mcontext.__gregs[]) is not defined in arch/riscv64/bits/signal.h.
> >>> > > > This
> >>> > > > constant is defined by glibc in
> >>> > > > sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> >>> > > >
> >>> > > > I was wondering whether the appropriate fix is just to add '#define
> >>> > > > REG_SP
> >>> > > > 2' to the top of arch/riscv64/bits/signal.h ? (Note that there is a
> >>> > > > REG_SP definition in arch/riscv64/bits/reg.h which isn't being
> >>> > > > included).
> >>> > > >
> >>> > > > Alternatively I could submit a patch to libsigsegv to modify the index
> >>> > > > into
> >>> > > > the '__gregs' array to be '2' rather than 'REG_SP', however there
> >>> > > > could be
> >>> > > > other glibc compatible RISC-V packages that make use of the 'REG_SP'
> >>> > > > definition.
> >>> > > >
> >>> > > > I'm happy to generate and submit any patches as appropriate.
> >>> > >
> >>> > > Generally, we like to avoid this kind of REG_* (or even bare names)
> >>> > > register macro in signal.h since it's highly namespace-polluting (can
> >>> > > break software using them for its own purposes that has no knowledge
> >>> > > that some arch has a reg by that name in its signal.h bits) and only
> >>> > > expose them under _GNU_SOURCE when we do. Right now musl has them
> >>> > > exposed via <sys/reg.h>. I'm not sure if there's any precedent for
> >>> > > that or if glibc only has them in <signal.h>
> >>> >
> >>> > I spent some time looking for a good method of handling this, but couldn't
> >>> > really find any consistency between architectures. I think that most of
> >>> > them access the appropriate register array using a numeric value rather
> >>> > than a register name in this scenario.
> >>> >
> >>> > > So my leaning would be to leave it as it is and ask applications to
> >>> > > include <sys/reg.h> if they want these macros. But if it looks like
> >>> > > this is contrary to what maintainers of other software want to do, we
> >>> > > could consider putting them under _GNU_SOURCE with <signal.h> like
> >>> > > many other archs do.
> >>> >
> >>> > I guess that it would probably be best to change the libsigsegv code to
> >>> > use a value of '2' instead of the REG_SP definition. I'll look at
> >>> > submitting a patch to the project.
> >>>
> >>> I think using a symbolic name is both more informative and more
> >>> portable (since the layout of the saved registers is an OS choice,
> >>> nothing universal to the architecture). The question is just where the
> >>> macro should be obtained from. As long as glibc (and any other
> >>> platforms that might be relevant?) has a sys/reg.h, it wouldn't hurt
> >>> to just add the include and continue using the macro, regardless of
> >>> whether musl moves it later.
> >>
> >>Glibc and uClibc don't have a sys/reg.h - is there a way that it could be
> >>included conditionally for musl only?
> >
> >If you want a configure test to detect it the yes; otherwise no. But
> >this suggests the way we did it is wrong. We should not be making this
> >kind of mess. I should probably just move the definitions...
> 
> The glibc definitions are in sys/ucontext.h as that's also where the relevant
> structures are defined.  They're all within a _GNU_SOURCE to avoid polluting
> the namespace too much.  Maybe the best bet is to have a riscv64-specific
> sys/ucontext.h?  I don't see any other ports with their own sys/ucontext.h,
> though.

The structures have to be defined in signal.h, which they are. glibc
handles this by having signal.h include ucontext.h, which is somewhat
wrong-ish but probably ok by the way the feature test macros are
handled. The reason is historical; long ago when ucontext api was
standard, POSIX specified ucontext.h, and since the only remaining
standard use of ucontext_t/mcontext_t is for SA_SIGINFO signal
handlers, they moved it to signal.h when removing ucontext.h.

Rich