Re: [PATCH 6/6] aarch64: Add hwcap string routines

[Szabolcs Nagy <szabolcs dot nagy at arm dot com>]

On 01/06/17 21:12, Siddhesh Poyarekar wrote:
> Add support for routines in dl-procinfo.h to show string versions of
> HWCAP entries when a program is invoked with the LD_SHOW_AUXV
> environment variable set and also to aid in path resolution for
> ldconfig.
> 
> 	* sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c
> 	(_dl_aarch64_cap_flags): New array.
> 	* sysdeps/unix/sysv/linux/aarch64/dl-procinfo.h
> 	(_dl_hwcap_string, _dl_string_hwcap, _dl_procinfo): Implement
> 	functions.
> ---
>  sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c | 15 +++++++
>  sysdeps/unix/sysv/linux/aarch64/dl-procinfo.h | 58 +++++++++++++++++++++++----
>  2 files changed, 65 insertions(+), 8 deletions(-)
> 

i'm not a fan of magic numbers and strings that
make hwcap flag updates harder.

may be add a note in bits/hwcap.h so whoever
touches that file does not forget to update
dl-procinfo.c ?

> diff --git a/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c b/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c
> index 438046a..bc37bad 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c
> +++ b/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c
> @@ -56,5 +56,20 @@ PROCINFO_CLASS struct cpu_features _dl_aarch64_cpu_features
>  # endif
>  #endif
>  
> +#if !defined PROCINFO_DECL && defined SHARED
> +  ._dl_aarch64_cap_flags
> +#else
> +PROCINFO_CLASS const char _dl_aarch64_cap_flags[13][10]

magic numbers

> +#endif
> +#ifndef PROCINFO_DECL
> += { "fp", "asimd", "evtstrm", "aes", "pmull", "sha1", "sha2", "crc32",
> +    "atomics", "fphp", "asimdhp", "cpuid", "asimdrdm"}

strings

...
> diff --git a/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.h b/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.h
> index 7a60d72..cdb36d3 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.h
> +++ b/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.h
...
> +static inline int
> +__attribute__ ((unused))
> +_dl_procinfo (unsigned int type, unsigned long int word)
> +{
> +  /* This table should match the information from arch/arm64/kernel/cpuinfo.c
> +     in the kernel sources.  */
> +  int i;
>  
> -/* There are no hardware capabilities defined.  */
> -#define _dl_hwcap_string(idx) ""
> +  /* Fallback to unknown output mechanism.  */
> +  if (type == AT_HWCAP2)
> +    return -1;
> +
> +  _dl_printf ("AT_HWCAP:   ");
> +
> +  for (i = 0; i < 32; ++i)
> +    if (word & (1 << i))
> +      _dl_printf (" %s", GLRO(dl_aarch64_cap_flags)[i]);
> +

1<<31 is ub, use 1UL << i or something

i think word can be proper 64bit number on lp64
so may be the loop should stop at 8 * sizeof word
instead of 32 (or _DL_HWCAP_COUNT or...).

> +  _dl_printf ("\n");
> +
> +  return 0;
> +}
...
> +/* 13 HWCAP bits set.  */
> +#define _DL_HWCAP_COUNT 13
> +
> +/* Low 13 bits are allocated in HWCAP.  */
> +#define _DL_HWCAP_LAST 12
>  

magic numbers.