Re: [PATCH 16/28] elf: Add glibc-hwcaps support for LD_LIBRARY_PATH

["Paul A. Clarke" <pc@us.ibm.com>]

just a couple of nits found while browsing...

On Thu, Oct 01, 2020 at 06:33:16PM +0200, Florian Weimer via Libc-alpha wrote:
> diff --git a/elf/dl-hwcaps.c b/elf/dl-hwcaps.c
> index 44dbac099f..4de94759a2 100644
> --- a/elf/dl-hwcaps.c
> +++ b/elf/dl-hwcaps.c
> @@ -26,20 +26,97 @@
>  #include <dl-procinfo.h>
>  #include <dl-hwcaps.h>
> 
> +/* This is the result of counting the substrings in a colon-separated
> +   hwcaps string.  */
> +struct count_hwcaps
> +{
> +  /* Number of substrings.  */
> +  size_t count;
> +
> +  /* Sum of the individual substring lengths (without separates or

separators?

> +     null terminators).  */
> +  size_t total_length;
> +
> +  /* Maximum length of an individual substring.  */
> +  size_t maximum_length;
> +};
> +
> +/* Update *COUNTS according to the contents of HWCAPS.  Skip over
> +   entries whose bit is not set in MASK.  */
> +static void
> +count_hwcaps (struct count_hwcaps *counts, const char *hwcaps,
> +	      int32_t bitmask, const char *mask)
> +{
> +  struct dl_hwcaps_split_masked sp;
> +  _dl_hwcaps_split_masked_init (&sp, hwcaps, bitmask, mask);
> +  while (_dl_hwcaps_split_masked (&sp))
> +    {
> +      ++counts->count;
> +      counts->total_length += sp.split.length;
> +      if (sp.split.length > counts->maximum_length)
> +	counts->maximum_length = sp.split.length;
> +    }
> +}
> +
> +/* State for copy_hwcaps.  Must be initialized to point to
> +   the storage areas for the array and the strings themselves.  */
> +struct copy_hwcaps
> +{
> +  struct r_strlenpair *next_pair;
> +  char *next_string;
> +};
> +
> +/* Copy HWCAPS into the string pairs and strings, advancing *TARGET.
> +   Skip over entries whose bit is not set in MASK.  */
> +static void
> +copy_hwcaps (struct copy_hwcaps *target, const char *hwcaps,
> +	     int32_t bitmask, const char *mask)
> +{
> +  struct dl_hwcaps_split_masked sp;
> +  _dl_hwcaps_split_masked_init (&sp, hwcaps, bitmask, mask);
> +  while (_dl_hwcaps_split_masked (&sp))
> +    {
> +      target->next_pair->str = target->next_string;
> +      char *slash = __mempcpy (__mempcpy (target->next_string,
> +					  GLIBC_HWCAPS_PREFIX,
> +					  strlen (GLIBC_HWCAPS_PREFIX)),
> +			       sp.split.segment, sp.split.length);
> +      *slash = '/';
> +      target->next_pair->len
> +	= strlen (GLIBC_HWCAPS_PREFIX) + sp.split.length + 1;
> +      ++target->next_pair;
> +      target->next_string = slash + 1;
> +    }
> +}
> +
>  /* Return an array of useful/necessary hardware capability names.  */
>  const struct r_strlenpair *
> -_dl_important_hwcaps (size_t *sz, size_t *max_capstrlen)
> +_dl_important_hwcaps (const char *glibc_hwcaps_prepend,
> +		      const char *glibc_hwcaps_mask,
> +		      size_t *sz, size_t *max_capstrlen)
>  {
>    uint64_t hwcap_mask = GET_HWCAP_MASK();
>    /* Determine how many important bits are set.  */
>    uint64_t masked = GLRO(dl_hwcap) & hwcap_mask;
>    size_t cnt = GLRO (dl_platform) != NULL;
>    size_t n, m;
> -  size_t total;
>    struct r_strlenpair *result;
>    struct r_strlenpair *rp;
>    char *cp;
> 
> +  /* glibc-hwcaps subdirectories.  These are exempted from the power
> +     set construction below below.  */

remove one "below" :-)

> +  int32_t hwcaps_subdirs_active = _dl_hwcaps_subdirs_active ();
> +  struct count_hwcaps hwcaps_counts =  { 0, };
> +  count_hwcaps (&hwcaps_counts, glibc_hwcaps_prepend, -1, NULL);
> +  count_hwcaps (&hwcaps_counts, _dl_hwcaps_subdirs, hwcaps_subdirs_active,
> +		glibc_hwcaps_mask);
> +
> +  /* Each hwcaps subdirectory has a GLIBC_HWCAPS_PREFIX string prefix
> +     and a "/" suffix once stored in the result.  */
> +  size_t total = (hwcaps_counts.count * (strlen (GLIBC_HWCAPS_PREFIX) + 1)
> +		  + hwcaps_counts.total_length);
> +
>    /* Count the number of bits set in the masked value.  */
>    for (n = 0; (~((1ULL << n) - 1) & masked) != 0; ++n)
>      if ((masked & (1ULL << n)) != 0)

> diff --git a/elf/dl-hwcaps.h b/elf/dl-hwcaps.h
> index b66da59b89..a6453f15f3 100644
> --- a/elf/dl-hwcaps.h
> +++ b/elf/dl-hwcaps.h
> @@ -28,3 +33,81 @@
>  #  define GET_HWCAP_MASK() (0)
>  # endif
>  #endif
> +
> +#define GLIBC_HWCAPS_SUBDIRECTORY "glibc-hwcaps"
> +#define GLIBC_HWCAPS_PREFIX GLIBC_HWCAPS_SUBDIRECTORY "/"
> +
> +/* Used by _dl_hwcaps_split below, to split strings at ':'
> +   separators.  */
> +struct dl_hwcaps_split
> +{
> +  const char *segment;          /* Start of the current segment.  */
> +  size_t length;                /* Number of bytes until ':' or NUL.  */
> +};
> +
> +/* Prepare *S to parse SUBJECT, for future _dl_hwcaps_split calls.  If
> +   SUBJECT is NULL, it is treated as the empty string.  */
> +static inline void
> +_dl_hwcaps_split_init (struct dl_hwcaps_split *s, const char *subject)
> +{
> +  s->segment = subject;
> +  /* The initial call to _dl_hwcaps_split will not skip anything.  */
> +  s->length = 0;
> +}
> +
> +/* Extract the next non-empty string segment, up to ':' or the null
> +   terminator.  Return true if one more segment was found, or false if
> +   the end of the string was reached.  On success, S->segment is the
> +   start of the segment found, and S->length is its length.
> +   (Typically, S->segment[S->length] is not null.)  */
> +_Bool _dl_hwcaps_split (struct dl_hwcaps_split *s) attribute_hidden;
> +
> +/* Similar to dl_hwcaps_split, but with bit-based and name-based
> +   masking.  */
> +struct dl_hwcaps_split_masked
> +{
> +  struct dl_hwcaps_split split;
> +
> +  /* For used by the iterator implementation.  */
> +  const char *mask;
> +  int32_t bitmask;
> +};
> +
> +/* Prepare *S for iteration with _dl_hwcaps_split_masked.  Only HWCAP
> +   names in SUBJECT whose bit is set in BITMASK and whose ane is in

s/ane/name/ ?

> +   MASK will be returned.  SUBJECT must not contain empty HWCAP names.
> +   If MASK is NULL, no name-based masking is applied.  Likewise for
> +   BITMASK if BITMASK is -1 (infinite number of bits).  */
> +static inline void
> +_dl_hwcaps_split_masked_init (struct dl_hwcaps_split_masked *s,
> +                              const char *subject,
> +                              int32_t bitmask, const char *mask)
> +{
> +  _dl_hwcaps_split_init (&s->split, subject);
> +  s->bitmask = bitmask;
> +  s->mask = mask;
> +}
> +
> +/* Like _dl_hwcaps_split, but apply masking.  */
> +_Bool _dl_hwcaps_split_masked (struct dl_hwcaps_split_masked *s)
> +  attribute_hidden;
> +
> +/* Returns true if the colon-separated HWCAP list HWCAPS contains the
> +   capability NAME (with length NAME_LENGTH).  If HWCAPS is NULL, the
> +   function returns true.  */
> +_Bool _dl_hwcaps_contains (const char *hwcaps, const char *name,
> +                           size_t name_length) attribute_hidden;
> +
> +/* Colon-separated string of glibc-hwcaps subdirectories, without the
> +   "glibc-hwcaps/" prefix.  The most preferred subdirectory needs to
> +   be listed first.  */
> +extern const char _dl_hwcaps_subdirs[] attribute_hidden;
> +
> +/* Returns a bitmap of active subdirectories in _dl_hwcaps_subdirs.
> +   Bit 0 (the LSB) corresponds to the first substring in
> +   _dl_hwcaps_subdirs, bit 1 to the second substring, and so on.
> +   There is no direct correspondence between HWCAP bitmasks and this
> +   bitmask.  */
> +int32_t _dl_hwcaps_subdirs_active (void) attribute_hidden;
> +
> +#endif /* _DL_HWCAPS_H */

PC