diff mbox

[6/6] Cleanup sigpause implementation

Message ID 1509745249-11404-6-git-send-email-adhemerval.zanella@linaro.org
State Committed
Commit ad4f43a2344864ae2304060dcc722a7a63bad1b4
Headers

Commit Message

Adhemerval Zanella Netto Nov. 3, 2017, 9:40 p.m. UTC 
  This patch simplify sigpause by remobing the single thread optimization
since it will be handled already by the __sigsuspend call.

Checked on x86_64-linux-gnu.

	* sysdeps/posix/sigpause.c (do_sigpause): Remove.
	(__sigpause): Rely on __sigsuspend to implement single thread
	optimization.

Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
---
 ChangeLog                |  4 ++++
 sysdeps/posix/sigpause.c | 25 +++++--------------------
 2 files changed, 9 insertions(+), 20 deletions(-)

Comments

Zack Weinberg Nov. 5, 2017, 9:27 p.m. UTC | #1 
On Fri, Nov 3, 2017 at 5:40 PM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> This patch simplify sigpause by remobing the single thread optimization
> since it will be handled already by the __sigsuspend call.
>
> Checked on x86_64-linux-gnu.
>
>         * sysdeps/posix/sigpause.c (do_sigpause): Remove.
>         (__sigpause): Rely on __sigsuspend to implement single thread
>         optimization.

LGTM except that the addition of LIBC_CANCEL_HANDLED sent me down a
rabbit hole - I might have to write a replacement for
tst-cancel-wrappers.sh that doesn't depend on these magic markers -
anyway, please mention in the ChangeLog entry that you added this
annotation, and put /* __sigsuspend handles cancellation */
immediately above the LIBC_CANCEL_HANDLED line, following the pattern
of all the other uses of LIBC_CANCEL_HANDLED; that'll make things at
least a _little_ easier for future archaeologists.

zw
Adhemerval Zanella Netto Nov. 6, 2017, 11:09 a.m. UTC | #2 
On 05/11/2017 19:27, Zack Weinberg wrote:
> On Fri, Nov 3, 2017 at 5:40 PM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>> This patch simplify sigpause by remobing the single thread optimization
>> since it will be handled already by the __sigsuspend call.
>>
>> Checked on x86_64-linux-gnu.
>>
>>         * sysdeps/posix/sigpause.c (do_sigpause): Remove.
>>         (__sigpause): Rely on __sigsuspend to implement single thread
>>         optimization.
> 
> LGTM except that the addition of LIBC_CANCEL_HANDLED sent me down a
> rabbit hole - I might have to write a replacement for
> tst-cancel-wrappers.sh that doesn't depend on these magic markers -
> anyway, please mention in the ChangeLog entry that you added this
> annotation, and put /* __sigsuspend handles cancellation */
> immediately above the LIBC_CANCEL_HANDLED line, following the pattern
> of all the other uses of LIBC_CANCEL_HANDLED; that'll make things at
> least a _little_ easier for future archaeologists.
> 
> zw
> 

In fact with my cancellation refactor patch my idea is just remove this
fragile test which require this extra marks (since now we have now 
coverage for pretty much all symbols with runtime cancellation tests).
Zack Weinberg Nov. 6, 2017, 2:08 p.m. UTC | #3 
On Mon, Nov 6, 2017 at 6:09 AM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> On 05/11/2017 19:27, Zack Weinberg wrote:
>> - I might have to write a replacement for
>> tst-cancel-wrappers.sh that doesn't depend on these magic markers -
>
> In fact with my cancellation refactor patch my idea is just remove this
> fragile test which require this extra marks (since now we have now
> coverage for pretty much all symbols with runtime cancellation tests).

I'm glad to hear this is already being addressed :)  I haven't looked
at your cancellation refactor patches because I don't know the guts of
NPTL very well, unfortunately.

zw
diff mbox

Patch

diff --git a/sysdeps/posix/sigpause.c b/sysdeps/posix/sigpause.c
index 9038ed3..706195c 100644
--- a/sysdeps/posix/sigpause.c
+++ b/sysdeps/posix/sigpause.c
@@ -19,15 +19,13 @@ 
 #include <errno.h>
 #include <signal.h>
 #include <stddef.h>		/* For NULL.  */
-#include <sysdep-cancel.h>
 #undef sigpause
 
 #include <sigset-cvt-mask.h>
+#include <sysdep-cancel.h>
 
-/* Set the mask of blocked signals to MASK,
-   wait for a signal to arrive, and then restore the mask.  */
-static int
-do_sigpause (int sig_or_mask, int is_sig)
+int
+__sigpause (int sig_or_mask, int is_sig)
 {
   sigset_t set;
 
@@ -46,21 +44,6 @@  do_sigpause (int sig_or_mask, int is_sig)
      to do anything here.  */
   return __sigsuspend (&set);
 }
-
-int
-__sigpause (int sig_or_mask, int is_sig)
-{
-  if (SINGLE_THREAD_P)
-    return do_sigpause (sig_or_mask, is_sig);
-
-  int oldtype = LIBC_CANCEL_ASYNC ();
-
-  int result = do_sigpause (sig_or_mask, is_sig);
-
-  LIBC_CANCEL_RESET (oldtype);
-
-  return result;
-}
 libc_hidden_def (__sigpause)
 
 /* We have to provide a default version of this function since the
@@ -87,3 +70,5 @@  __xpg_sigpause (int sig)
   return __sigpause (sig, 1);
 }
 strong_alias (__xpg_sigpause, __libc___xpg_sigpause)
+
+LIBC_CANCEL_HANDLED ();