From 8e25d1e7721d8078d8925e325799740dd53a5801 Mon Sep 17 00:00:00 2001 From: Carlos O'Donell Date: Fri, 28 Feb 2014 18:11:06 -0500 Subject: [PATCH] Promote do_lookup_x:check_match to a full function. While it may be argued that nested functions make the resulting code easier to read, or worse to read the following two bugs make it difficult to debug: Bug 8300 - no local symbol information within nested or nesting procedures https://sourceware.org/bugzilla/show_bug.cgi?id=8300 Bug 53927 - wrong value for DW_AT_static_link http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927 Until these are fixed I've made check_match a full function. After they are fixed we can resume arguing about the merits of nested functions on readability and maintenance. --- ChangeLog | 7 ++ elf/dl-lookup.c | 226 ++++++++++++++++++++++++++---------------------- 2 files changed, 129 insertions(+), 104 deletions(-) diff --git a/ChangeLog b/ChangeLog index a5b1de26d0..60d63dbf88 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2014-02-28 Carlos O'Donell + + * elf/dl-lookup.c (check_match): New function. + (ELF_MACHINE_SYM_NO_MATCH): Adjust comment. + (do_lookup_x): Remove nested function check_match. Use non-nested + function check_match. + 2014-02-28 Roland McGrath * csu/Makefile (generated, before-compile): Use += rather than =. diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c index 0b43db8d9b..d1b8efc5d7 100644 --- a/elf/dl-lookup.c +++ b/elf/dl-lookup.c @@ -31,9 +31,8 @@ #include -/* Return nonzero if do_lookup_x:check_match should consider SYM to - fail to match a symbol reference for some machine-specific - reason. */ +/* Return nonzero if check_match should consider SYM to fail to match a + symbol reference for some machine-specific reason. */ #ifndef ELF_MACHINE_SYM_NO_MATCH # define ELF_MACHINE_SYM_NO_MATCH(sym) 0 #endif @@ -75,6 +74,118 @@ struct sym_val # define bump_num_relocations() ((void) 0) #endif +/* Utility function for do_lookup_x. The caller is called with undef_name, + ref, version, flags and type_class, and those are passed as the first + five arguments. The caller then computes sym, symidx, strtab, and map + and passes them as the next four arguments. Lastly the caller passes in + versioned_sym and num_versions which are modified by check_match during + the checking process. */ +static const ElfW(Sym) * +check_match (const char *const undef_name, + const ElfW(Sym) *const ref, + const struct r_found_version *const version, + const int flags, + const int type_class, + const ElfW(Sym) *const sym, + const Elf_Symndx symidx, + const char *const strtab, + const struct link_map *const map, + const ElfW(Sym) **const versioned_sym, + int *const num_versions) +{ + unsigned int stt = ELFW(ST_TYPE) (sym->st_info); + assert (ELF_RTYPE_CLASS_PLT == 1); + if (__builtin_expect ((sym->st_value == 0 /* No value. */ + && stt != STT_TLS) + || ELF_MACHINE_SYM_NO_MATCH (sym) + || (type_class & (sym->st_shndx == SHN_UNDEF)), + 0)) + return NULL; + + /* Ignore all but STT_NOTYPE, STT_OBJECT, STT_FUNC, + STT_COMMON, STT_TLS, and STT_GNU_IFUNC since these are no + code/data definitions. */ +#define ALLOWED_STT \ + ((1 << STT_NOTYPE) | (1 << STT_OBJECT) | (1 << STT_FUNC) \ + | (1 << STT_COMMON) | (1 << STT_TLS) | (1 << STT_GNU_IFUNC)) + if (__glibc_unlikely (((1 << stt) & ALLOWED_STT) == 0)) + return NULL; + + if (sym != ref && strcmp (strtab + sym->st_name, undef_name)) + /* Not the symbol we are looking for. */ + return NULL; + + const ElfW(Half) *verstab = map->l_versyms; + if (version != NULL) + { + if (__glibc_unlikely (verstab == NULL)) + { + /* We need a versioned symbol but haven't found any. If + this is the object which is referenced in the verneed + entry it is a bug in the library since a symbol must + not simply disappear. + + It would also be a bug in the object since it means that + the list of required versions is incomplete and so the + tests in dl-version.c haven't found a problem.*/ + assert (version->filename == NULL + || ! _dl_name_match_p (version->filename, map)); + + /* Otherwise we accept the symbol. */ + } + else + { + /* We can match the version information or use the + default one if it is not hidden. */ + ElfW(Half) ndx = verstab[symidx] & 0x7fff; + if ((map->l_versions[ndx].hash != version->hash + || strcmp (map->l_versions[ndx].name, version->name)) + && (version->hidden || map->l_versions[ndx].hash + || (verstab[symidx] & 0x8000))) + /* It's not the version we want. */ + return NULL; + } + } + else + { + /* No specific version is selected. There are two ways we + can got here: + + - a binary which does not include versioning information + is loaded + + - dlsym() instead of dlvsym() is used to get a symbol which + might exist in more than one form + + If the library does not provide symbol version information + there is no problem at all: we simply use the symbol if it + is defined. + + These two lookups need to be handled differently if the + library defines versions. In the case of the old + unversioned application the oldest (default) version + should be used. In case of a dlsym() call the latest and + public interface should be returned. */ + if (verstab != NULL) + { + if ((verstab[symidx] & 0x7fff) + >= ((flags & DL_LOOKUP_RETURN_NEWEST) ? 2 : 3)) + { + /* Don't accept hidden symbols. */ + if ((verstab[symidx] & 0x8000) == 0 + && (*num_versions)++ == 0) + /* No version so far. */ + *versioned_sym = sym; + + return NULL; + } + } + } + + /* There cannot be another entry for this symbol so stop here. */ + return sym; +} + /* Inner part of the lookup functions. We return a value > 0 if we found the symbol, the value 0 if nothing is found and < 0 if @@ -130,105 +241,6 @@ do_lookup_x (const char *undef_name, uint_fast32_t new_hash, const ElfW(Sym) *symtab = (const void *) D_PTR (map, l_info[DT_SYMTAB]); const char *strtab = (const void *) D_PTR (map, l_info[DT_STRTAB]); - - /* Nested routine to check whether the symbol matches. */ - const ElfW(Sym) * - __attribute_noinline__ - check_match (const ElfW(Sym) *sym) - { - unsigned int stt = ELFW(ST_TYPE) (sym->st_info); - assert (ELF_RTYPE_CLASS_PLT == 1); - if (__builtin_expect ((sym->st_value == 0 /* No value. */ - && stt != STT_TLS) - || ELF_MACHINE_SYM_NO_MATCH (sym) - || (type_class & (sym->st_shndx == SHN_UNDEF)), - 0)) - return NULL; - - /* Ignore all but STT_NOTYPE, STT_OBJECT, STT_FUNC, - STT_COMMON, STT_TLS, and STT_GNU_IFUNC since these are no - code/data definitions. */ -#define ALLOWED_STT \ - ((1 << STT_NOTYPE) | (1 << STT_OBJECT) | (1 << STT_FUNC) \ - | (1 << STT_COMMON) | (1 << STT_TLS) | (1 << STT_GNU_IFUNC)) - if (__glibc_unlikely (((1 << stt) & ALLOWED_STT) == 0)) - return NULL; - - if (sym != ref && strcmp (strtab + sym->st_name, undef_name)) - /* Not the symbol we are looking for. */ - return NULL; - - const ElfW(Half) *verstab = map->l_versyms; - if (version != NULL) - { - if (__glibc_unlikely (verstab == NULL)) - { - /* We need a versioned symbol but haven't found any. If - this is the object which is referenced in the verneed - entry it is a bug in the library since a symbol must - not simply disappear. - - It would also be a bug in the object since it means that - the list of required versions is incomplete and so the - tests in dl-version.c haven't found a problem.*/ - assert (version->filename == NULL - || ! _dl_name_match_p (version->filename, map)); - - /* Otherwise we accept the symbol. */ - } - else - { - /* We can match the version information or use the - default one if it is not hidden. */ - ElfW(Half) ndx = verstab[symidx] & 0x7fff; - if ((map->l_versions[ndx].hash != version->hash - || strcmp (map->l_versions[ndx].name, version->name)) - && (version->hidden || map->l_versions[ndx].hash - || (verstab[symidx] & 0x8000))) - /* It's not the version we want. */ - return NULL; - } - } - else - { - /* No specific version is selected. There are two ways we - can got here: - - - a binary which does not include versioning information - is loaded - - - dlsym() instead of dlvsym() is used to get a symbol which - might exist in more than one form - - If the library does not provide symbol version information - there is no problem at all: we simply use the symbol if it - is defined. - - These two lookups need to be handled differently if the - library defines versions. In the case of the old - unversioned application the oldest (default) version - should be used. In case of a dlsym() call the latest and - public interface should be returned. */ - if (verstab != NULL) - { - if ((verstab[symidx] & 0x7fff) - >= ((flags & DL_LOOKUP_RETURN_NEWEST) ? 2 : 3)) - { - /* Don't accept hidden symbols. */ - if ((verstab[symidx] & 0x8000) == 0 - && num_versions++ == 0) - /* No version so far. */ - versioned_sym = sym; - - return NULL; - } - } - } - - /* There cannot be another entry for this symbol so stop here. */ - return sym; - } - const ElfW(Sym) *sym; const ElfW(Addr) *bitmask = map->l_gnu_bitmask; if (__glibc_likely (bitmask != NULL)) @@ -254,7 +266,10 @@ do_lookup_x (const char *undef_name, uint_fast32_t new_hash, if (((*hasharr ^ new_hash) >> 1) == 0) { symidx = hasharr - map->l_gnu_chain_zero; - sym = check_match (&symtab[symidx]); + sym = check_match (undef_name, ref, version, flags, + type_class, &symtab[symidx], symidx, + strtab, map, &versioned_sym, + &num_versions); if (sym != NULL) goto found_it; } @@ -276,7 +291,10 @@ do_lookup_x (const char *undef_name, uint_fast32_t new_hash, symidx != STN_UNDEF; symidx = map->l_chain[symidx]) { - sym = check_match (&symtab[symidx]); + sym = check_match (undef_name, ref, version, flags, + type_class, &symtab[symidx], symidx, + strtab, map, &versioned_sym, + &num_versions); if (sym != NULL) goto found_it; } -- 2.43.5