[PATCH 1/3] elf: Add glibc-hwcaps support for LD_LIBRARY_PATH

[Paul A. Clarke]

On Wed, Oct 14, 2020 at 03:58:59PM +0200, Florian Weimer via Libc-alpha wrote:
> * Paul A. Clarke:
> 
> >> +/* 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;
> >
> > Should we note the limitations, that the number of subdirectories must
> > be <= 32?
> 
> Fair enough, I'm going to expand the comment.

OK.

> >> +/* 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.  */
> >> +uint32_t _dl_hwcaps_subdirs_active (void) attribute_hidden;
> >> +
> >> +/* Returns a bitmask that marks the last ACTIVE subdirectories in a
> >> +   _dl_hwcaps_subdirs_active string (containing SUBDIRS directories in
> >> +   total) as active.  Intended for use in _dl_hwcaps_subdirs_active
> >> +   implementations.  */
> >> +static inline uint32_t
> >> +_dl_hwcaps_subdirs_build_bitmask (int subdirs, int active)
> >> +{
> >> +  /* Leading subdirectories that are not active.  */
> >> +  int inactive = subdirs - active;
> >> +  if (inactive == 32)
> >> +    return 0;
> >> +
> >> +  uint32_t mask;
> >> +  if (subdirs < 32)
> >> +    mask = (1U << subdirs) - 1;
> >> +  else
> >> +    mask = -1;
> >> +  return mask ^ ((1U << inactive) - 1);
> >
> > Should we validate any inputs in this function, that:
> > - subdirs <= 32
> > - active <= 32 and active <= subdirs
> 
> Violating these preconditions result in undefined behavior at compile
> time, so I expected GCC (and Clang) to warn about that.  But no such
> luck there.  I asked two colleagues about what we can do on the GCC
> side.  I do think GCC should warn about this under -Wall because it
> returns a totally made-up value.
> 
> I think if we can get that fixed in GCC mainline, we don't have to
> clutter our code with asserts.
> 

With sufficient visibility, GCC can issue such warnings:
test.c:4:34: warning: left shift count >= width of type [-Wshift-count-overflow]
  printf ("%x << 32 = %x\n", r, r << 32);

...so maybe it already "just works", but your patches don't exercise
that because they aren't broken.  :-)

> > While validating this function, I created an equivalent:
> >         if (subdirs == 0) return 0;
> >         if (active == 32) return -1;
> >         uint32_t mask = -1;
> >         /* Mask to include all subdirs.  */
> >         mask >>= 32 - s;
> >         /* Unmask all inactive.  */
> >         mask &= ~(mask >> a);
> >         return mask;
> > ...I found this more readable, but it's subjective.
> 
> Yeah, what we really want here is LDB or DPB, or Erlang's bit syntax. 8-/
> 
> > Also, this routine makes a broad assumption that active subdirectories
> > are all contiguous and at the head of the list.  Maybe this should be
> > renamed _dl_hwcaps_subdirs_build_range_bitmask (or ..._top_range_...),
> > with an updated comment that reflects its limited use-case.
> 
> That makes sense.  I think we can delay that until such a targer
> arrives.

I'm not sure what you mean here.

PC