From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: libc-alpha@sourceware.org
Subject: Re: [PATCH 23/28] elf: Unify old and new format cache handling code in ld.so
Date: Fri, 16 Oct 2020 11:37:38 -0300 [thread overview]
Message-ID: <286692c4-80ef-3115-0980-a5029fe8faa7@linaro.org> (raw)
In-Reply-To: <e1b021451aa66d1c03334a401dde007906d03b34.1601569371.git.fweimer@redhat.com>
On 01/10/2020 13:34, Florian Weimer via Libc-alpha wrote:
> struct file_entry_new starts with the fields of struct file_entry,
> so the code can be shared if the size computation is made dynamic.
LGTM, with some nits below.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
> elf/dl-cache.c | 287 +++++++++++++++++++------------------
> sysdeps/generic/dl-cache.h | 17 ++-
> 2 files changed, 158 insertions(+), 146 deletions(-)
>
> diff --git a/elf/dl-cache.c b/elf/dl-cache.c
> index 3aa8ed6c13..02c46ffb0c 100644
> --- a/elf/dl-cache.c
> +++ b/elf/dl-cache.c
> @@ -35,103 +35,141 @@ static struct cache_file *cache;
> static struct cache_file_new *cache_new;
> static size_t cachesize;
>
> -/* 1 if cache_data + PTR points into the cache. */
> -#define _dl_cache_verify_ptr(ptr) (ptr < cache_data_size)
> -
> -#define SEARCH_CACHE(cache) \
> -/* We use binary search since the table is sorted in the cache file. \
> - The first matching entry in the table is returned. \
> - It is important to use the same algorithm as used while generating \
> - the cache file. */ \
> -do \
> - { \
> - left = 0; \
> - right = cache->nlibs - 1; \
> - \
> - while (left <= right) \
> - { \
> - __typeof__ (cache->libs[0].key) key; \
> - \
> - middle = (left + right) / 2; \
> - \
> - key = cache->libs[middle].key; \
> - \
> - /* Make sure string table indices are not bogus before using \
> - them. */ \
> - if (! _dl_cache_verify_ptr (key)) \
> - { \
> - cmpres = 1; \
> - break; \
> - } \
> - \
> - /* Actually compare the entry with the key. */ \
> - cmpres = _dl_cache_libcmp (name, cache_data + key); \
> - if (__glibc_unlikely (cmpres == 0)) \
> - { \
> - /* Found it. LEFT now marks the last entry for which we \
> - know the name is correct. */ \
> - left = middle; \
> - \
> - /* There might be entries with this name before the one we \
> - found. So we have to find the beginning. */ \
> - while (middle > 0) \
> - { \
> - __typeof__ (cache->libs[0].key) key; \
> - \
> - key = cache->libs[middle - 1].key; \
> - /* Make sure string table indices are not bogus before \
> - using them. */ \
> - if (! _dl_cache_verify_ptr (key) \
> - /* Actually compare the entry. */ \
> - || _dl_cache_libcmp (name, cache_data + key) != 0) \
> - break; \
> - --middle; \
> - } \
> - \
> - do \
> - { \
> - int flags; \
> - __typeof__ (cache->libs[0]) *lib = &cache->libs[middle]; \
> - \
> - /* Only perform the name test if necessary. */ \
> - if (middle > left \
> - /* We haven't seen this string so far. Test whether the \
> - index is ok and whether the name matches. Otherwise \
> - we are done. */ \
> - && (! _dl_cache_verify_ptr (lib->key) \
> - || (_dl_cache_libcmp (name, cache_data + lib->key) \
> - != 0))) \
> - break; \
> - \
> - flags = lib->flags; \
> - if (_dl_cache_check_flags (flags) \
> - && _dl_cache_verify_ptr (lib->value)) \
> - { \
> - if (best == NULL || flags == GLRO(dl_correct_cache_id)) \
> - { \
> - HWCAP_CHECK; \
> - best = cache_data + lib->value; \
> - \
> - if (flags == GLRO(dl_correct_cache_id)) \
> - /* We've found an exact match for the shared \
> - object and no general `ELF' release. Stop \
> - searching. */ \
> - break; \
> - } \
> - } \
> - } \
> - while (++middle <= right); \
> - break; \
> - } \
> - \
> - if (cmpres < 0) \
> - left = middle + 1; \
> - else \
> - right = middle - 1; \
> - } \
> - } \
> -while (0)
Ok.
> +/* True if PTR is a valid string table index. */
> +static inline bool
> +_dl_cache_verify_ptr (uint32_t ptr, size_t string_table_size)
> +{
> + return ptr < string_table_size;
> +}
> +
Ok.
> +/* Compute the address of the element INDEX of the array at LIBS.
> + Conceptually, this is &LIBS[INDEX], but use ENTRY_SIZE for the size
> + of *LIBS. */
> +static inline const struct file_entry *
> +_dl_cache_file_entry (const struct file_entry *libs, size_t entry_size,
> + size_t index)
> +{
> + return (const void *) libs + index * entry_size;
> +}
> +
Although pointer arithmetic with void *is valid for gcc, I think we should
use uintptr_t here.
> +/* We use binary search since the table is sorted in the cache file.
> + The first matching entry in the table is returned. It is important
> + to use the same algorithm as used while generating the cache file.
> + STRING_TABLE_SIZE indicates the maximum offset in STRING_TABLE at
> + which data is mapped; it is not exact. */
> +static const char *
> +search_cache (const char *string_table, uint32_t string_table_size,
> + struct file_entry *libs, uint32_t nlibs, uint32_t entry_size,
> + const char *name)
> +{
> + /* Used by the HWCAP check in the struct file_entry_new case. */
> + uint64_t platform = _dl_string_platform (GLRO (dl_platform));
> + if (platform != (uint64_t) -1)
> + platform = 1ULL << platform;
Maybe UINT64_C(1) << platform?
> + uint64_t hwcap_mask = GET_HWCAP_MASK ();
> +#define _DL_HWCAP_TLS_MASK (1LL << 63)
Same as before.
> + uint64_t hwcap_exclude = ~((GLRO (dl_hwcap) & hwcap_mask)
> + | _DL_HWCAP_PLATFORM | _DL_HWCAP_TLS_MASK);
> +
> + int left = 0;
> + int right = nlibs - 1;
> + const char *best = NULL;
> +
> + while (left <= right)
> + {
> + int middle = (left + right) / 2;
> + uint32_t key = _dl_cache_file_entry (libs, entry_size, middle)->key;
> +
> + /* Make sure string table indices are not bogus before using
> + them. */
> + if (!_dl_cache_verify_ptr (key, string_table_size))
> + return NULL;
>
> + /* Actually compare the entry with the key. */
> + int cmpres = _dl_cache_libcmp (name, string_table + key);
> + if (__glibc_unlikely (cmpres == 0))
> + {
> + /* Found it. LEFT now marks the last entry for which we
> + know the name is correct. */
> + left = middle;
> +
> + /* There might be entries with this name before the one we
> + found. So we have to find the beginning. */
> + while (middle > 0)
> + {
> + key = _dl_cache_file_entry (libs, entry_size, middle - 1)->key;
> + /* Make sure string table indices are not bogus before
> + using them. */
> + if (!_dl_cache_verify_ptr (key, string_table_size)
> + /* Actually compare the entry. */
> + || _dl_cache_libcmp (name, string_table + key) != 0)
> + break;
> + --middle;
> + }
> +
> + do
> + {
> + int flags;
> + const struct file_entry *lib
> + = _dl_cache_file_entry (libs, entry_size, middle);
> +
> + /* Only perform the name test if necessary. */
> + if (middle > left
> + /* We haven't seen this string so far. Test whether the
> + index is ok and whether the name matches. Otherwise
> + we are done. */
> + && (! _dl_cache_verify_ptr (lib->key, string_table_size)
> + || (_dl_cache_libcmp (name, string_table + lib->key)
> + != 0)))
> + break;
> +
> + flags = lib->flags;
> + if (_dl_cache_check_flags (flags)
> + && _dl_cache_verify_ptr (lib->value, string_table_size))
> + {
> + if (best == NULL || flags == GLRO (dl_correct_cache_id))
> + {
> + if (entry_size >= sizeof (struct file_entry_new))
> + {
> + /* The entry is large enough to include
> + HWCAP data. Check it. */
> + struct file_entry_new *libnew
> + = (struct file_entry_new *) lib;
> +
> + if (libnew->hwcap & hwcap_exclude)
> + continue;
> + if (GLRO (dl_osversion)
> + && libnew->osversion > GLRO (dl_osversion))
> + continue;
> + if (_DL_PLATFORMS_COUNT
> + && (libnew->hwcap & _DL_HWCAP_PLATFORM) != 0
> + && ((libnew->hwcap & _DL_HWCAP_PLATFORM)
> + != platform))
> + continue;
> + }
Ok, this is HWCAP_CHECK expanded.
> +
> + best = string_table + lib->value;
> +
> + if (flags == GLRO (dl_correct_cache_id))
> + /* We've found an exact match for the shared
> + object and no general `ELF' release. Stop
Should we replace the grave accent with apostrophe in this case?
> + searching. */
> + break;
> + }
> + }
> + }
> + while (++middle <= right);
> + break;
> + }
> +
> + if (cmpres < 0)
> + left = middle + 1;
> + else
> + right = middle - 1;
> + }
> +
> + return best;
> +}
>
> int
> _dl_cache_libcmp (const char *p1, const char *p2)
Ok.
> @@ -182,12 +220,6 @@ _dl_cache_libcmp (const char *p1, const char *p2)
> char *
> _dl_load_cache_lookup (const char *name)
> {
> - int left, right, middle;
> - int cmpres;
> - const char *cache_data;
> - uint32_t cache_data_size;
> - const char *best;
> -
> /* Print a message if the loading of libs is traced. */
> if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_LIBS))
> _dl_debug_printf (" search cache=%s\n", LD_SO_CACHE);
Ok.
> @@ -265,51 +297,22 @@ _dl_load_cache_lookup (const char *name)
> /* Previously looked for the cache file and didn't find it. */
> return NULL;
>
> - best = NULL;
> -
> + const char *best;
> if (cache_new != (void *) -1)
> {
> - uint64_t platform;
> -
> - /* This is where the strings start. */
> - cache_data = (const char *) cache_new;
> -
> - /* Now we can compute how large the string table is. */
> - cache_data_size = (const char *) cache + cachesize - cache_data;
> -
> - platform = _dl_string_platform (GLRO(dl_platform));
> - if (platform != (uint64_t) -1)
> - platform = 1ULL << platform;
> -
> - uint64_t hwcap_mask = GET_HWCAP_MASK();
> -
> -#define _DL_HWCAP_TLS_MASK (1LL << 63)
> - uint64_t hwcap_exclude = ~((GLRO(dl_hwcap) & hwcap_mask)
> - | _DL_HWCAP_PLATFORM | _DL_HWCAP_TLS_MASK);
> -
> - /* Only accept hwcap if it's for the right platform. */
> -#define HWCAP_CHECK \
> - if (lib->hwcap & hwcap_exclude) \
> - continue; \
> - if (GLRO(dl_osversion) && lib->osversion > GLRO(dl_osversion)) \
> - continue; \
> - if (_DL_PLATFORMS_COUNT \
> - && (lib->hwcap & _DL_HWCAP_PLATFORM) != 0 \
> - && (lib->hwcap & _DL_HWCAP_PLATFORM) != platform) \
> - continue
> - SEARCH_CACHE (cache_new);
> + const char *string_table = (const char *) cache_new;
> + best = search_cache (string_table, cachesize,
> + &cache_new->libs[0].entry, cache_new->nlibs,
> + sizeof (cache_new->libs[0]), name);
> }
Ok.
> else
> {
> - /* This is where the strings start. */
> - cache_data = (const char *) &cache->libs[cache->nlibs];
> -
> - /* Now we can compute how large the string table is. */
> - cache_data_size = (const char *) cache + cachesize - cache_data;
> -
> -#undef HWCAP_CHECK
> -#define HWCAP_CHECK do {} while (0)
> - SEARCH_CACHE (cache);
> + const char *string_table = (const char *) &cache->libs[cache->nlibs];
> + uint32_t string_table_size
> + = (const char *) cache + cachesize - string_table;
> + best = search_cache (string_table, string_table_size,
> + &cache->libs[0], cache->nlibs,
> + sizeof (cache->libs[0]), name);
> }
>
> /* Print our result if wanted. */
Ok.
> diff --git a/sysdeps/generic/dl-cache.h b/sysdeps/generic/dl-cache.h
> index b154740da9..fec209509d 100644
> --- a/sysdeps/generic/dl-cache.h
> +++ b/sysdeps/generic/dl-cache.h
> @@ -66,8 +66,8 @@
> */
> struct file_entry
> {
> - int flags; /* This is 1 for an ELF library. */
> - unsigned int key, value; /* String table indices. */
> + int32_t flags; /* This is 1 for an ELF library. */
> + uint32_t key, value; /* String table indices. */
> };
>
Ok, although not sure if our code guidelines allows multiple fields in the
same line.
> struct cache_file
> @@ -84,8 +84,17 @@ struct cache_file
>
> struct file_entry_new
> {
> - int32_t flags; /* This is 1 for an ELF library. */
> - uint32_t key, value; /* String table indices. */
> + union
> + {
> + /* Fields shared with struct file_entry. */
> + struct file_entry entry;
> + /* Also expose these fields directly. */
> + struct
> + {
> + int32_t flags; /* This is 1 for an ELF library. */
> + uint32_t key, value; /* String table indices. */
> + };
> + };
> uint32_t osversion; /* Required OS version. */
> uint64_t hwcap; /* Hwcap entry. */
> };
>
Ok.
next prev parent reply other threads:[~2020-10-16 14:37 UTC|newest]
Thread overview: 103+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-01 16:31 [PATCH 00/28] glibc-hwcaps support Florian Weimer
2020-10-01 16:31 ` [PATCH 01/28] elf: Do not search HWCAP subdirectories in statically linked binaries Florian Weimer
2020-10-01 18:22 ` Adhemerval Zanella
2020-10-01 18:24 ` Carlos O'Donell
2020-10-01 18:29 ` Adhemerval Zanella
2020-10-01 20:24 ` Carlos O'Donell
2020-10-01 16:31 ` [PATCH 02/28] elf: Implement __rtld_malloc_is_full Florian Weimer
2020-10-01 18:23 ` Adhemerval Zanella
2020-10-08 9:44 ` Florian Weimer
2020-10-01 16:31 ` [PATCH 03/28] elf: Implement _dl_write Florian Weimer
2020-10-05 19:46 ` Adhemerval Zanella
2020-10-01 16:31 ` [PATCH 04/28] elf: Extract command-line/environment variables state from rtld.c Florian Weimer
2020-10-06 20:45 ` Adhemerval Zanella
2020-10-08 11:32 ` Florian Weimer
2020-10-01 16:32 ` [PATCH 05/28] elf: Move ld.so error/help output to _dl_usage Florian Weimer
2020-10-06 21:06 ` Adhemerval Zanella
2020-10-08 12:19 ` Florian Weimer
2020-10-01 16:32 ` [PATCH 06/28] elf: Record whether paths come from LD_LIBRARY_PATH or --library-path Florian Weimer
2020-10-07 16:39 ` Adhemerval Zanella
2020-10-07 16:49 ` Florian Weimer
2020-10-01 16:32 ` [PATCH 07/28] elf: Implement ld.so --help Florian Weimer
2020-10-07 17:16 ` Adhemerval Zanella
2020-10-08 13:13 ` Florian Weimer
2020-10-01 16:32 ` [PATCH 08/28] elf: Implement ld.so --version Florian Weimer
2020-10-07 18:36 ` Adhemerval Zanella
2020-10-07 18:38 ` Adhemerval Zanella
2020-10-08 13:37 ` Florian Weimer
2020-10-01 16:32 ` [PATCH 09/28] scripts/update-copyrights: Update csu/version.c, elf/dl-usage.c Florian Weimer
2020-10-07 18:41 ` Adhemerval Zanella
2020-10-01 16:32 ` [PATCH 10/28] elf: Use the term "program interpreter" in the ld.so help message Florian Weimer
2020-10-07 21:08 ` Adhemerval Zanella
2020-10-08 14:08 ` Florian Weimer
2020-10-01 16:32 ` [PATCH 11/28] elf: Print the full name of the dynamic loader " Florian Weimer
2020-10-08 12:38 ` Adhemerval Zanella
2020-10-01 16:32 ` [PATCH 12/28] elf: Make __rtld_env_path_list and __rtld_search_dirs global variables Florian Weimer
2020-10-08 13:27 ` Adhemerval Zanella
2020-10-01 16:32 ` [PATCH 13/28] elf: Add library search path information to ld.so --help Florian Weimer
2020-10-08 16:22 ` Adhemerval Zanella
2020-10-01 16:33 ` [PATCH 14/28] elf: Enhance ld.so --help to print HWCAP subdirectories Florian Weimer
2020-10-08 16:27 ` Adhemerval Zanella
2020-10-09 8:18 ` Florian Weimer
2020-10-09 13:49 ` Matheus Castanho
2020-10-09 17:08 ` Florian Weimer
2020-10-09 17:12 ` Florian Weimer
2020-10-09 18:54 ` Matheus Castanho
2020-10-12 9:47 ` Florian Weimer
2020-10-01 16:33 ` [PATCH 15/28] elf: Do not pass GLRO(dl_platform), GLRO(dl_platformlen) to _dl_important_hwcaps Florian Weimer
2020-10-08 18:04 ` Adhemerval Zanella
2020-10-01 16:33 ` [PATCH 16/28] elf: Add glibc-hwcaps support for LD_LIBRARY_PATH Florian Weimer
2020-10-08 10:13 ` Szabolcs Nagy
2020-10-09 9:08 ` Florian Weimer
2020-10-09 10:50 ` Szabolcs Nagy
2020-10-09 10:55 ` Florian Weimer
2020-10-09 11:03 ` Szabolcs Nagy
2020-10-08 23:16 ` Paul A. Clarke
2020-10-09 8:56 ` Florian Weimer
2020-10-09 13:19 ` Adhemerval Zanella
2020-10-12 11:54 ` Florian Weimer
2020-10-01 16:33 ` [PATCH 17/28] x86_64: Add glibc-hwcaps support Florian Weimer
2020-10-01 16:33 ` [PATCH 18/28] powerpc64le: " Florian Weimer
2020-10-01 18:56 ` Paul A. Clarke
2020-10-05 9:47 ` Florian Weimer
2020-10-05 19:15 ` Paul A. Clarke
2020-10-06 12:20 ` Florian Weimer
2020-10-06 17:45 ` Paul A. Clarke
2020-10-09 9:06 ` Florian Weimer
2020-10-01 16:33 ` [PATCH 19/28] s390x: Add " Florian Weimer
2020-10-01 16:33 ` [PATCH 20/28] aarch64: " Florian Weimer
2020-10-14 13:46 ` Adhemerval Zanella
2020-10-14 14:08 ` Florian Weimer
2020-10-14 14:15 ` Adhemerval Zanella
2020-10-14 14:37 ` Szabolcs Nagy
2020-10-14 14:43 ` Adhemerval Zanella
2020-10-14 15:13 ` Florian Weimer
2020-10-14 14:44 ` Florian Weimer
2020-10-14 15:09 ` Szabolcs Nagy
2020-10-01 16:33 ` [PATCH 21/28] elf: Add endianness markup to ld.so.cache Florian Weimer
2020-10-14 14:07 ` Adhemerval Zanella
2020-10-01 16:33 ` [PATCH 22/28] elf: Add extension mechanism " Florian Weimer
2020-10-15 17:52 ` Adhemerval Zanella
2020-10-30 12:22 ` Florian Weimer
2020-11-03 12:45 ` Adhemerval Zanella
2020-11-03 15:30 ` Florian Weimer
2020-10-01 16:34 ` [PATCH 23/28] elf: Unify old and new format cache handling code in ld.so Florian Weimer
2020-10-16 14:37 ` Adhemerval Zanella [this message]
2020-10-30 13:22 ` Florian Weimer
2020-11-03 13:02 ` Adhemerval Zanella
2020-10-01 16:34 ` [PATCH 24/28] elf: Implement a string table for ldconfig, with tail merging Florian Weimer
2020-10-20 14:25 ` Adhemerval Zanella
2020-10-30 17:08 ` Florian Weimer
2020-11-03 13:05 ` Adhemerval Zanella
2020-11-03 15:29 ` Florian Weimer
2020-10-01 16:34 ` [PATCH 25/28] elf: Implement tail merging of strings in ldconfig Florian Weimer
2020-10-22 21:08 ` Adhemerval Zanella
2020-10-30 17:36 ` Florian Weimer
2020-10-01 16:34 ` [PATCH 26/28] elf: In ldconfig, extract the new_sub_entry function from search_dir Florian Weimer
2020-10-27 13:15 ` Adhemerval Zanella
2020-10-01 16:34 ` [PATCH 27/28] elf: Process glibc-hwcaps subdirectories in ldconfig Florian Weimer
2020-10-27 17:28 ` Adhemerval Zanella
2020-11-04 11:57 ` Florian Weimer
2020-10-01 16:34 ` [PATCH 28/28] elf: Add glibc-hwcaps subdirectory support to ld.so cache processing Florian Weimer
2020-10-01 16:50 ` [PATCH 00/28] glibc-hwcaps support H.J. Lu
2020-10-01 16:54 ` Florian Weimer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=286692c4-80ef-3115-0980-a5029fe8faa7@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).