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

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

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.