[3/8] nptl: Remove always-disabled debugging support

Message ID 313425893d8419592a17bdad60ce2bf37d485b3f.1620650045.git.fweimer@redhat.com
State New
Headers show
Series
  • nptl: Move pthread_detach and pthread_join into libc
Related show

Commit Message

Adhemerval Zanella via Libc-alpha May 10, 2021, 12:37 p.m.
This removes the DEBUGGING_P macro and the __pthread_debug variable.
The __find_in_stack_list function is now unused and deleted as well.
---
 nptl/pthreadP.h         | 26 +++++-----------------
 nptl/pthread_create.c   | 49 -----------------------------------------
 nptl/pthread_sigqueue.c |  5 -----
 3 files changed, 5 insertions(+), 75 deletions(-)

-- 
2.31.1

Comments

Adhemerval Zanella via Libc-alpha May 10, 2021, 1:47 p.m. | #1
On 10/05/2021 09:37, Florian Weimer via Libc-alpha wrote:
> This removes the DEBUGGING_P macro and the __pthread_debug variable.

> The __find_in_stack_list function is now unused and deleted as well.


LGTM, thanks.  As a side note we might revise the INVALID_TD_P and 
INVALID_NOT_TERMINATED_TD_P in the future, I don't their usage are
entirely safe within glibc.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>


> ---

>  nptl/pthreadP.h         | 26 +++++-----------------

>  nptl/pthread_create.c   | 49 -----------------------------------------

>  nptl/pthread_sigqueue.c |  5 -----

>  3 files changed, 5 insertions(+), 75 deletions(-)

> 

> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h

> index d9b97c814a..d9a6137bd3 100644

> --- a/nptl/pthreadP.h

> +++ b/nptl/pthreadP.h

> @@ -243,23 +243,11 @@ libc_hidden_proto (__pthread_tpp_change_priority)

>  extern int __pthread_current_priority (void);

>  libc_hidden_proto (__pthread_current_priority)

>  

> -/* The library can run in debugging mode where it performs a lot more

> -   tests.  */

> -extern int __pthread_debug attribute_hidden;

> -/** For now disable debugging support.  */

> -#if 0

> -# define DEBUGGING_P __builtin_expect (__pthread_debug, 0)

> -# define INVALID_TD_P(pd) (DEBUGGING_P && __find_in_stack_list (pd) == NULL)

> -# define INVALID_NOT_TERMINATED_TD_P(pd) INVALID_TD_P (pd)

> -#else

> -# define DEBUGGING_P 0

> -/* Simplified test.  This will not catch all invalid descriptors but

> -   is better than nothing.  And if the test triggers the thread

> -   descriptor is guaranteed to be invalid.  */

> -# define INVALID_TD_P(pd) __builtin_expect ((pd)->tid <= 0, 0)

> -# define INVALID_NOT_TERMINATED_TD_P(pd) __builtin_expect ((pd)->tid < 0, 0)

> -#endif

> -

> +/* This will not catch all invalid descriptors but is better than

> +   nothing.  And if the test triggers the thread descriptor is

> +   guaranteed to be invalid.  */

> +#define INVALID_TD_P(pd) __builtin_expect ((pd)->tid <= 0, 0)

> +#define INVALID_NOT_TERMINATED_TD_P(pd) __builtin_expect ((pd)->tid < 0, 0)

>  

>  /* Cancellation test.  */

>  #define CANCELLATION_P(self) \

> @@ -322,10 +310,6 @@ __do_cancel (void)

>  

>  /* Internal prototypes.  */

>  

> -/* Thread list handling.  */

> -extern struct pthread *__find_in_stack_list (struct pthread *pd)

> -     attribute_hidden;

> -

>  /* Deallocate a thread's stack after optionally making sure the thread

>     descriptor is still valid.  */

>  extern void __free_tcb (struct pthread *pd) attribute_hidden;


Ok.

> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c

> index 775287d0e4..d19456d48b 100644

> --- a/nptl/pthread_create.c

> +++ b/nptl/pthread_create.c

> @@ -42,9 +42,6 @@

>  #include <stap-probe.h>

>  

>  

> -/* Nozero if debugging mode is enabled.  */

> -int __pthread_debug;

> -

>  /* Globally enabled events.  */

>  static td_thr_events_t __nptl_threads_events __attribute_used__;

>  

> @@ -210,46 +207,6 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,

>  

>  #include <createthread.c>

>  

> -

> -struct pthread *

> -__find_in_stack_list (struct pthread *pd)

> -{

> -  list_t *entry;

> -  struct pthread *result = NULL;

> -

> -  lll_lock (GL (dl_stack_cache_lock), LLL_PRIVATE);

> -

> -  list_for_each (entry, &GL (dl_stack_used))

> -    {

> -      struct pthread *curp;

> -

> -      curp = list_entry (entry, struct pthread, list);

> -      if (curp == pd)

> -	{

> -	  result = curp;

> -	  break;

> -	}

> -    }

> -

> -  if (result == NULL)

> -    list_for_each (entry, &GL (dl_stack_user))

> -      {

> -	struct pthread *curp;

> -

> -	curp = list_entry (entry, struct pthread, list);

> -	if (curp == pd)

> -	  {

> -	    result = curp;

> -	    break;

> -	  }

> -      }

> -

> -  lll_unlock (GL (dl_stack_cache_lock), LLL_PRIVATE);

> -

> -  return result;

> -}

> -

> -

>  /* Deallocate a thread's stack after optionally making sure the thread

>     descriptor is still valid.  */

>  void


Ok.

> @@ -259,12 +216,6 @@ __free_tcb (struct pthread *pd)

>    if (__builtin_expect (atomic_bit_test_set (&pd->cancelhandling,

>  					     TERMINATED_BIT) == 0, 1))

>      {

> -      /* Remove the descriptor from the list.  */

> -      if (DEBUGGING_P && __find_in_stack_list (pd) == NULL)

> -	/* Something is really wrong.  The descriptor for a still

> -	   running thread is gone.  */

> -	abort ();

> -

>        /* Free TPP data.  */

>        if (__glibc_unlikely (pd->tpp != NULL))

>  	{


Ok.

> diff --git a/nptl/pthread_sigqueue.c b/nptl/pthread_sigqueue.c

> index 64bacfe41b..3ffb595489 100644

> --- a/nptl/pthread_sigqueue.c

> +++ b/nptl/pthread_sigqueue.c

> @@ -31,11 +31,6 @@ pthread_sigqueue (pthread_t threadid, int signo, const union sigval value)

>  #ifdef __NR_rt_tgsigqueueinfo

>    struct pthread *pd = (struct pthread *) threadid;

>  

> -  /* Make sure the descriptor is valid.  */

> -  if (DEBUGGING_P && INVALID_TD_P (pd))

> -    /* Not a valid thread handle.  */

> -    return ESRCH;

> -

>    /* Force load of pd->tid into local variable or register.  Otherwise

>       if a thread exits between ESRCH test and tgkill, we might return

>       EINVAL, because pd->tid would be cleared by the kernel.  */

> 


Ok.

Patch

diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index d9b97c814a..d9a6137bd3 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -243,23 +243,11 @@  libc_hidden_proto (__pthread_tpp_change_priority)
 extern int __pthread_current_priority (void);
 libc_hidden_proto (__pthread_current_priority)
 
-/* The library can run in debugging mode where it performs a lot more
-   tests.  */
-extern int __pthread_debug attribute_hidden;
-/** For now disable debugging support.  */
-#if 0
-# define DEBUGGING_P __builtin_expect (__pthread_debug, 0)
-# define INVALID_TD_P(pd) (DEBUGGING_P && __find_in_stack_list (pd) == NULL)
-# define INVALID_NOT_TERMINATED_TD_P(pd) INVALID_TD_P (pd)
-#else
-# define DEBUGGING_P 0
-/* Simplified test.  This will not catch all invalid descriptors but
-   is better than nothing.  And if the test triggers the thread
-   descriptor is guaranteed to be invalid.  */
-# define INVALID_TD_P(pd) __builtin_expect ((pd)->tid <= 0, 0)
-# define INVALID_NOT_TERMINATED_TD_P(pd) __builtin_expect ((pd)->tid < 0, 0)
-#endif
-
+/* This will not catch all invalid descriptors but is better than
+   nothing.  And if the test triggers the thread descriptor is
+   guaranteed to be invalid.  */
+#define INVALID_TD_P(pd) __builtin_expect ((pd)->tid <= 0, 0)
+#define INVALID_NOT_TERMINATED_TD_P(pd) __builtin_expect ((pd)->tid < 0, 0)
 
 /* Cancellation test.  */
 #define CANCELLATION_P(self) \
@@ -322,10 +310,6 @@  __do_cancel (void)
 
 /* Internal prototypes.  */
 
-/* Thread list handling.  */
-extern struct pthread *__find_in_stack_list (struct pthread *pd)
-     attribute_hidden;
-
 /* Deallocate a thread's stack after optionally making sure the thread
    descriptor is still valid.  */
 extern void __free_tcb (struct pthread *pd) attribute_hidden;
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 775287d0e4..d19456d48b 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -42,9 +42,6 @@ 
 #include <stap-probe.h>
 
 
-/* Nozero if debugging mode is enabled.  */
-int __pthread_debug;
-
 /* Globally enabled events.  */
 static td_thr_events_t __nptl_threads_events __attribute_used__;
 
@@ -210,46 +207,6 @@  static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
 
 #include <createthread.c>
 
-
-struct pthread *
-__find_in_stack_list (struct pthread *pd)
-{
-  list_t *entry;
-  struct pthread *result = NULL;
-
-  lll_lock (GL (dl_stack_cache_lock), LLL_PRIVATE);
-
-  list_for_each (entry, &GL (dl_stack_used))
-    {
-      struct pthread *curp;
-
-      curp = list_entry (entry, struct pthread, list);
-      if (curp == pd)
-	{
-	  result = curp;
-	  break;
-	}
-    }
-
-  if (result == NULL)
-    list_for_each (entry, &GL (dl_stack_user))
-      {
-	struct pthread *curp;
-
-	curp = list_entry (entry, struct pthread, list);
-	if (curp == pd)
-	  {
-	    result = curp;
-	    break;
-	  }
-      }
-
-  lll_unlock (GL (dl_stack_cache_lock), LLL_PRIVATE);
-
-  return result;
-}
-
-
 /* Deallocate a thread's stack after optionally making sure the thread
    descriptor is still valid.  */
 void
@@ -259,12 +216,6 @@  __free_tcb (struct pthread *pd)
   if (__builtin_expect (atomic_bit_test_set (&pd->cancelhandling,
 					     TERMINATED_BIT) == 0, 1))
     {
-      /* Remove the descriptor from the list.  */
-      if (DEBUGGING_P && __find_in_stack_list (pd) == NULL)
-	/* Something is really wrong.  The descriptor for a still
-	   running thread is gone.  */
-	abort ();
-
       /* Free TPP data.  */
       if (__glibc_unlikely (pd->tpp != NULL))
 	{
diff --git a/nptl/pthread_sigqueue.c b/nptl/pthread_sigqueue.c
index 64bacfe41b..3ffb595489 100644
--- a/nptl/pthread_sigqueue.c
+++ b/nptl/pthread_sigqueue.c
@@ -31,11 +31,6 @@  pthread_sigqueue (pthread_t threadid, int signo, const union sigval value)
 #ifdef __NR_rt_tgsigqueueinfo
   struct pthread *pd = (struct pthread *) threadid;
 
-  /* Make sure the descriptor is valid.  */
-  if (DEBUGGING_P && INVALID_TD_P (pd))
-    /* Not a valid thread handle.  */
-    return ESRCH;
-
   /* Force load of pd->tid into local variable or register.  Otherwise
      if a thread exits between ESRCH test and tgkill, we might return
      EINVAL, because pd->tid would be cleared by the kernel.  */