[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