diff mbox series

[2/2] mips: Remove rt_sigreturn usage on context function

Message ID 20190215141132.24404-2-adhemerval.zanella@linaro.org
State New
Headers show
Series [1/2] powerpc: Remove rt_sigreturn usage on context function | expand

Commit Message

Adhemerval Zanella Feb. 15, 2019, 2:11 p.m. UTC
Similar to powerpc, mips also issues rt_sigreturn for setcontext
case the v0 value saved is not the one set by setcontext or
makecontext. As for powerpc, it is intention is no really supported
since setcontext is not async-signal-safe.

Checked the context tests on mips64-linux-gnu and mips-linux-gnu.

	* sysdeps/unix/sysv/linux/mips/getcontext.S (__getcontext): Remove
	the magic flag store.
	* sysdeps/unix/sysv/linux/mips/makecontext.S (__makecontext):
	Likewise.
	* sysdeps/unix/sysv/linux/mips/swapcontext.S (__swapcontext):
	Likewise.
	* sysdeps/unix/sysv/linux/mips/setcontext.S (__setcontext):
	Remove rt_sigreturn call.
---
 sysdeps/unix/sysv/linux/mips/getcontext.S  |  5 ---
 sysdeps/unix/sysv/linux/mips/makecontext.S |  5 ---
 sysdeps/unix/sysv/linux/mips/setcontext.S  | 38 ----------------------
 sysdeps/unix/sysv/linux/mips/swapcontext.S |  5 ---
 4 files changed, 53 deletions(-)

-- 
2.17.1

Comments

Adhemerval Zanella April 17, 2019, 5:10 p.m. UTC | #1
If no one opposes I will commit this shortly.

On 15/02/2019 12:11, Adhemerval Zanella wrote:
> Similar to powerpc, mips also issues rt_sigreturn for setcontext

> case the v0 value saved is not the one set by setcontext or

> makecontext. As for powerpc, it is intention is no really supported

> since setcontext is not async-signal-safe.

> 

> Checked the context tests on mips64-linux-gnu and mips-linux-gnu.

> 

> 	* sysdeps/unix/sysv/linux/mips/getcontext.S (__getcontext): Remove

> 	the magic flag store.

> 	* sysdeps/unix/sysv/linux/mips/makecontext.S (__makecontext):

> 	Likewise.

> 	* sysdeps/unix/sysv/linux/mips/swapcontext.S (__swapcontext):

> 	Likewise.

> 	* sysdeps/unix/sysv/linux/mips/setcontext.S (__setcontext):

> 	Remove rt_sigreturn call.

> ---

>  sysdeps/unix/sysv/linux/mips/getcontext.S  |  5 ---

>  sysdeps/unix/sysv/linux/mips/makecontext.S |  5 ---

>  sysdeps/unix/sysv/linux/mips/setcontext.S  | 38 ----------------------

>  sysdeps/unix/sysv/linux/mips/swapcontext.S |  5 ---

>  4 files changed, 53 deletions(-)

> 

> diff --git a/sysdeps/unix/sysv/linux/mips/getcontext.S b/sysdeps/unix/sysv/linux/mips/getcontext.S

> index 4f7f89ee9a..015bd5bff6 100644

> --- a/sysdeps/unix/sysv/linux/mips/getcontext.S

> +++ b/sysdeps/unix/sysv/linux/mips/getcontext.S

> @@ -78,11 +78,6 @@ NESTED (__getcontext, FRAMESZ, ra)

>  	.set	at

>  #endif

>  

> -	/* Store a magic flag.	*/

> -	li	v1, 1

> -	/* zero */

> -	REG_S	v1, (MCONTEXT_GREGOFF + 0 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)

> -

>  	REG_S	s0, (MCONTEXT_GREGOFF + 16 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)

>  	REG_S	s1, (MCONTEXT_GREGOFF + 17 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)

>  	REG_S	s2, (MCONTEXT_GREGOFF + 18 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)

> diff --git a/sysdeps/unix/sysv/linux/mips/makecontext.S b/sysdeps/unix/sysv/linux/mips/makecontext.S

> index 4439fec3ff..3f40f7c9f2 100644

> --- a/sysdeps/unix/sysv/linux/mips/makecontext.S

> +++ b/sysdeps/unix/sysv/linux/mips/makecontext.S

> @@ -93,11 +93,6 @@ NESTED (__makecontext, FRAMESZ, ra)

>  	REG_S	a7, A7OFF(sp)

>  #endif

>  

> -	/* Store a magic flag.  */

> -	li	v1, 1

> -	/* zero */

> -	REG_S	v1, (MCONTEXT_GREGOFF + 0 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)

> -

>  	/* Set up the stack.  */

>  	PTR_L	t0, STACK_SP(a0)

>  	PTR_L	t2, STACK_SIZE(a0)

> diff --git a/sysdeps/unix/sysv/linux/mips/setcontext.S b/sysdeps/unix/sysv/linux/mips/setcontext.S

> index b6553bdb5e..98afe6cbac 100644

> --- a/sysdeps/unix/sysv/linux/mips/setcontext.S

> +++ b/sysdeps/unix/sysv/linux/mips/setcontext.S

> @@ -77,12 +77,6 @@ NESTED (__setcontext, FRAMESZ, ra)

>  	.set	at

>  #endif

>  

> -	/* Check for the magic flag.  */

> -	li	v0, 1

> -	/* zero */

> -	REG_L	v1, (MCONTEXT_GREGOFF + 0 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)

> -	bne	v0, v1, 98f

> -

>  	REG_S	a0, A0OFF(sp)

>  

>  /* rt_sigprocmask (SIG_SETMASK, &ucp->uc_sigmask, NULL, _NSIG8) */

> @@ -154,38 +148,6 @@ NESTED (__setcontext, FRAMESZ, ra)

>  	move	v0, zero

>  	jr	t9

>  

> -98:

> -	/* This is a context obtained from a signal handler.

> -	   Perform a full restore by pushing the context

> -	   passed onto a simulated signal frame on the stack

> -	   and call the signal return syscall as if a signal

> -	   handler exited normally.  */

> -	PTR_ADDIU sp, -((RT_SIGFRAME_SIZE + ALSZ) & ALMASK)

> -	cfi_adjust_cfa_offset ((RT_SIGFRAME_SIZE + ALSZ) & ALMASK)

> -

> -	/* Only ucontext is referred to from rt_sigreturn,

> -	   copy it.  */

> -	PTR_ADDIU t1, sp, RT_SIGFRAME_UCONTEXT

> -	li	t3, ((UCONTEXT_SIZE + SZREG - 1) / SZREG) - 1

> -0:

> -	REG_L	t2, (a0)

> -	PTR_ADDIU a0, SZREG

> -	REG_S	t2, (t1)

> -	PTR_ADDIU t1, SZREG

> -	.set	noreorder

> -	bgtz	t3, 0b

> -	 addiu	t3, -1

> -	.set	reorder

> -

> -/* rt_sigreturn () -- no arguments, sp points to struct rt_sigframe.  */

> -	li	v0, SYS_ify (rt_sigreturn)

> -	syscall

> -

> -	/* Restore the stack and fall through to the error

> -	   path.  Successful rt_sigreturn never returns to

> -	   its calling place.  */

> -	PTR_ADDIU sp, ((RT_SIGFRAME_SIZE + ALSZ) & ALMASK)

> -	cfi_adjust_cfa_offset (-((RT_SIGFRAME_SIZE + ALSZ) & ALMASK))

>  99:

>  #ifdef __PIC__

>  	PTR_LA	t9, JUMPTARGET (__syscall_error)

> diff --git a/sysdeps/unix/sysv/linux/mips/swapcontext.S b/sysdeps/unix/sysv/linux/mips/swapcontext.S

> index 9c68961345..6612e757cd 100644

> --- a/sysdeps/unix/sysv/linux/mips/swapcontext.S

> +++ b/sysdeps/unix/sysv/linux/mips/swapcontext.S

> @@ -87,11 +87,6 @@ NESTED (__swapcontext, FRAMESZ, ra)

>  	.set	at

>  #endif

>  

> -	/* Store a magic flag.	*/

> -	li	v1, 1

> -	/* zero */

> -	REG_S	v1, (MCONTEXT_GREGOFF + 0 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)

> -

>  	REG_S	s0, (MCONTEXT_GREGOFF + 16 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)

>  	REG_S	s1, (MCONTEXT_GREGOFF + 17 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)

>  	REG_S	s2, (MCONTEXT_GREGOFF + 18 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)

>
Maciej W. Rozycki June 1, 2019, 2:22 a.m. UTC | #2
On Wed, 17 Apr 2019, Adhemerval Zanella wrote:

> If no one opposes I will commit this shortly.

> 

> On 15/02/2019 12:11, Adhemerval Zanella wrote:

> > Similar to powerpc, mips also issues rt_sigreturn for setcontext

> > case the v0 value saved is not the one set by setcontext or

> > makecontext. As for powerpc, it is intention is no really supported

> > since setcontext is not async-signal-safe.


 This is an obsolete interface, but FYI according to the relevant version 
of SUS[1] you are supposed to be able to return from a signal handler by 
calling setcontext(3) similarly to siglongjmp(3) and by removing this code 
you make the context not to be fully restored in that case (on the MIPS 
target the distinction is due to the DSP register state).

 What do you mean by: "setcontext is not async-signal-safe"?  I think such 
an assertion (if indeed true) should have been explained in the commit 
message even if it was previously discussed (where?).

References:

[1] <http://pubs.opengroup.org/onlinepubs/009695399/functions/setcontext.html>

  Maciej
Adhemerval Zanella June 3, 2019, 1:13 p.m. UTC | #3
On 31/05/2019 23:22, Maciej W. Rozycki wrote:
> On Wed, 17 Apr 2019, Adhemerval Zanella wrote:

> 

>> If no one opposes I will commit this shortly.

>>

>> On 15/02/2019 12:11, Adhemerval Zanella wrote:

>>> Similar to powerpc, mips also issues rt_sigreturn for setcontext

>>> case the v0 value saved is not the one set by setcontext or

>>> makecontext. As for powerpc, it is intention is no really supported

>>> since setcontext is not async-signal-safe.

> 

>  This is an obsolete interface, but FYI according to the relevant version 

> of SUS[1] you are supposed to be able to return from a signal handler by 

> calling setcontext(3) similarly to siglongjmp(3) and by removing this code 

> you make the context not to be fully restored in that case (on the MIPS 

> target the distinction is due to the DSP register state).

> 

>  What do you mean by: "setcontext is not async-signal-safe"?  I think such 

> an assertion (if indeed true) should have been explained in the commit 

> message even if it was previously discussed (where?).


It is in fact blurry on POSIX 2001, since it defines on '3. Definitions' [1] that
function *is not* async-cancel-safe unless explicitly described as such and
later on '2.4 Signal Concepts' [2] it defines the function set that should be
async-signal-safe and it does not list 'setcontext' as one.

It is even more complicated because this is not portable over architectures on
Linux, since:

    1. not every one supports rt_sigaction. It means that either that architecture
       does not really require additional context that userland might not
       save/restore or that architecture extensions save/restore are not really 
       supported.
       For instance, it seems that newer architectures (like RiscV) follow this
       approach and just really support synchronous by saving/restore the preserved
       state. It is the same with some newer ABI extension, such as AArch64 SVE.

    2. Not every architecture passes an mcontext_t and third argument for signal
       handlers with SA_SIGINFO (sparc and ia64 for instance). 

    3. Even for architecture that does support passing an mcontext_t, userland
       can not have enough information that some context is lazy-saved (such some
       architecture extension such as vector register) by the kernel.

So that's why I think setcontext is not really aync-signal-safe and it is really
hard to get it right in Linux (check powerpc complexity over time to support 
Altivec and later VSX) and while it is support is very architecture-specific.  

I am not really against reverting the patch, and it might be better outcome for an 
already deprecated symbol, but my idea was to make it somewhat more sane and 
portable on Linux.

As a side node, should we move these symbols to deprecated status?

[1] http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html

> 

> References:

> 

> [1] <http://pubs.opengroup.org/onlinepubs/009695399/functions/setcontext.html>

> 

>   Maciej

>
Joseph Myers June 3, 2019, 5:33 p.m. UTC | #4
On Mon, 3 Jun 2019, Adhemerval Zanella wrote:

> As a side node, should we move these symbols to deprecated status?


I think POSIX considering these functions as obsolescent is because it's 
hard to define them in a way that's as portable as POSIX APIs try to be, 
not because POSIX provides a suitable replacement for the application uses 
of these APIs - much as with some other functions such as vfork.  That is, 
we should keep them as supported for applications trying to do less 
portable things using them, because the scope of applications supported by 
glibc is wider than that of portable applications supported by POSIX.

-- 
Joseph S. Myers
joseph@codesourcery.com
Maciej W. Rozycki June 24, 2019, 4:55 p.m. UTC | #5
On Mon, 3 Jun 2019, Adhemerval Zanella wrote:

> >  This is an obsolete interface, but FYI according to the relevant version 

> > of SUS[1] you are supposed to be able to return from a signal handler by 

> > calling setcontext(3) similarly to siglongjmp(3) and by removing this code 

> > you make the context not to be fully restored in that case (on the MIPS 

> > target the distinction is due to the DSP register state).

> > 

> >  What do you mean by: "setcontext is not async-signal-safe"?  I think such 

> > an assertion (if indeed true) should have been explained in the commit 

> > message even if it was previously discussed (where?).

> 

> It is in fact blurry on POSIX 2001, since it defines on '3. Definitions' [1] that

> function *is not* async-cancel-safe unless explicitly described as such and

> later on '2.4 Signal Concepts' [2] it defines the function set that should be

> async-signal-safe and it does not list 'setcontext' as one.


 Well, it says specifically that:

"[An async-cancel-safe function is] A function that may be invoked, 
without restriction, from signal-catching functions."

which in my opinion does not preclude a non-async-cancel-safe function to 
be called with some restrictions.  And the description of the function 
clearly states it is intended to be used in a signal handler.  So while we 
may not be required by the standard to produce a predictable result, we 
certainly are allowed to if feasible.

 NB `siglongjmp' isn't listed as an async-cancel-safe function either and 
I think we have no doubt as to whether it can be called in a signal 
handler and produce a predictable result.

> It is even more complicated because this is not portable over architectures on

> Linux, since:

> 

>     1. not every one supports rt_sigaction. It means that either that architecture

>        does not really require additional context that userland might not

>        save/restore or that architecture extensions save/restore are not really 

>        supported.

>        For instance, it seems that newer architectures (like RiscV) follow this

>        approach and just really support synchronous by saving/restore the preserved

>        state. It is the same with some newer ABI extension, such as AArch64 SVE.

> 

>     2. Not every architecture passes an mcontext_t and third argument for signal

>        handlers with SA_SIGINFO (sparc and ia64 for instance). 

> 

>     3. Even for architecture that does support passing an mcontext_t, userland

>        can not have enough information that some context is lazy-saved (such some

>        architecture extension such as vector register) by the kernel.

> 

> So that's why I think setcontext is not really aync-signal-safe and it is really

> hard to get it right in Linux (check powerpc complexity over time to support 

> Altivec and later VSX) and while it is support is very architecture-specific.  


 Perhaps Power would have to define and use a set of syscalls to aid the 
userland in selecting the set of registers to save and restore.  It was 
proposed for the MIPS/Linux port too, but it turned out unnecessary.

 I see no need or sense to remove what used to just work.

> I am not really against reverting the patch, and it might be better outcome for an 

> already deprecated symbol, but my idea was to make it somewhat more sane and 

> portable on Linux.


 In what sense portable?

  Maciej
Adhemerval Zanella June 24, 2019, 6:15 p.m. UTC | #6
On 24/06/2019 13:55, Maciej W. Rozycki wrote:
> On Mon, 3 Jun 2019, Adhemerval Zanella wrote:

> 

>>>  This is an obsolete interface, but FYI according to the relevant version 

>>> of SUS[1] you are supposed to be able to return from a signal handler by 

>>> calling setcontext(3) similarly to siglongjmp(3) and by removing this code 

>>> you make the context not to be fully restored in that case (on the MIPS 

>>> target the distinction is due to the DSP register state).

>>>

>>>  What do you mean by: "setcontext is not async-signal-safe"?  I think such 

>>> an assertion (if indeed true) should have been explained in the commit 

>>> message even if it was previously discussed (where?).

>>

>> It is in fact blurry on POSIX 2001, since it defines on '3. Definitions' [1] that

>> function *is not* async-cancel-safe unless explicitly described as such and

>> later on '2.4 Signal Concepts' [2] it defines the function set that should be

>> async-signal-safe and it does not list 'setcontext' as one.

> 

>  Well, it says specifically that:

> 

> "[An async-cancel-safe function is] A function that may be invoked, 

> without restriction, from signal-catching functions."

> 

> which in my opinion does not preclude a non-async-cancel-safe function to 

> be called with some restrictions.  And the description of the function 

> clearly states it is intended to be used in a signal handler.  So while we 

> may not be required by the standard to produce a predictable result, we 

> certainly are allowed to if feasible.


My understanding is an implementation detail that a function not required to
be a async-signal-safe might be safe to be called from a signal handler.
And the problem is exactly what is 'predictable' and ' feasible', since 
it requires some kernel support along with some out of the scope choices
(ABI boundary and which state is saved/restored). That's why I stated is
is 'blurry' on POSIX 2001 and if the symbols were not deprecated I think
it would require to open a defect report to Austin Group make it clear.

> 

>  NB `siglongjmp' isn't listed as an async-cancel-safe function either and 

> I think we have no doubt as to whether it can be called in a signal 

> handler and produce a predictable result.


My understanding is POSIX.1-2008 Technical Corrigendum 2 added both longjmp and 
siglongjmp to the list of async-signal-safe functions. And it also adds some
issues regarding its usage [1]:

"Note that although longjmp() and siglongjmp() are in the list of async-signal-safe 
functions, there are restrictions on subsequent behavior after the function is 
called from a signal-catching function. This is because the code executing after 
longjmp() or siglongjmp() can call any unsafe functions with the same danger as calling 
those unsafe functions directly from the signal handler. Applications that use longjmp() 
or siglongjmp() out of signal handlers require rigorous protection in order to be 
portable. Many of the other functions that are excluded from the list are traditionally
implemented using either the C language malloc() or free() functions or the ISO C standard
I/O library, both of which traditionally use data structures in a non-async-signal-safe 
manner. Because any combination of different functions using a common data structure
can cause async-signal-safety problems, POSIX.1 does not define the behavior when any
unsafe function is called in (or after a longjmp() or siglongjmp() out of) a signal handler 
that interrupts any unsafe function or the non-async-signal-safe processing equivalent 
to exit() that is performed after return from the initial call to main()."

Basically it is undefined if you call siglongjmp on a signal handler that has
interrupted a non-asignal-safe function.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html
 
> 

>> It is even more complicated because this is not portable over architectures on

>> Linux, since:

>>

>>     1. not every one supports rt_sigaction. It means that either that architecture

>>        does not really require additional context that userland might not

>>        save/restore or that architecture extensions save/restore are not really 

>>        supported.

>>        For instance, it seems that newer architectures (like RiscV) follow this

>>        approach and just really support synchronous by saving/restore the preserved

>>        state. It is the same with some newer ABI extension, such as AArch64 SVE.

>>

>>     2. Not every architecture passes an mcontext_t and third argument for signal

>>        handlers with SA_SIGINFO (sparc and ia64 for instance). 

>>

>>     3. Even for architecture that does support passing an mcontext_t, userland

>>        can not have enough information that some context is lazy-saved (such some

>>        architecture extension such as vector register) by the kernel.

>>

>> So that's why I think setcontext is not really aync-signal-safe and it is really

>> hard to get it right in Linux (check powerpc complexity over time to support 

>> Altivec and later VSX) and while it is support is very architecture-specific.  

> 

>  Perhaps Power would have to define and use a set of syscalls to aid the 

> userland in selecting the set of registers to save and restore.  It was 

> proposed for the MIPS/Linux port too, but it turned out unnecessary.


POWER does that setting the uc_mcontext.gp_regs[PT_MSR] with expected state
(MSR_VEC, MSR_VSX).

> 

>  I see no need or sense to remove what used to just work.

> 

>> I am not really against reverting the patch, and it might be better outcome for an 

>> already deprecated symbol, but my idea was to make it somewhat more sane and 

>> portable on Linux.

> 

>  In what sense portable?


Besides the issues POSIX itself alerts by using setcontext/sig*jmp in a signal
handler, also the fact most architecture only defines a rational subset of the
CPU state is set/restore instead of trying to support all possible extensions
(which most likely requires kernel support). So, for instance an application that
use some vector extension might add arch-specific handle to either check or
not expect the fully state to be set/restored.

> 

>   Maciej

>
Florian Weimer June 25, 2019, 12:24 p.m. UTC | #7
* Adhemerval Zanella:

> My understanding is an implementation detail that a function not

> required to be a async-signal-safe might be safe to be called from a

> signal handler.


This is my understanding as well, but the glibc project as a whole
clearly does not share this view.  The manual documents AS-safety based
on the state of the implementation a while back.  We even document *why*
an interface is not AS-safe, so presumably applications which do not
care about certain deadlocks (because they take measures that they
cannot happen) can still use AS-unsafe functions in signal handlers.

The downside is that a programmer reading the manual cannot know if a
function marked as AS-safe is so because it's a supported property of
the interface, or an emergent aspect of the implementation at the time
of the last review.

I also think it's wrong to document the extent to which AS-unsafe
functions can be called in signal handlers.  It's undefined behavior,
plain and simple.

Thanks,
Florian
Adhemerval Zanella June 25, 2019, 12:35 p.m. UTC | #8
On 25/06/2019 09:24, Florian Weimer wrote:
> * Adhemerval Zanella:

> 

>> My understanding is an implementation detail that a function not

>> required to be a async-signal-safe might be safe to be called from a

>> signal handler.

> 

> This is my understanding as well, but the glibc project as a whole

> clearly does not share this view.  The manual documents AS-safety based

> on the state of the implementation a while back.  We even document *why*

> an interface is not AS-safe, so presumably applications which do not

> care about certain deadlocks (because they take measures that they

> cannot happen) can still use AS-unsafe functions in signal handlers.

> 

> The downside is that a programmer reading the manual cannot know if a

> function marked as AS-safe is so because it's a supported property of

> the interface, or an emergent aspect of the implementation at the time

> of the last review.


Some documentation specifics emerge from implementation detail, so I
see that this does not really clash with my understanding. Maybe we should
add that although glibc implementation is AS-safe, this is defined by
standard and non-portable (something like what gnulib does).

> 

> I also think it's wrong to document the extent to which AS-unsafe

> functions can be called in signal handlers.  It's undefined behavior,

> plain and simple.




> 

> Thanks,

> Florian

>
Maciej W. Rozycki June 25, 2019, 6:12 p.m. UTC | #9
On Mon, 24 Jun 2019, Adhemerval Zanella wrote:

> >  Well, it says specifically that:

> > 

> > "[An async-cancel-safe function is] A function that may be invoked, 

> > without restriction, from signal-catching functions."

> > 

> > which in my opinion does not preclude a non-async-cancel-safe function to 

> > be called with some restrictions.  And the description of the function 

> > clearly states it is intended to be used in a signal handler.  So while we 

> > may not be required by the standard to produce a predictable result, we 

> > certainly are allowed to if feasible.

> 

> My understanding is an implementation detail that a function not required to

> be a async-signal-safe might be safe to be called from a signal handler.

> And the problem is exactly what is 'predictable' and ' feasible', since 

> it requires some kernel support along with some out of the scope choices

> (ABI boundary and which state is saved/restored). That's why I stated is

> is 'blurry' on POSIX 2001 and if the symbols were not deprecated I think

> it would require to open a defect report to Austin Group make it clear.


 Agreed as to the defect report.  The situation WRT the kernel is not 
really any different from POSIX threads support however, as they also 
require assistance from the underlying OS kernel.

> >  NB `siglongjmp' isn't listed as an async-cancel-safe function either and 

> > I think we have no doubt as to whether it can be called in a signal 

> > handler and produce a predictable result.

> 

> My understanding is POSIX.1-2008 Technical Corrigendum 2 added both longjmp and 

> siglongjmp to the list of async-signal-safe functions. And it also adds some

> issues regarding its usage [1]:

> 

> "Note that although longjmp() and siglongjmp() are in the list of async-signal-safe 

> functions, there are restrictions on subsequent behavior after the function is 

> called from a signal-catching function. This is because the code executing after 

> longjmp() or siglongjmp() can call any unsafe functions with the same danger as calling 

> those unsafe functions directly from the signal handler. Applications that use longjmp() 

> or siglongjmp() out of signal handlers require rigorous protection in order to be 

> portable. Many of the other functions that are excluded from the list are traditionally

> implemented using either the C language malloc() or free() functions or the ISO C standard

> I/O library, both of which traditionally use data structures in a non-async-signal-safe 

> manner. Because any combination of different functions using a common data structure

> can cause async-signal-safety problems, POSIX.1 does not define the behavior when any

> unsafe function is called in (or after a longjmp() or siglongjmp() out of) a signal handler 

> that interrupts any unsafe function or the non-async-signal-safe processing equivalent 

> to exit() that is performed after return from the initial call to main()."

> 

> Basically it is undefined if you call siglongjmp on a signal handler that has

> interrupted a non-asignal-safe function.

> 

> [1] https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html


 At which point the `*context' functions have already been obsoleted and 
hence nobody bothered to do anything about them, or I presume the same 
concerns would apply to `setcontext' and `swapcontext' as do to 
`siglongjmp'.

> >> I am not really against reverting the patch, and it might be better outcome for an 

> >> already deprecated symbol, but my idea was to make it somewhat more sane and 

> >> portable on Linux.

> > 

> >  In what sense portable?

> 

> Besides the issues POSIX itself alerts by using setcontext/sig*jmp in a signal

> handler, also the fact most architecture only defines a rational subset of the

> CPU state is set/restore instead of trying to support all possible extensions

> (which most likely requires kernel support). So, for instance an application that

> use some vector extension might add arch-specific handle to either check or

> not expect the fully state to be set/restored.


 About the only non-trivial sensible use I can envisage for the `*context' 
functions is to have an in-process multitasking execution environment 
where multiple contexts of execution are switched between in a scheduler 
based around SIGALRM or a similar signal arranged to be raised at regular 
intervals (a poor man's threads implementation).  For this to work at all 
obviously all the user registers have to be saved and restored as the 
scheduling signal can trigger at any time with respect to linear code 
execution, and the relevant functions, especially `swapcontext', have to 
be invocable from within a signal handler.

 While I agree we are not bound by the standard (anymore) to implement 
this feature for supported architectures we have not before, I believe the 
MIPS implementation used to fulfil the purpose for the register sets 
defined at the time (i.e. the extended FP context of the later MSA ASE is 
not supported) and as I say I see no reason to remove it.  It remains up 
to the user of the API to avoid the use of non-async-signal-safe 
functions, including `malloc' and `free' in particular.

 I had a small test case included with the original submission of MIPS 
`*context' support that covered the use of `*context' functions in a 
signal handler, which however was dropped as the change was applied.  I 
meant to write a more proper one with a signal-based scheduler as outlined 
above cycling across a few contexts a few times, however I ran out of time 
and never got back to it.

  Maciej
Adhemerval Zanella June 26, 2019, 7:40 p.m. UTC | #10
On 25/06/2019 15:12, Maciej W. Rozycki wrote:
> On Mon, 24 Jun 2019, Adhemerval Zanella wrote:

> 

>>>  Well, it says specifically that:

>>>

>>> "[An async-cancel-safe function is] A function that may be invoked, 

>>> without restriction, from signal-catching functions."

>>>

>>> which in my opinion does not preclude a non-async-cancel-safe function to 

>>> be called with some restrictions.  And the description of the function 

>>> clearly states it is intended to be used in a signal handler.  So while we 

>>> may not be required by the standard to produce a predictable result, we 

>>> certainly are allowed to if feasible.

>>

>> My understanding is an implementation detail that a function not required to

>> be a async-signal-safe might be safe to be called from a signal handler.

>> And the problem is exactly what is 'predictable' and ' feasible', since 

>> it requires some kernel support along with some out of the scope choices

>> (ABI boundary and which state is saved/restored). That's why I stated is

>> is 'blurry' on POSIX 2001 and if the symbols were not deprecated I think

>> it would require to open a defect report to Austin Group make it clear.

> 

>  Agreed as to the defect report.  The situation WRT the kernel is not 

> really any different from POSIX threads support however, as they also 

> require assistance from the underlying OS kernel.


Yes, but at least Linux went on safe way to handle threads a different
process regarding state save/restore. It means it is up to kernel to
correctly handle the required state and allows some clever optimization
like lazy-save cpu extensions, such as floating-point or vector register.

> 

>>>  NB `siglongjmp' isn't listed as an async-cancel-safe function either and 

>>> I think we have no doubt as to whether it can be called in a signal 

>>> handler and produce a predictable result.

>>

>> My understanding is POSIX.1-2008 Technical Corrigendum 2 added both longjmp and 

>> siglongjmp to the list of async-signal-safe functions. And it also adds some

>> issues regarding its usage [1]:

>>

>> "Note that although longjmp() and siglongjmp() are in the list of async-signal-safe 

>> functions, there are restrictions on subsequent behavior after the function is 

>> called from a signal-catching function. This is because the code executing after 

>> longjmp() or siglongjmp() can call any unsafe functions with the same danger as calling 

>> those unsafe functions directly from the signal handler. Applications that use longjmp() 

>> or siglongjmp() out of signal handlers require rigorous protection in order to be 

>> portable. Many of the other functions that are excluded from the list are traditionally

>> implemented using either the C language malloc() or free() functions or the ISO C standard

>> I/O library, both of which traditionally use data structures in a non-async-signal-safe 

>> manner. Because any combination of different functions using a common data structure

>> can cause async-signal-safety problems, POSIX.1 does not define the behavior when any

>> unsafe function is called in (or after a longjmp() or siglongjmp() out of) a signal handler 

>> that interrupts any unsafe function or the non-async-signal-safe processing equivalent 

>> to exit() that is performed after return from the initial call to main()."

>>

>> Basically it is undefined if you call siglongjmp on a signal handler that has

>> interrupted a non-asignal-safe function.

>>

>> [1] https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html

> 

>  At which point the `*context' functions have already been obsoleted and 

> hence nobody bothered to do anything about them, or I presume the same 

> concerns would apply to `setcontext' and `swapcontext' as do to 

> `siglongjmp'.

> 

>>>> I am not really against reverting the patch, and it might be better outcome for an 

>>>> already deprecated symbol, but my idea was to make it somewhat more sane and 

>>>> portable on Linux.

>>>

>>>  In what sense portable?

>>

>> Besides the issues POSIX itself alerts by using setcontext/sig*jmp in a signal

>> handler, also the fact most architecture only defines a rational subset of the

>> CPU state is set/restore instead of trying to support all possible extensions

>> (which most likely requires kernel support). So, for instance an application that

>> use some vector extension might add arch-specific handle to either check or

>> not expect the fully state to be set/restored.

> 

>  About the only non-trivial sensible use I can envisage for the `*context' 

> functions is to have an in-process multitasking execution environment 

> where multiple contexts of execution are switched between in a scheduler 

> based around SIGALRM or a similar signal arranged to be raised at regular 

> intervals (a poor man's threads implementation).  For this to work at all 

> obviously all the user registers have to be saved and restored as the 

> scheduling signal can trigger at any time with respect to linear code 

> execution, and the relevant functions, especially `swapcontext', have to 

> be invocable from within a signal handler.

> 

>  While I agree we are not bound by the standard (anymore) to implement 

> this feature for supported architectures we have not before, I believe the 

> MIPS implementation used to fulfil the purpose for the register sets 

> defined at the time (i.e. the extended FP context of the later MSA ASE is 

> not supported) and as I say I see no reason to remove it.  It remains up 

> to the user of the API to avoid the use of non-async-signal-safe 

> functions, including `malloc' and `free' in particular.


My understanding is this scenario only safe if for every non as-safe call
you actually unmask/mask all signals, and even though my previous remarks
still hold that this is not really portable in the sense that depending
of the computation you won't get reliable results over the architectures
(it might depend of the compiler flags or compiler that might generate
extension usage that might not be saved/restored).

Not sure how this scenario is used in real-world cases and if the computation
involved does require to correct save/restore all possible state.

> 

>  I had a small test case included with the original submission of MIPS 

> `*context' support that covered the use of `*context' functions in a 

> signal handler, which however was dropped as the change was applied.  I 

> meant to write a more proper one with a signal-based scheduler as outlined 

> above cycling across a few contexts a few times, however I ran out of time 

> and never got back to it.


The main problem now is with compiler aggressively using more architecture
extension and with *context definition not being correctly defined, it support
is really arch-specific. And I think we should move away of such behaviour,
unless it is really the intention (such ABI arch-specific ABIs or libraries
that leverages some extension such as libmvec).
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/mips/getcontext.S b/sysdeps/unix/sysv/linux/mips/getcontext.S
index 4f7f89ee9a..015bd5bff6 100644
--- a/sysdeps/unix/sysv/linux/mips/getcontext.S
+++ b/sysdeps/unix/sysv/linux/mips/getcontext.S
@@ -78,11 +78,6 @@  NESTED (__getcontext, FRAMESZ, ra)
 	.set	at
 #endif
 
-	/* Store a magic flag.	*/
-	li	v1, 1
-	/* zero */
-	REG_S	v1, (MCONTEXT_GREGOFF + 0 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)
-
 	REG_S	s0, (MCONTEXT_GREGOFF + 16 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)
 	REG_S	s1, (MCONTEXT_GREGOFF + 17 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)
 	REG_S	s2, (MCONTEXT_GREGOFF + 18 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)
diff --git a/sysdeps/unix/sysv/linux/mips/makecontext.S b/sysdeps/unix/sysv/linux/mips/makecontext.S
index 4439fec3ff..3f40f7c9f2 100644
--- a/sysdeps/unix/sysv/linux/mips/makecontext.S
+++ b/sysdeps/unix/sysv/linux/mips/makecontext.S
@@ -93,11 +93,6 @@  NESTED (__makecontext, FRAMESZ, ra)
 	REG_S	a7, A7OFF(sp)
 #endif
 
-	/* Store a magic flag.  */
-	li	v1, 1
-	/* zero */
-	REG_S	v1, (MCONTEXT_GREGOFF + 0 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)
-
 	/* Set up the stack.  */
 	PTR_L	t0, STACK_SP(a0)
 	PTR_L	t2, STACK_SIZE(a0)
diff --git a/sysdeps/unix/sysv/linux/mips/setcontext.S b/sysdeps/unix/sysv/linux/mips/setcontext.S
index b6553bdb5e..98afe6cbac 100644
--- a/sysdeps/unix/sysv/linux/mips/setcontext.S
+++ b/sysdeps/unix/sysv/linux/mips/setcontext.S
@@ -77,12 +77,6 @@  NESTED (__setcontext, FRAMESZ, ra)
 	.set	at
 #endif
 
-	/* Check for the magic flag.  */
-	li	v0, 1
-	/* zero */
-	REG_L	v1, (MCONTEXT_GREGOFF + 0 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)
-	bne	v0, v1, 98f
-
 	REG_S	a0, A0OFF(sp)
 
 /* rt_sigprocmask (SIG_SETMASK, &ucp->uc_sigmask, NULL, _NSIG8) */
@@ -154,38 +148,6 @@  NESTED (__setcontext, FRAMESZ, ra)
 	move	v0, zero
 	jr	t9
 
-98:
-	/* This is a context obtained from a signal handler.
-	   Perform a full restore by pushing the context
-	   passed onto a simulated signal frame on the stack
-	   and call the signal return syscall as if a signal
-	   handler exited normally.  */
-	PTR_ADDIU sp, -((RT_SIGFRAME_SIZE + ALSZ) & ALMASK)
-	cfi_adjust_cfa_offset ((RT_SIGFRAME_SIZE + ALSZ) & ALMASK)
-
-	/* Only ucontext is referred to from rt_sigreturn,
-	   copy it.  */
-	PTR_ADDIU t1, sp, RT_SIGFRAME_UCONTEXT
-	li	t3, ((UCONTEXT_SIZE + SZREG - 1) / SZREG) - 1
-0:
-	REG_L	t2, (a0)
-	PTR_ADDIU a0, SZREG
-	REG_S	t2, (t1)
-	PTR_ADDIU t1, SZREG
-	.set	noreorder
-	bgtz	t3, 0b
-	 addiu	t3, -1
-	.set	reorder
-
-/* rt_sigreturn () -- no arguments, sp points to struct rt_sigframe.  */
-	li	v0, SYS_ify (rt_sigreturn)
-	syscall
-
-	/* Restore the stack and fall through to the error
-	   path.  Successful rt_sigreturn never returns to
-	   its calling place.  */
-	PTR_ADDIU sp, ((RT_SIGFRAME_SIZE + ALSZ) & ALMASK)
-	cfi_adjust_cfa_offset (-((RT_SIGFRAME_SIZE + ALSZ) & ALMASK))
 99:
 #ifdef __PIC__
 	PTR_LA	t9, JUMPTARGET (__syscall_error)
diff --git a/sysdeps/unix/sysv/linux/mips/swapcontext.S b/sysdeps/unix/sysv/linux/mips/swapcontext.S
index 9c68961345..6612e757cd 100644
--- a/sysdeps/unix/sysv/linux/mips/swapcontext.S
+++ b/sysdeps/unix/sysv/linux/mips/swapcontext.S
@@ -87,11 +87,6 @@  NESTED (__swapcontext, FRAMESZ, ra)
 	.set	at
 #endif
 
-	/* Store a magic flag.	*/
-	li	v1, 1
-	/* zero */
-	REG_S	v1, (MCONTEXT_GREGOFF + 0 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)
-
 	REG_S	s0, (MCONTEXT_GREGOFF + 16 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)
 	REG_S	s1, (MCONTEXT_GREGOFF + 17 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)
 	REG_S	s2, (MCONTEXT_GREGOFF + 18 * MCONTEXT_GREGSZ + MCONTEXT_GREGS)(a0)