mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] [PATCH v2] riscv64: correct struct __ucontext name
@ 2020-12-05 18:10 Drew DeVault
  2020-12-06  8:51 ` Szabolcs Nagy
  0 siblings, 1 reply; 8+ messages in thread
From: Drew DeVault @ 2020-12-05 18:10 UTC (permalink / raw)
  To: musl; +Cc: Drew DeVault


This makes it consistent with other architectures and fixes some issues
with downstream software.
---
v1 missed a spot. Please Cc me on replies.

 arch/riscv64/bits/signal.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/riscv64/bits/signal.h b/arch/riscv64/bits/signal.h
index b006334f..287367db 100644
--- a/arch/riscv64/bits/signal.h
+++ b/arch/riscv64/bits/signal.h
@@ -60,10 +60,10 @@ struct sigaltstack {
 	size_t ss_size;
 };
 
-typedef struct ucontext_t
+typedef struct __ucontext
 {
 	unsigned long uc_flags;
-	struct ucontext_t *uc_link;
+	struct __ucontext *uc_link;
 	stack_t uc_stack;
 	sigset_t uc_sigmask;
 	mcontext_t uc_mcontext;
-- 
2.29.2


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

* Re: [musl] [PATCH v2] riscv64: correct struct __ucontext name
  2020-12-05 18:10 [musl] [PATCH v2] riscv64: correct struct __ucontext name Drew DeVault
@ 2020-12-06  8:51 ` Szabolcs Nagy
  2020-12-06 12:49   ` Drew DeVault
  0 siblings, 1 reply; 8+ messages in thread
From: Szabolcs Nagy @ 2020-12-06  8:51 UTC (permalink / raw)
  To: Drew DeVault; +Cc: musl

* Drew DeVault <sir@cmpwn.com> [2020-12-05 18:10:06 +0000]:
> This makes it consistent with other architectures and fixes some issues
> with downstream software.

which software?

glibc uses struct ucontext_t too and user code should use ucontext_t
without struct.

> ---
> v1 missed a spot. Please Cc me on replies.
> 
>  arch/riscv64/bits/signal.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv64/bits/signal.h b/arch/riscv64/bits/signal.h
> index b006334f..287367db 100644
> --- a/arch/riscv64/bits/signal.h
> +++ b/arch/riscv64/bits/signal.h
> @@ -60,10 +60,10 @@ struct sigaltstack {
>  	size_t ss_size;
>  };
>  
> -typedef struct ucontext_t
> +typedef struct __ucontext
>  {
>  	unsigned long uc_flags;
> -	struct ucontext_t *uc_link;
> +	struct __ucontext *uc_link;
>  	stack_t uc_stack;
>  	sigset_t uc_sigmask;
>  	mcontext_t uc_mcontext;
> -- 
> 2.29.2

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

* Re: [musl] [PATCH v2] riscv64: correct struct __ucontext name
  2020-12-06  8:51 ` Szabolcs Nagy
@ 2020-12-06 12:49   ` Drew DeVault
  2020-12-06 16:55     ` Ariadne Conill
  0 siblings, 1 reply; 8+ messages in thread
From: Drew DeVault @ 2020-12-06 12:49 UTC (permalink / raw)
  To: Szabolcs Nagy; +Cc: musl


On Sun Dec 6, 2020 at 3:51 AM EST, Szabolcs Nagy wrote:
> * Drew DeVault <sir@cmpwn.com> [2020-12-05 18:10:06 +0000]:
> > This makes it consistent with other architectures and fixes some issues
> > with downstream software.
>
> which software?
>
> glibc uses struct ucontext_t too and user code should use ucontext_t
> without struct.

libucontext, which does use ucontext_t.

In fact, the issue was more related to the type conflict with
ucontext.h, which declared struct __ucontext in the scope of its
function declarations due to the naming mismatch.

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

* Re: [musl] [PATCH v2] riscv64: correct struct __ucontext name
  2020-12-06 12:49   ` Drew DeVault
@ 2020-12-06 16:55     ` Ariadne Conill
  2020-12-06 17:06       ` Rich Felker
  0 siblings, 1 reply; 8+ messages in thread
From: Ariadne Conill @ 2020-12-06 16:55 UTC (permalink / raw)
  To: Szabolcs Nagy, musl; +Cc: musl, Drew DeVault


Hello,

On Sunday, December 6, 2020 5:49:25 AM MST Drew DeVault wrote:
> On Sun Dec 6, 2020 at 3:51 AM EST, Szabolcs Nagy wrote:
> > * Drew DeVault <sir@cmpwn.com> [2020-12-05 18:10:06 +0000]:
> > > This makes it consistent with other architectures and fixes some issues
> > > with downstream software.
> > 
> > which software?
> > 
> > glibc uses struct ucontext_t too and user code should use ucontext_t
> > without struct.

Some glibc architecture ports use the struct __ucontext and even struct 
ucontext names, or at least did in the past.

> libucontext, which does use ucontext_t.
> 
> In fact, the issue was more related to the type conflict with
> ucontext.h, which declared struct __ucontext in the scope of its
> function declarations due to the naming mismatch.

glibc uses the POSIX 2004 standardized ucontext_t type in its public 
definitions.  I believe musl should do the same.

As far as libucontext goes, this is increasingly moot because 0.13 will 
introduce freestanding mode which avoids the musl definitions entirely, instead 
using simplified (though ABI compatible) definitions, allowing it to not only be 
used on musl but on other libc and other OS entirely (for example, it is known 
to now build on AmigaOS and Darwin).

libucontext using its own definitions is an important step toward eventually 
taking ucontext.h out of musl entirely, and providing it in libucontext 
instead, too, which I think musl should do since the ucontext API was dropped 
from POSIX.

But right now, I think the best way forward is to leave the architecture 
headers alone and just fix the ucontext.h definitions instead.  I can send a 
patch doing that if you want to focus on other things.

Ariadne



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

* Re: [musl] [PATCH v2] riscv64: correct struct __ucontext name
  2020-12-06 16:55     ` Ariadne Conill
@ 2020-12-06 17:06       ` Rich Felker
  2020-12-06 17:10         ` Ariadne Conill
  0 siblings, 1 reply; 8+ messages in thread
From: Rich Felker @ 2020-12-06 17:06 UTC (permalink / raw)
  To: Ariadne Conill; +Cc: Szabolcs Nagy, musl, Drew DeVault

On Sun, Dec 06, 2020 at 04:55:39PM +0000, Ariadne Conill wrote:
> 
> Hello,
> 
> On Sunday, December 6, 2020 5:49:25 AM MST Drew DeVault wrote:
> > On Sun Dec 6, 2020 at 3:51 AM EST, Szabolcs Nagy wrote:
> > > * Drew DeVault <sir@cmpwn.com> [2020-12-05 18:10:06 +0000]:
> > > > This makes it consistent with other architectures and fixes some issues
> > > > with downstream software.
> > > 
> > > which software?
> > > 
> > > glibc uses struct ucontext_t too and user code should use ucontext_t
> > > without struct.
> 
> Some glibc architecture ports use the struct __ucontext and even struct 
> ucontext names, or at least did in the past.
> 
> > libucontext, which does use ucontext_t.
> > 
> > In fact, the issue was more related to the type conflict with
> > ucontext.h, which declared struct __ucontext in the scope of its
> > function declarations due to the naming mismatch.
> 
> glibc uses the POSIX 2004 standardized ucontext_t type in its public 
> definitions.  I believe musl should do the same.

This produces a compile-time error is ucontext.h is included without
the right feature test macros, since signal.h will not have defined
ucontext_t in that case. That's why the public declarations must use
the struct tag.

> As far as libucontext goes, this is increasingly moot because 0.13 will 
> introduce freestanding mode which avoids the musl definitions entirely, instead 
> using simplified (though ABI compatible) definitions, allowing it to not only be 
> used on musl but on other libc and other OS entirely (for example, it is known 
> to now build on AmigaOS and Darwin).
> 
> libucontext using its own definitions is an important step toward eventually 
> taking ucontext.h out of musl entirely, and providing it in libucontext 
> instead, too, which I think musl should do since the ucontext API was dropped 
> from POSIX.

It's still an open question whether musl will eventually add these,
but the contents of ucontext.h are independent of the implementation;
they're just function declarations. The *type* declarations are in
signal.h and are *not* dropped from POSIX, so they can't be removed.

> But right now, I think the best way forward is to leave the architecture 
> headers alone and just fix the ucontext.h definitions instead.  I can send a 
> patch doing that if you want to focus on other things.

As described above this does not work.

Rich

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

* Re: [musl] [PATCH v2] riscv64: correct struct __ucontext name
  2020-12-06 17:06       ` Rich Felker
@ 2020-12-06 17:10         ` Ariadne Conill
  2020-12-06 17:19           ` Rich Felker
  0 siblings, 1 reply; 8+ messages in thread
From: Ariadne Conill @ 2020-12-06 17:10 UTC (permalink / raw)
  To: Rich Felker; +Cc: Szabolcs Nagy, musl, Drew DeVault


On Sunday, December 6, 2020 10:06:49 AM MST Rich Felker wrote:
> On Sun, Dec 06, 2020 at 04:55:39PM +0000, Ariadne Conill wrote:
> > Hello,
> > 
> > On Sunday, December 6, 2020 5:49:25 AM MST Drew DeVault wrote:
> > > On Sun Dec 6, 2020 at 3:51 AM EST, Szabolcs Nagy wrote:
> > > > * Drew DeVault <sir@cmpwn.com> [2020-12-05 18:10:06 +0000]:
> > > > > This makes it consistent with other architectures and fixes some
> > > > > issues
> > > > > with downstream software.
> > > > 
> > > > which software?
> > > > 
> > > > glibc uses struct ucontext_t too and user code should use ucontext_t
> > > > without struct.
> > 
> > Some glibc architecture ports use the struct __ucontext and even struct
> > ucontext names, or at least did in the past.
> > 
> > > libucontext, which does use ucontext_t.
> > > 
> > > In fact, the issue was more related to the type conflict with
> > > ucontext.h, which declared struct __ucontext in the scope of its
> > > function declarations due to the naming mismatch.
> > 
> > glibc uses the POSIX 2004 standardized ucontext_t type in its public
> > definitions.  I believe musl should do the same.
> 
> This produces a compile-time error is ucontext.h is included without
> the right feature test macros, since signal.h will not have defined
> ucontext_t in that case. That's why the public declarations must use
> the struct tag.

Bummer.  In that case, I suggest musl use the same struct tag consistently. It 
should probably be struct ucontext_t for consistency with glibc.

Ariadne



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

* Re: [musl] [PATCH v2] riscv64: correct struct __ucontext name
  2020-12-06 17:10         ` Ariadne Conill
@ 2020-12-06 17:19           ` Rich Felker
  2020-12-06 17:32             ` Szabolcs Nagy
  0 siblings, 1 reply; 8+ messages in thread
From: Rich Felker @ 2020-12-06 17:19 UTC (permalink / raw)
  To: Ariadne Conill; +Cc: Szabolcs Nagy, musl, Drew DeVault

On Sun, Dec 06, 2020 at 05:10:25PM +0000, Ariadne Conill wrote:
> 
> On Sunday, December 6, 2020 10:06:49 AM MST Rich Felker wrote:
> > On Sun, Dec 06, 2020 at 04:55:39PM +0000, Ariadne Conill wrote:
> > > Hello,
> > > 
> > > On Sunday, December 6, 2020 5:49:25 AM MST Drew DeVault wrote:
> > > > On Sun Dec 6, 2020 at 3:51 AM EST, Szabolcs Nagy wrote:
> > > > > * Drew DeVault <sir@cmpwn.com> [2020-12-05 18:10:06 +0000]:
> > > > > > This makes it consistent with other architectures and fixes some
> > > > > > issues
> > > > > > with downstream software.
> > > > > 
> > > > > which software?
> > > > > 
> > > > > glibc uses struct ucontext_t too and user code should use ucontext_t
> > > > > without struct.
> > > 
> > > Some glibc architecture ports use the struct __ucontext and even struct
> > > ucontext names, or at least did in the past.
> > > 
> > > > libucontext, which does use ucontext_t.
> > > > 
> > > > In fact, the issue was more related to the type conflict with
> > > > ucontext.h, which declared struct __ucontext in the scope of its
> > > > function declarations due to the naming mismatch.
> > > 
> > > glibc uses the POSIX 2004 standardized ucontext_t type in its public
> > > definitions.  I believe musl should do the same.
> > 
> > This produces a compile-time error is ucontext.h is included without
> > the right feature test macros, since signal.h will not have defined
> > ucontext_t in that case. That's why the public declarations must use
> > the struct tag.
> 
> Bummer.  In that case, I suggest musl use the same struct tag consistently. It 
> should probably be struct ucontext_t for consistency with glibc.

I don't recall the original reason it was inconsitent with glibc here.
Note that signal.h has (which is probably wrong and should be
removed):

#ifdef _GNU_SOURCE
#define __ucontext ucontext
#endif

so that the tag gets renamed if _GNU_SOURCE is defined. Maybe at one
point glibc called it "struct ucontext", which was a namespace
violation since it's not in a reserved namespace, and we used
__ucontext to get the freedom to rename it and expose that only in a
_GNU_SOURCE context.

Whatever the reason, though, the struct tags is C++ABI and should not
be changed. The rv64 inconsitency was unintentional and hopefully is
not yet widely used enough for anyone to care that it's being fixed
now (in theory could mess up linking C++ libs with ucontext_t in their
public interface surface).

Note that none of this is visible to applications that aren't doing
anything horribly wrong. Neither "struct ucontext_t" nor "struct
__ucontext" (nor "struct ucontext", which it looks like we were
wrongly trying to support) is API.

Rich

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

* Re: [musl] [PATCH v2] riscv64: correct struct __ucontext name
  2020-12-06 17:19           ` Rich Felker
@ 2020-12-06 17:32             ` Szabolcs Nagy
  0 siblings, 0 replies; 8+ messages in thread
From: Szabolcs Nagy @ 2020-12-06 17:32 UTC (permalink / raw)
  To: Rich Felker; +Cc: Ariadne Conill, musl, Drew DeVault

* Rich Felker <dalias@libc.org> [2020-12-06 12:19:30 -0500]:
> On Sun, Dec 06, 2020 at 05:10:25PM +0000, Ariadne Conill wrote:
> > Bummer.  In that case, I suggest musl use the same struct tag consistently. It 
> > should probably be struct ucontext_t for consistency with glibc.
> 
> I don't recall the original reason it was inconsitent with glibc here.

glibc changed the declaration 3 years ago (breaking c++ abi)
before that it was not posix conform so musl could not be
consistent with it.

see "Rename struct ucontext tag" in
https://sourceware.org/bugzilla/show_bug.cgi?id=21457

> Note that signal.h has (which is probably wrong and should be
> removed):
> 
> #ifdef _GNU_SOURCE
> #define __ucontext ucontext
> #endif
> 
> so that the tag gets renamed if _GNU_SOURCE is defined. Maybe at one
> point glibc called it "struct ucontext", which was a namespace
> violation since it's not in a reserved namespace, and we used
> __ucontext to get the freedom to rename it and expose that only in a
> _GNU_SOURCE context.
> 
> Whatever the reason, though, the struct tags is C++ABI and should not
> be changed. The rv64 inconsitency was unintentional and hopefully is
> not yet widely used enough for anyone to care that it's being fixed
> now (in theory could mess up linking C++ libs with ucontext_t in their
> public interface surface).
> 
> Note that none of this is visible to applications that aren't doing
> anything horribly wrong. Neither "struct ucontext_t" nor "struct
> __ucontext" (nor "struct ucontext", which it looks like we were
> wrongly trying to support) is API.
> 
> Rich

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

end of thread, other threads:[~2020-12-06 17:32 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-05 18:10 [musl] [PATCH v2] riscv64: correct struct __ucontext name Drew DeVault
2020-12-06  8:51 ` Szabolcs Nagy
2020-12-06 12:49   ` Drew DeVault
2020-12-06 16:55     ` Ariadne Conill
2020-12-06 17:06       ` Rich Felker
2020-12-06 17:10         ` Ariadne Conill
2020-12-06 17:19           ` Rich Felker
2020-12-06 17:32             ` Szabolcs Nagy

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