[PATCH] Remove nested function in elf/dl-lookup.c.

["Carlos O'Donell" <carlos at redhat dot com>]

This patch moves check_match out from being a nested function
to being a normal function.

This is a follow-on to my WIP patch here:
https://www.sourceware.org/ml/libc-alpha/2013-12/msg00559.html

The generated code is *almost* identical but in this version the
check_match function is 33 bytes larger because of the additional
arguments that are no longer implied. This answer's Ondrej's
question, in that this isn't just a strict refactoring since the
inline function had direct access to locals while the function
doesn't and even inlining both doesn't produce exactly the same
code (though it is similar).

I've had to debug this code several times now and I find the
real function version both easier to debug, modify, and understand.
It provides a narrower interface for the match checking function
and clearly marks which data is constant and which is not.

In general I'm opposed to inline function like this since my
opinion is that they make the code more difficult to read and can
have unitended side-effects more easily than a normal function.

I've been testing this for 2.19, and on x86-64 and x86 with no
problems (as this is mostly a mechanical refactoring).

OK to checkin?

2013-12-14  Carlos O'Donell  <carlos@redhat.com>

	* elf/dl-lookup.c (check_match): New function.
	(do_lookup_x): Remove nested function check_match.
	Use non-nested function check_match.

diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
index 0b43db8..d64609e 100644
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@@ -75,6 +75,116 @@ 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)
+			|| (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 get 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 +240,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 +265,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 +290,11 @@ 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;
 	    }
---

Cheers,
Carlos.