[PATCH 03/11] elf: Add glibc-hwcaps support for LD_LIBRARY_PATH

Matheus Castanho msc@linux.ibm.com
Fri Dec 4 12:25:27 GMT 2020


Hi Florian, after merging this patch builds with --disable-tunables started failing on ppc*:

In file included from dl-hwcaps_split.c:19:
./dl-hwcaps.h:45:3: error: unknown type name ‘size_t’
   size_t length;                /* Number of bytes until ':' or NUL.  */
   ^~~~~~
./dl-hwcaps.h:99:28: error: unknown type name ‘size_t’
                            size_t name_length) attribute_hidden;
                            ^~~~~~
./dl-hwcaps.h:99:28: note: ‘size_t’ is defined in header ‘<stddef.h>’; did you forget to ‘#include <stddef.h>’?
./dl-hwcaps.h:25:1:
+#include <stddef.h>
 
./dl-hwcaps.h:99:28:
                            size_t name_length) attribute_hidden;
                            ^~~~~~
In file included from dl-hwcaps-subdirs.c:19:
./dl-hwcaps.h:45:3: error: unknown type name ‘size_t’
   size_t length;                /* Number of bytes until ':' or NUL.  */
   ^~~~~~
./dl-hwcaps.h:99:28: error: unknown type name ‘size_t’
                            size_t name_length) attribute_hidden;
                            ^~~~~~
./dl-hwcaps.h:99:28: note: ‘size_t’ is defined in header ‘<stddef.h>’; did you forget to ‘#include <stddef.h>’?
./dl-hwcaps.h:25:1:
+#include <stddef.h>
 
./dl-hwcaps.h:99:28:
                            size_t name_length) attribute_hidden;
                            ^~~~~~

Including stddef.h on dl-hwcaps.h fixes the issue:

diff --git a/elf/dl-hwcaps.h b/elf/dl-hwcaps.h
index e6fcac6edc..147dc3d2a8 100644
--- a/elf/dl-hwcaps.h
+++ b/elf/dl-hwcaps.h
@@ -20,6 +20,7 @@
 #define _DL_HWCAPS_H
 
 #include <stdint.h>
+#include <stddef.h>
 
 #include <elf/dl-tunables.h>

Thanks,
Matheus Castanho

On 11/27/20 4:49 PM, Florian Weimer via Libc-alpha wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> On 09/11/2020 15:40, Florian Weimer via Libc-alpha wrote:
>>> This hacks non-power-set processing into _dl_important_hwcaps.
>>> Once the legacy hwcaps handling goes away, the subdirectory
>>> handling needs to be reworked, but it is premature to do this
>>> while both approaches are still supported.
>>
>> Could you extend the commit message with the extra options you are adding
>> on ld.so and their expected semantic? I would be good to have some
>> documentation at least on the commit message.
> 
> I came up with this:
> 
> ld.so supports two new arguments, --glibc-hwcaps-prepend and
> --glibc-hwcaps-mask.  Each accepts a colon-separated list of
> glibc-hwcaps subdirectory names.  The prepend option adds additional 
> subdirectories that are searched first, in the specified order.  The 
> mask option restricts the automatically selected subdirectories to 
> those listed in the option argument.
> 
> I'll also try to come up with a way to document ld.so invocations in the
> manual, but this will take some time (and probably some command line
> interface improvements).
> 
>>> @@ -1812,3 +1816,56 @@ $(objpfx)argv0test.out: tst-rtld-argv0.sh $(objpfx)ld.so \
>>>              '$(test-wrapper-env)' '$(run_program_env)' \
>>>              '$(rpath-link)' 'test-argv0' > $@; \
>>>      $(evaluate-test)
>>> +
>>> +# Most likely search subdirectories, for each supported architecture.
>>> +# Used to obtain test coverage wide test coverage.
>>
>> This comments sounds strange, specially the second line.
> 
> Eh, that was quite garbled.  I 
> 
> # A list containing the name of the most likely searched subdirectory
> # of the glibc-hwcaps directory, for each supported architecture (in
> # other words, the oldest hardware level recognized by the
> # glibc-hwcaps mechanism for this architecture).  Used to obtain test
> # coverage for some glibc-hwcaps tests for the widest possible range
> # of systems.
> glibc-hwcaps-first-subdirs-for-tests =
> 
>>> +/* 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;
>>
>> I find it confusing that an null hwcap result true while an empty
>> relies on name.  Do we need that NULL to be handled special here?
> 
> NULL means there is no mask.  It denotes the universe in set terms.
> The caller assumes this.  The empty set can be written as "".
> 
>>> +/* Colon-separated string of glibc-hwcaps subdirectories, without the
>>> +/* 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 (if a contiguous tail of the list in
>>> +   _dl_hwcaps_subdirs is selected).  */
>>> +static inline uint32_t
>>> +_dl_hwcaps_subdirs_build_bitmask (int subdirs, int active)
>>> +{
>>
>> Should it any assert to check if subdirs and/or active is within the
>> expected range?
> 
> We discussed this before, I think.  I think GCC should warn if the
> numbers are out of range because the result is undefined.  I filed GCC
> PR 97424 for this.  GCC knows about the undefined nature and exploits it
> (but not as agressively as Clang), but does not warn about it because
> this issue occurs very often in code that is eventually optimized away
> completely.
> 
>>>  /* Write a list of hwcap subdirectories to standard output.  See
>>>   _dl_important_hwcaps in dl-hwcaps.c.  */
>>>  static void
>>> @@ -186,6 +247,10 @@ setting environment variables (which would be inherited by subprocesses).\n\
>>>    --inhibit-cache       Do not use " LD_SO_CACHE "\n\
>>>    --library-path PATH   use given PATH instead of content of the environment\n\
>>>                          variable LD_LIBRARY_PATH\n\
>>> +  --glibc-hwcaps-prepend LIST\n\
>>> +                        search glibc-hwcaps subdirectories in LIST\n\
>>> +  --glibc-hwcaps-mask LIST\n\
>>> +                        only search built-in subdirectories if in LIST\n\
>>>    --inhibit-rpath LIST  ignore RUNPATH and RPATH information in object names\n\
>>>                          in LIST\n\
>>>    --audit LIST          use objects named in LIST as auditors\n\
>>> @@ -198,6 +263,7 @@ This program interpreter self-identifies as: " RTLD "\n\
>>>  ",
>>>                argv0);
>>>    print_search_path_for_help (state);
>>> +  print_hwcaps_subdirectories (state);
>>>    print_legacy_hwcap_directories ();
>>>    _exit (EXIT_SUCCESS);
>>>  }
>>
>> Do we really need to export the new options with the '--glibc' prefix?
>> The loader options are an implementation detail (same for the internal
>> variable names).
> 
> I did that to match the glibc-hwcaps directory name.  We already have
> LD_HWCAP_MASK, and --hwcaps-mask looks like it would be related to that.
> 
>>> +#ifndef MARKER
>>> +# error MARKER not defined
>>> +#endif
>>
>> Maybe also add a check for the VALUE?
> 
> Compilation will fail anyway if VALUE is not defined:
> 
>>> +
>>> +int
>>> +MARKER (void)
>>> +{
>>> +  return VALUE;
>>> +}
> 
>>> +  /* Tests for _dl_hwcaps_contains.  */
>>> +  TEST_VERIFY (_dl_hwcaps_contains (NULL, "first", strlen ("first")));
>>> +  TEST_VERIFY (_dl_hwcaps_contains (NULL, "", 0));
>>> +  TEST_VERIFY (! _dl_hwcaps_contains ("", "first", strlen ("first")));
>>> +  TEST_VERIFY (! _dl_hwcaps_contains ("firs", "first", strlen ("first")));
>>> +  TEST_VERIFY (_dl_hwcaps_contains ("firs", "first", strlen ("first") - 1));
>>> +  for (int i = 0; i < strlen ("first"); ++i)
>>> +    TEST_VERIFY (! _dl_hwcaps_contains ("first", "first", i));
>>> +  TEST_VERIFY (_dl_hwcaps_contains ("first", "first", strlen ("first")));
>>> +  TEST_VERIFY (_dl_hwcaps_contains ("first:", "first", strlen ("first")));
>>> +  TEST_VERIFY (_dl_hwcaps_contains ("first:second",
>>> +                                    "first", strlen ("first")));
>>> +  TEST_VERIFY (_dl_hwcaps_contains (":first:second", "first",
>>> +                                    strlen ("first")));
>>> +  TEST_VERIFY (_dl_hwcaps_contains ("first:second", "second",
>>> +                                    strlen ("second")));
>>> +  TEST_VERIFY (_dl_hwcaps_contains ("first:second:", "second",
>>> +                                    strlen ("second")));
>>> +  for (int i = 0; i < strlen ("second"); ++i)
>>> +    TEST_VERIFY (!_dl_hwcaps_contains ("first:second:", "sec", i));
>>
>> Maybe add some tests with multiple ':'.
> 
> Good idea, I came up with this:
> 
> @@ -126,8 +126,17 @@ do_test (void)
>                                      strlen ("second")));
>    TEST_VERIFY (_dl_hwcaps_contains ("first:second:", "second",
>                                      strlen ("second")));
> +  TEST_VERIFY (_dl_hwcaps_contains ("first::second:", "second",
> +                                    strlen ("second")));
> +  TEST_VERIFY (_dl_hwcaps_contains ("first:second::", "second",
> +                                    strlen ("second")));
>    for (int i = 0; i < strlen ("second"); ++i)
> -    TEST_VERIFY (!_dl_hwcaps_contains ("first:second:", "sec", i));
> +    {
> +      TEST_VERIFY (!_dl_hwcaps_contains ("first:second", "second", i));
> +      TEST_VERIFY (!_dl_hwcaps_contains ("first:second:", "second", i));
> +      TEST_VERIFY (!_dl_hwcaps_contains ("first:second::", "second", i));
> +      TEST_VERIFY (!_dl_hwcaps_contains ("first::second", "second", i));
> +    }
>  
>    return 0;
>  }
> 
> i == 0 in the loop handles empty strings.
> 
> Thanks,
> Florian
> 


More information about the Libc-alpha mailing list