[WIP] Remove nested function in elf/dl-lookup.c? (Nested functions considered harmful?)

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

Is it me or does reading do_lookup_x get very very complicated
with the nested function check_match which has access to all
sorts of function local variables?

I'm always suspicious of nested functions like this as bad
programming form since it complicates reading the function.

Is anyone opposed to rewriting this with a non-nested function
that clearly shows which arguments are constant, and which are
modified?

Cheers,
Carlos.

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 f869dcf..91f5212 100644
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@@ -68,6 +68,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 (__builtin_expect (((1 << stt) & ALLOWED_STT) == 0, 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 (__builtin_expect (verstab == NULL, 0))
+        {
+	/* 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
@@ -123,106 +233,9 @@ 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)
-			      || (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 (__builtin_expect (((1 << stt) & ALLOWED_STT) == 0, 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 (__builtin_expect (verstab == NULL, 0))
-	      {
-		/* 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 (__builtin_expect (bitmask != NULL, 1))
 	{
 	  ElfW(Addr) bitmask_word
@@ -246,7 +259,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;
 		      }
@@ -268,7 +284,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;
 	    }
---