mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] REG_SP Definition for RISC-V
@ 2020-02-03 11:42 Mark Corbin
  2020-02-03 13:32 ` Rich Felker
  0 siblings, 1 reply; 10+ messages in thread
From: Mark Corbin @ 2020-02-03 11:42 UTC (permalink / raw)
  To: musl

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.

Thanks

Mark Corbin

[1] http://savannah.gnu.org/projects/libsigsegv/





^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] REG_SP Definition for RISC-V
  2020-02-03 11:42 [musl] REG_SP Definition for RISC-V Mark Corbin
@ 2020-02-03 13:32 ` Rich Felker
  2020-02-03 15:17   ` Mark Corbin
  0 siblings, 1 reply; 10+ messages in thread
From: Rich Felker @ 2020-02-03 13:32 UTC (permalink / raw)
  To: musl; +Cc: Mark Corbin

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>

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.

Rich

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] REG_SP Definition for RISC-V
  2020-02-03 13:32 ` Rich Felker
@ 2020-02-03 15:17   ` Mark Corbin
  2020-02-03 15:24     ` Rich Felker
  0 siblings, 1 reply; 10+ messages in thread
From: Mark Corbin @ 2020-02-03 15:17 UTC (permalink / raw)
  To: musl; +Cc: Rich Felker

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.

Thanks

Mark Corbin




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] REG_SP Definition for RISC-V
  2020-02-03 15:17   ` Mark Corbin
@ 2020-02-03 15:24     ` Rich Felker
  2020-02-04 10:03       ` Mark Corbin
  0 siblings, 1 reply; 10+ messages in thread
From: Rich Felker @ 2020-02-03 15:24 UTC (permalink / raw)
  To: Mark Corbin; +Cc: musl

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.

Rich

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] REG_SP Definition for RISC-V
  2020-02-03 15:24     ` Rich Felker
@ 2020-02-04 10:03       ` Mark Corbin
  2020-02-04 14:26         ` Rich Felker
  0 siblings, 1 reply; 10+ messages in thread
From: Mark Corbin @ 2020-02-04 10:03 UTC (permalink / raw)
  To: musl; +Cc: Rich Felker

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?

Thanks

Mark



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] REG_SP Definition for RISC-V
  2020-02-04 10:03       ` Mark Corbin
@ 2020-02-04 14:26         ` Rich Felker
  2020-02-04 14:31           ` [musl] [PATCH] move riscv64 register index constants to signal.h Rich Felker
  2020-02-18 19:17           ` [musl] REG_SP Definition for RISC-V Palmer Dabbelt
  0 siblings, 2 replies; 10+ messages in thread
From: Rich Felker @ 2020-02-04 14:26 UTC (permalink / raw)
  To: Mark Corbin; +Cc: musl

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...

Rich

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [musl] [PATCH] move riscv64 register index constants to signal.h
  2020-02-04 14:26         ` Rich Felker
@ 2020-02-04 14:31           ` Rich Felker
  2020-02-11 14:19             ` Mark Corbin
  2020-02-18 19:17           ` [musl] REG_SP Definition for RISC-V Palmer Dabbelt
  1 sibling, 1 reply; 10+ messages in thread
From: Rich Felker @ 2020-02-04 14:31 UTC (permalink / raw)
  To: Mark Corbin; +Cc: musl

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

On Tue, Feb 04, 2020 at 09:26:31AM -0500, Rich Felker wrote:
> > > > 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...

Patch attached. Any objections?

Rich

[-- Attachment #2: 0001-move-riscv64-register-index-constants-to-signal.h.patch --]
[-- Type: text/plain, Size: 1417 bytes --]

From 329e79299daaa994b8e75941331a1093051ea5d9 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Tue, 4 Feb 2020 09:29:13 -0500
Subject: [PATCH] move riscv64 register index constants to signal.h

under _GNU_SOURCE for namespace cleanliness, analogous to other archs.
the original placement in sys/reg.h seems not to have been motivated;
such a header isn't even present on other implementations.
---
 arch/riscv64/bits/reg.h    | 6 ------
 arch/riscv64/bits/signal.h | 9 +++++++++
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/riscv64/bits/reg.h b/arch/riscv64/bits/reg.h
index c800788c..2633f39d 100644
--- a/arch/riscv64/bits/reg.h
+++ b/arch/riscv64/bits/reg.h
@@ -1,8 +1,2 @@
 #undef __WORDSIZE
 #define __WORDSIZE 64
-#define REG_PC 0
-#define REG_RA 1
-#define REG_SP 2
-#define REG_TP 4
-#define REG_S0 8
-#define REG_A0 10
diff --git a/arch/riscv64/bits/signal.h b/arch/riscv64/bits/signal.h
index 2ff4be30..b006334f 100644
--- a/arch/riscv64/bits/signal.h
+++ b/arch/riscv64/bits/signal.h
@@ -35,6 +35,15 @@ typedef struct mcontext_t {
 	union __riscv_mc_fp_state __fpregs;
 } mcontext_t;
 
+#if defined(_GNU_SOURCE)
+#define REG_PC 0
+#define REG_RA 1
+#define REG_SP 2
+#define REG_TP 4
+#define REG_S0 8
+#define REG_A0 10
+#endif
+
 #if defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
 typedef unsigned long greg_t;
 typedef unsigned long gregset_t[32];
-- 
2.21.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH] move riscv64 register index constants to signal.h
  2020-02-04 14:31           ` [musl] [PATCH] move riscv64 register index constants to signal.h Rich Felker
@ 2020-02-11 14:19             ` Mark Corbin
  0 siblings, 0 replies; 10+ messages in thread
From: Mark Corbin @ 2020-02-11 14:19 UTC (permalink / raw)
  To: musl; +Cc: Rich Felker

On Tuesday, 4 February 2020 14:31:36 GMT Rich Felker wrote:
> On Tue, Feb 04, 2020 at 09:26:31AM -0500, Rich Felker wrote:
> > > > > 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...
> 
> Patch attached. Any objections?
> 
> Rich

Thanks Rich, that solves the problem.





^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] REG_SP Definition for RISC-V
  2020-02-04 14:26         ` Rich Felker
  2020-02-04 14:31           ` [musl] [PATCH] move riscv64 register index constants to signal.h Rich Felker
@ 2020-02-18 19:17           ` Palmer Dabbelt
  2020-02-19  3:17             ` Rich Felker
  1 sibling, 1 reply; 10+ messages in thread
From: Palmer Dabbelt @ 2020-02-18 19:17 UTC (permalink / raw)
  To: dalias; +Cc: mark, musl

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] REG_SP Definition for RISC-V
  2020-02-18 19:17           ` [musl] REG_SP Definition for RISC-V Palmer Dabbelt
@ 2020-02-19  3:17             ` Rich Felker
  0 siblings, 0 replies; 10+ messages in thread
From: Rich Felker @ 2020-02-19  3:17 UTC (permalink / raw)
  To: musl

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-02-19  3:43 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-03 11:42 [musl] REG_SP Definition for RISC-V Mark Corbin
2020-02-03 13:32 ` Rich Felker
2020-02-03 15:17   ` Mark Corbin
2020-02-03 15:24     ` Rich Felker
2020-02-04 10:03       ` Mark Corbin
2020-02-04 14:26         ` Rich Felker
2020-02-04 14:31           ` [musl] [PATCH] move riscv64 register index constants to signal.h Rich Felker
2020-02-11 14:19             ` Mark Corbin
2020-02-18 19:17           ` [musl] REG_SP Definition for RISC-V Palmer Dabbelt
2020-02-19  3:17             ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

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 NNTP newsgroup(s).