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

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

On 02/12/2014 03:50 PM, Joseph S. Myers wrote:
> This patch loses the call to ELF_MACHINE_SYM_NO_MATCH that was previously 
> in check_match, and also needs to update the comment on the default 
> definition of ELF_MACHINE_SYM_NO_MATCH to reflect that check_match is no 
> longer a nested function.

I have thrown away the existing refactoring and started again,
and then compared my new version to the old version to ensure
that previous testing would still be valid.

This should catch any changes since the last time I refactored.
I had included Ondrej's changes but had not noticed your changes.
Git had given me a merge warning for Ondrej's changes for
__glibc_*likely, but not yours, perhaps I did something wrong there.

Either way I've rested the following and it looks good.

OK?

v2
- Fixed ELF_MACHINE_SYM_NO_MATCH comment.
- Added ELF_MACHINE_SYM_NO_MATCH check.

2013-02-12  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.

diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
index 0b43db8..d1b8efc 100644
--- 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;
 	    }
---

Cheers,
Carlos.