Re: [PATCH 18/28] powerpc64le: Add glibc-hwcaps support

[Florian Weimer <fweimer@redhat.com>]

* Paul A. Clarke:

> On Mon, Oct 05, 2020 at 11:47:32AM +0200, Florian Weimer wrote:
>> * Paul A. Clarke:
>> >> diff --git a/sysdeps/powerpc/powerpc64/le/dl-hwcaps-subdirs.c b/sysdeps/powerpc/powerpc64/le/dl-hwcaps-subdirs.c
>> >> new file mode 100644
>> >> index 0000000000..496daf0fa0
>> >> --- /dev/null
>> >> +++ b/sysdeps/powerpc/powerpc64/le/dl-hwcaps-subdirs.c
> [...snip...]
>> >> +_dl_hwcaps_subdirs_active (void)
>> >> +{
>> >> +  if (GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_1)
>> >> +    return 3;
>> >> +
>> >> +  if (GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_00)
>> >> +    return 1;

Meh, the second test should do: return 2; 8-(

Maybe we should write it this way:

  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_00)) == 0)
    return 0;  /* No subdirectories active.  */

  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_1)) == 0)
    return 2;  /* Only the second directory (power9) is active.  */

  return 3;  /* Both directories (power10, power9) are active.  */

> ...which still creates magic numbers and really doesn't explain what the
> result represents.
> The entire API is documented in a 2-line comment before count_hwcaps().
> For someone to update the implementation for a new hwcap, which will
> need to be done fairly often, seems to me to be a bit challenging for
> the uninitiated (like me ;-).

The comments are in elf/dl-hwcaps.h:

/* 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;

>> > Perhaps something like (not tested):
>> > --
>> > const char * const _dl_hwcaps_subdirs[] = {
>> > #define _DL_HWCAPS_SUBDIR_POWER10_BIT 0x2 /* or 1 to preserve same order.  */
>> > 	"power10",
>> > #define _DL_HWCAPS_SUBDIR_POWER9_BIT 0x1 /* or 2.  */
>> > 	"power9"
>> > };
>> >
>> > int32_t
>> > _dl_hwcaps_subdirs_active (void)
>> > {
>> >   int32_t result = 0;
>> >
>> >   if (GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_1)
>> >     result |= _DL_HWCAPS_SUBDIR_POWER10_BIT;
>> >
>> >   if (GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_00)
>> >     result |= _DL_HWCAPS_SUBDIR_POWER9_BIT;
>> >
>> >   return result;
>> > }
>> > --
>> >
>> > Of course, that would require changes to the code that parses
>> > _dl_hwcaps_subdirs.
>> 
>> I chose the current approach to avoid relocations and memory allocations
>> for processing hwcaps settings (e.g. from the ld.so command line).
>
> Does the "array of strings" approach introduce new/additional relocations
> and memory allocations?  It would seem to avoid the need for the splitting
> code, at least.

The string pointers need runtime relocations (although they probably do
not matter in the grand scheme of things).  We still need the splitting
code for the ld.so command line argument.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill