[PATCH 3/4] elf: Use TLS_DTV_OFFSET in __tls_get_addr

[Adhemerval Zanella Netto]

On 27/12/24 12:37, Florian Weimer wrote:
> This fixes commit 5e249192cac7354af02a7347a0d8c984e0c88ed3 ("elf:
> Remove the GET_ADDR_ARGS and related macros from the TLS code"):
> GET_ADDR_ARGS was indeed unused, but GET_ADDR_OFFSET was used
> on several targets, those that define TLS_DTV_OFFSET.  Instead
> of reintroducing GET_ADDR_OFFSET, use TLS_DTV_OFFSET directly,
> now that it is defined on all targets.
> 
> In the new tls_get_addr_adjust helper function, add a cast to
> uintptr_t to help the s390 case, where the offset can be positive or
> negative, depending on the addresses malloc returns.  The cast avoids
> pointer wraparound/overflow.  The outer uintptr_t cast is needed
> to suppress a warning on x86-64 x32 about mismatched integer/pointer
> sizes.
> 
> Eventually this offset should be folded into the DTV addresses
> themselves, to eliminate the subtraction on the TLS fast path.
> This will require an adjustment to libthread_db because the
> debugger interface currently returns unadjusted pointers.

LGTM, thanks.

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

> ---
>  elf/dl-tls.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/elf/dl-tls.c b/elf/dl-tls.c
> index 4df83b9bb2..5f3c03edf8 100644
> --- a/elf/dl-tls.c
> +++ b/elf/dl-tls.c
> @@ -890,6 +890,14 @@ _dl_update_slotinfo (unsigned long int req_modid, size_t new_gen)
>    return the_map;
>  }
>  
> +/* Adjust the TLS variable pointer using the TLS descriptor offset and
> +   the ABI-specific offset.  */
> +static inline void *
> +tls_get_addr_adjust (void *from_dtv, tls_index *ti)
> +{
> +  return (void *) (uintptr_t) ((uintptr_t) from_dtv + ti->ti_offset
> +			       + TLS_DTV_OFFSET);
> +}

On sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h the code explicitly disables the
warning to zero-extend; but this should not be affect.  Maybe add a comment 
that the extra cast is required because on ILP32 the ti_offset could be a
64-bit value and thus the whole result would result in a 64-bit value that
needs to be truncated to a address.

>  
>  static void *
>  __attribute_noinline__
> @@ -939,7 +947,7 @@ tls_get_addr_tail (tls_index *ti, dtv_t *dtv, struct link_map *the_map)
>  	  dtv[ti->ti_module].pointer.to_free = NULL;
>  	  dtv[ti->ti_module].pointer.val = p;
>  
> -	  return (char *) p + ti->ti_offset;
> +	  return tls_get_addr_adjust (p, ti);
>  	}
>        else
>  	__rtld_lock_unlock_recursive (GL(dl_load_tls_lock));
> @@ -948,7 +956,7 @@ tls_get_addr_tail (tls_index *ti, dtv_t *dtv, struct link_map *the_map)
>    dtv[ti->ti_module].pointer = result;
>    assert (result.to_free != NULL);
>  
> -  return (char *) result.val + ti->ti_offset;
> +  return tls_get_addr_adjust (result.val, ti);
>  }
>  
>  
> @@ -964,7 +972,7 @@ update_get_addr (tls_index *ti, size_t gen)
>    if (__glibc_unlikely (p == TLS_DTV_UNALLOCATED))
>      return tls_get_addr_tail (ti, dtv, the_map);
>  
> -  return (void *) p + ti->ti_offset;
> +  return tls_get_addr_adjust (p, ti);
>  }
>  
>  /* For all machines that have a non-macro version of __tls_get_addr, we
> @@ -1015,7 +1023,7 @@ __tls_get_addr (tls_index *ti)
>    if (__glibc_unlikely (p == TLS_DTV_UNALLOCATED))
>      return tls_get_addr_tail (ti, dtv, NULL);
>  
> -  return (char *) p + ti->ti_offset;
> +  return tls_get_addr_adjust (p, ti);
>  }
>  #endif /* SHARED */
>  

Ok.