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.
12 years ago · 8e25d1e772
2 changed files with 129 additions and 104 deletions
--- a/7
+++ b/7
@ -1,3 +1,10 @@
+2014-02-28  Carlos O'Donell  <carlos@redhat.com>
+
+	* 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  <roland@hack.frob.com>

 	* csu/Makefile (generated, before-compile): Use += rather than =.
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@ -31,9 +31,8 @@

 #include <assert.h>

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