Remove nested functions from src/strip.c

Remove nested functions from src/strip.c

[tbaeder]

Hi,

here a couple of patches to remove the nested functions from
src/strip.c. I tried to keep the resulting code clean but had to do some
refactorings to get that done. I hope the result is worth considering.
Otherwise, I'm open for suggestions.


- Timm

[PATCH 1/4] strip: Replace nested check_preserved function with loop

[tbaeder]

From: Timm Bäder <tbaeder@redhat.com>

Signed-off-by: Timm Bäder <tbaeder@redhat.com>
---
 src/strip.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/src/strip.c b/src/strip.c
index 7ce14ab8..c971b6c2 100644
--- a/src/strip.c
+++ b/src/strip.c
@@ -1535,25 +1535,30 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 		 files by setting the .debug_data pointer to the original
 		 file's .data pointer.  Below, we'll copy the section
 		 contents.  */
+	      size_t shdr_indices[2] = { shdr_info[cnt].shdr.sh_link, 0 };
+	      int n = 1;
 
-	      inline void check_preserved (size_t i)
-	      {
-		if (i != 0 && i < shnum + 2 && shdr_info[i].idx != 0
-		    && shdr_info[i].debug_data == NULL)
-		  {
-		    if (shdr_info[i].data == NULL)
-		      shdr_info[i].data = elf_getdata (shdr_info[i].scn, NULL);
-		    if (shdr_info[i].data == NULL)
-		      INTERNAL_ERROR (fname);
+	      if (SH_INFO_LINK_P (&shdr_info[cnt].shdr))
+		{
+		  shdr_indices[1] = shdr_info[cnt].shdr.sh_info;
+		  n++;
+		}
 
-		    shdr_info[i].debug_data = shdr_info[i].data;
-		    changes |= i < cnt;
-		  }
-	      }
+	      for (int j = 0; j < n; j++)
+		{
+		  size_t i = shdr_indices[j];
+		  if (i != 0 && i < shnum + 2 && shdr_info[i].idx != 0
+		      && shdr_info[i].debug_data == NULL)
+		    {
+		      if (shdr_info[i].data == NULL)
+			shdr_info[i].data = elf_getdata (shdr_info[i].scn, NULL);
+		      if (shdr_info[i].data == NULL)
+			INTERNAL_ERROR (fname);
 
-	      check_preserved (shdr_info[cnt].shdr.sh_link);
-	      if (SH_INFO_LINK_P (&shdr_info[cnt].shdr))
-		check_preserved (shdr_info[cnt].shdr.sh_info);
+		      shdr_info[i].debug_data = shdr_info[i].data;
+		      changes |= i < cnt;
+		    }
+		}
 	    }
 	}
     }
-- 
2.26.2

Re: [PATCH 1/4] strip: Replace nested check_preserved function with loop

[Mark Wielaard]

Hi Timm,

OK, this simply expands the inlined check_preserved function and keeps
track of the section indexes with the new shdr_indices array that is
populated with the sh_link and possibly the sh_info indices.

Added a ChangeLog entry and pushed.

Thanks,

Mark

[PATCH 2/4] strip: Pull relocate() info file scope

[tbaeder]

From: Timm Bäder <tbaeder@redhat.com>

Pull relocate() info file scope and get rid of a nested function this
way. Refactor remove_debug_relocations() to minimize the parameters we
need to pass to relocate().

Signed-off-by: Timm Bäder <tbaeder@redhat.com>
---
 src/strip.c | 347 ++++++++++++++++++++++++++++------------------------
 1 file changed, 184 insertions(+), 163 deletions(-)

diff --git a/src/strip.c b/src/strip.c
index c971b6c2..71913fac 100644
--- a/src/strip.c
+++ b/src/strip.c
@@ -442,6 +442,127 @@ update_shdrstrndx (Elf *elf, size_t shdrstrndx)
   return 0;
 }
 
+
+/* Apply one relocation.  Returns true when trivial
+   relocation actually done.  */
+static bool
+relocate (Elf *elf, GElf_Addr offset, const GElf_Sxword addend,
+	  Elf_Data *tdata, unsigned int ei_data, const char *fname,
+	  bool is_rela, GElf_Sym *sym, int addsub, Elf_Type type)
+{
+  /* These are the types we can relocate.  */
+#define TYPES   DO_TYPE (BYTE, Byte); DO_TYPE (HALF, Half);		\
+      DO_TYPE (WORD, Word); DO_TYPE (SWORD, Sword);		\
+      DO_TYPE (XWORD, Xword); DO_TYPE (SXWORD, Sxword)
+
+  size_t size;
+
+#define DO_TYPE(NAME, Name) GElf_##Name Name;
+  union { TYPES; } tmpbuf;
+#undef DO_TYPE
+
+  switch (type)
+    {
+#define DO_TYPE(NAME, Name)				\
+      case ELF_T_##NAME:			\
+	size = sizeof (GElf_##Name);	\
+	tmpbuf.Name = 0;			\
+	break;
+      TYPES;
+#undef DO_TYPE
+    default:
+      return false;
+    }
+
+  if (offset > tdata->d_size
+      || tdata->d_size - offset < size)
+    {
+      cleanup_debug ();
+      error (EXIT_FAILURE, 0, _("bad relocation"));
+    }
+
+  /* When the symbol value is zero then for SHT_REL
+     sections this is all that needs to be checked.
+     The addend is contained in the original data at
+     the offset already.  So if the (section) symbol
+     address is zero and the given addend is zero
+     just remove the relocation, it isn't needed
+     anymore.  */
+  if (addend == 0 && sym->st_value == 0)
+    return true;
+
+  Elf_Data tmpdata =
+    {
+      .d_type = type,
+      .d_buf = &tmpbuf,
+      .d_size = size,
+      .d_version = EV_CURRENT,
+    };
+  Elf_Data rdata =
+    {
+      .d_type = type,
+      .d_buf = tdata->d_buf + offset,
+      .d_size = size,
+      .d_version = EV_CURRENT,
+    };
+
+  GElf_Addr value = sym->st_value;
+  if (is_rela)
+    {
+      /* For SHT_RELA sections we just take the
+	 given addend and add it to the value.  */
+      value += addend;
+      /* For ADD/SUB relocations we need to fetch the
+	 current section contents.  */
+      if (addsub != 0)
+	{
+	  Elf_Data *d = gelf_xlatetom (elf, &tmpdata,
+				       &rdata,
+				       ei_data);
+	  if (d == NULL)
+	    INTERNAL_ERROR (fname);
+	  assert (d == &tmpdata);
+	}
+    }
+  else
+    {
+      /* For SHT_REL sections we have to peek at
+	 what is already in the section at the given
+	 offset to get the addend.  */
+      Elf_Data *d = gelf_xlatetom (elf, &tmpdata,
+				   &rdata,
+				   ei_data);
+      if (d == NULL)
+	INTERNAL_ERROR (fname);
+      assert (d == &tmpdata);
+    }
+
+  switch (type)
+    {
+#define DO_TYPE(NAME, Name)					 \
+      case ELF_T_##NAME:				 \
+	if (addsub < 0)				 \
+	  tmpbuf.Name -= (GElf_##Name) value;	 \
+	else					 \
+	  tmpbuf.Name += (GElf_##Name) value;	 \
+	break;
+      TYPES;
+#undef DO_TYPE
+    default:
+      abort ();
+    }
+
+  /* Now finally put in the new value.  */
+  Elf_Data *s = gelf_xlatetof (elf, &rdata,
+			       &tmpdata,
+			       ei_data);
+  if (s == NULL)
+    INTERNAL_ERROR (fname);
+  assert (s == &rdata);
+
+  return true;
+}
+
 /* Remove any relocations between debug sections in ET_REL
    for the debug file when requested.  These relocations are always
    zero based between the unallocated sections.  */
@@ -517,180 +638,80 @@ remove_debug_relocations (Ebl *ebl, Elf *elf, GElf_Ehdr *ehdr,
 	  if (symdata == NULL)
 	    INTERNAL_ERROR (fname);
 
-	  /* Apply one relocation.  Returns true when trivial
-	     relocation actually done.  */
-	  bool relocate (GElf_Addr offset, const GElf_Sxword addend,
-			 bool is_rela, int rtype, int symndx)
-	  {
-	    /* R_*_NONE relocs can always just be removed.  */
-	    if (rtype == 0)
-	      return true;
+	  if (shdr->sh_entsize == 0)
+	    INTERNAL_ERROR (fname);
 
-	    /* We only do simple absolute relocations.  */
-	    int addsub = 0;
-	    Elf_Type type = ebl_reloc_simple_type (ebl, rtype, &addsub);
-	    if (type == ELF_T_NUM)
-	      return false;
+	  size_t nrels = shdr->sh_size / shdr->sh_entsize;
+	  size_t next = 0;
+	  const bool is_rela = (shdr->sh_type == SHT_RELA);
+	  const unsigned int ei_data = ehdr->e_ident[EI_DATA];
 
-	    /* These are the types we can relocate.  */
-#define TYPES   DO_TYPE (BYTE, Byte); DO_TYPE (HALF, Half);		\
-		DO_TYPE (WORD, Word); DO_TYPE (SWORD, Sword);		\
-		DO_TYPE (XWORD, Xword); DO_TYPE (SXWORD, Sxword)
-
-	    /* And only for relocations against other debug sections.  */
-	    GElf_Sym sym_mem;
-	    Elf32_Word xndx;
-	    GElf_Sym *sym = gelf_getsymshndx (symdata, xndxdata,
-					      symndx, &sym_mem,
-					      &xndx);
-	    Elf32_Word sec = (sym->st_shndx == SHN_XINDEX
-			      ? xndx : sym->st_shndx);
-
-	    if (ebl_debugscn_p (ebl, secndx_name (elf, sec)))
-	      {
-		size_t size;
+	  for (size_t relidx = 0; relidx < nrels; ++relidx)
+	    {
+	      int rtype, symndx, offset, addend;
+	      union { GElf_Rela rela; GElf_Rel rel; } mem;
+	      void *rel_p; /* Pointer to either rela or rel above */
 
-#define DO_TYPE(NAME, Name) GElf_##Name Name;
-		union { TYPES; } tmpbuf;
-#undef DO_TYPE
+	      if (is_rela)
+		{
+		  GElf_Rela *r = gelf_getrela (reldata, relidx, &mem.rela);
+		  offset = r->r_offset;
+		  addend = r->r_addend;
+		  rtype = GELF_R_TYPE (r->r_info);
+		  symndx = GELF_R_SYM (r->r_info);
+		  rel_p = r;
+		}
+	      else
+		{
+		  GElf_Rel *r = gelf_getrel (reldata, relidx, &mem.rel);
+		  offset = r->r_offset;
+		  addend = 0;
+		  rtype = GELF_R_TYPE (r->r_info);
+		  symndx = GELF_R_SYM (r->r_info);
+		  rel_p = r;
+		}
 
-		switch (type)
-		  {
-#define DO_TYPE(NAME, Name)				\
-		    case ELF_T_##NAME:			\
-		      size = sizeof (GElf_##Name);	\
-		      tmpbuf.Name = 0;			\
-		      break;
-		    TYPES;
-#undef DO_TYPE
-		  default:
-		    return false;
-		  }
+	      /* R_*_NONE relocs can always just be removed.  */
+	      if (rtype == 0)
+		continue;
 
-		if (offset > tdata->d_size
-		    || tdata->d_size - offset < size)
-		  {
-		    cleanup_debug ();
-		    error (EXIT_FAILURE, 0, _("bad relocation"));
-		  }
+	      /* We only do simple absolute relocations.  */
+	      int addsub = 0;
+	      Elf_Type type = ebl_reloc_simple_type (ebl, rtype, &addsub);
+	      if (type == ELF_T_NUM)
+		goto relocate_failed;
 
-		/* When the symbol value is zero then for SHT_REL
-		   sections this is all that needs to be checked.
-		   The addend is contained in the original data at
-		   the offset already.  So if the (section) symbol
-		   address is zero and the given addend is zero
-		   just remove the relocation, it isn't needed
-		   anymore.  */
-		if (addend == 0 && sym->st_value == 0)
-		  return true;
-
-		Elf_Data tmpdata =
-		  {
-		    .d_type = type,
-		    .d_buf = &tmpbuf,
-		    .d_size = size,
-		    .d_version = EV_CURRENT,
-		  };
-		Elf_Data rdata =
-		  {
-		    .d_type = type,
-		    .d_buf = tdata->d_buf + offset,
-		    .d_size = size,
-		    .d_version = EV_CURRENT,
-		  };
-
-		GElf_Addr value = sym->st_value;
-		if (is_rela)
-		  {
-		    /* For SHT_RELA sections we just take the
-		       given addend and add it to the value.  */
-		    value += addend;
-		    /* For ADD/SUB relocations we need to fetch the
-		       current section contents.  */
-		    if (addsub != 0)
-		      {
-			Elf_Data *d = gelf_xlatetom (elf, &tmpdata,
-						     &rdata,
-						     ehdr->e_ident[EI_DATA]);
-			if (d == NULL)
-			  INTERNAL_ERROR (fname);
-			assert (d == &tmpdata);
-		      }
-		  }
-		else
-		  {
-		    /* For SHT_REL sections we have to peek at
-		       what is already in the section at the given
-		       offset to get the addend.  */
-		    Elf_Data *d = gelf_xlatetom (elf, &tmpdata,
-						 &rdata,
-						 ehdr->e_ident[EI_DATA]);
-		    if (d == NULL)
-		      INTERNAL_ERROR (fname);
-		    assert (d == &tmpdata);
-		  }
+	      /* And only for relocations against other debug sections.  */
+	      GElf_Sym sym_mem;
+	      Elf32_Word xndx;
+	      GElf_Sym *sym = gelf_getsymshndx (symdata, xndxdata,
+						symndx, &sym_mem,
+						  &xndx);
+	      Elf32_Word sec = (sym->st_shndx == SHN_XINDEX
+				? xndx : sym->st_shndx);
 
-		switch (type)
-		  {
-#define DO_TYPE(NAME, Name)					 \
-		    case ELF_T_##NAME:				 \
-		      if (addsub < 0)				 \
-			tmpbuf.Name -= (GElf_##Name) value;	 \
-		      else					 \
-			tmpbuf.Name += (GElf_##Name) value;	 \
-		      break;
-		    TYPES;
-#undef DO_TYPE
-		  default:
-		    abort ();
-		  }
+	      bool dbg_scn = ebl_debugscn_p (ebl, secndx_name (elf, sec));
 
-		/* Now finally put in the new value.  */
-		Elf_Data *s = gelf_xlatetof (elf, &rdata,
-					     &tmpdata,
-					     ehdr->e_ident[EI_DATA]);
-		if (s == NULL)
-		  INTERNAL_ERROR (fname);
-		assert (s == &rdata);
+	      if (!dbg_scn)
+		goto relocate_failed;
 
-		return true;
-	      }
-	    return false;
-	  }
+	      if (! relocate (elf, offset, addend,
+			    tdata, ei_data, fname, is_rela,
+			    sym, addsub, type))
+	      goto relocate_failed;
 
-	  if (shdr->sh_entsize == 0)
-	    INTERNAL_ERROR (fname);
+	      continue; /* Next */
 
-	  size_t nrels = shdr->sh_size / shdr->sh_entsize;
-	  size_t next = 0;
-	  if (shdr->sh_type == SHT_REL)
-	    for (size_t relidx = 0; relidx < nrels; ++relidx)
-	      {
-		GElf_Rel rel_mem;
-		GElf_Rel *r = gelf_getrel (reldata, relidx, &rel_mem);
-		if (! relocate (r->r_offset, 0, false,
-				GELF_R_TYPE (r->r_info),
-				GELF_R_SYM (r->r_info)))
-		  {
-		    if (relidx != next)
-		      gelf_update_rel (reldata, next, r);
-		    ++next;
-		  }
-	      }
-	  else
-	    for (size_t relidx = 0; relidx < nrels; ++relidx)
-	      {
-		GElf_Rela rela_mem;
-		GElf_Rela *r = gelf_getrela (reldata, relidx, &rela_mem);
-		if (! relocate (r->r_offset, r->r_addend, true,
-				GELF_R_TYPE (r->r_info),
-				GELF_R_SYM (r->r_info)))
-		  {
-		    if (relidx != next)
-		      gelf_update_rela (reldata, next, r);
-		    ++next;
-		  }
-	      }
+relocate_failed:
+	      if (relidx != next)
+		{
+		  if (is_rela)
+		    gelf_update_rela (reldata, next, rel_p);
+		  else
+		    gelf_update_rel (reldata, next, rel_p);
+		}
+	      ++next;
+	    }
 
 	  nrels = next;
 	  shdr->sh_size = reldata->d_size = nrels * shdr->sh_entsize;
-- 
2.26.2

Re: [PATCH 2/4] strip: Pull relocate() info file scope

[Mark Wielaard]

Hi Timm,

On Fri, Jan 08, 2021 at 09:04:47AM +0100, Timm Bäder via Elfutils-devel wrote:
> Pull relocate() info file scope and get rid of a nested function this
> way. Refactor remove_debug_relocations() to minimize the parameters we
> need to pass to relocate().

OK. The diff makes it slightly hard to see whether things were
moved/updated correctly, but if we missed something I am sure the
buildbots will catch it. I added a ChangeLog entry and pushed.

Cheers,

Mark

[PATCH 3/4] strip: Pull update_section_size() into file scope

[tbaeder]

From: Timm Bäder <tbaeder@redhat.com>

Get rid of a nested function this way.

Signed-off-by: Timm Bäder <tbaeder@redhat.com>
---
 src/strip.c | 51 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/src/strip.c b/src/strip.c
index 71913fac..e608dc5e 100644
--- a/src/strip.c
+++ b/src/strip.c
@@ -939,6 +939,31 @@ handle_debug_relocs (Elf *elf, Ebl *ebl, Elf *new_elf,
   return 0;
 }
 
+/* Update section headers when the data size has changed.
+   We also update the SHT_NOBITS section in the debug
+   file so that the section headers match in sh_size.  */
+static inline void
+update_section_size (Elf_Scn *scn,
+		     const Elf_Data *newdata,
+		     Elf *debugelf,
+		     size_t cnt,
+		     const char *fname)
+{
+  GElf_Shdr shdr_mem;
+  GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);
+  shdr->sh_size = newdata->d_size;
+  (void) gelf_update_shdr (scn, shdr);
+  if (debugelf != NULL)
+    {
+      /* libelf will use d_size to set sh_size.  */
+      Elf_Data *debugdata = elf_getdata (elf_getscn (debugelf,
+						     cnt), NULL);
+      if (debugdata == NULL)
+	INTERNAL_ERROR (fname);
+      debugdata->d_size = newdata->d_size;
+    }
+}
+
 /* Maximum size of array allocated on stack.  */
 #define MAX_STACK_ALLOC	(400 * 1024)
 
@@ -2150,26 +2175,6 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
     /* Find all relocation sections which use this symbol table.  */
     for (cnt = 1; cnt <= shdridx; ++cnt)
       {
-	/* Update section headers when the data size has changed.
-	   We also update the SHT_NOBITS section in the debug
-	   file so that the section headers match in sh_size.  */
-	inline void update_section_size (const Elf_Data *newdata)
-	{
-	  GElf_Shdr shdr_mem;
-	  GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);
-	  shdr->sh_size = newdata->d_size;
-	  (void) gelf_update_shdr (scn, shdr);
-	  if (debugelf != NULL)
-	    {
-	      /* libelf will use d_size to set sh_size.  */
-	      Elf_Data *debugdata = elf_getdata (elf_getscn (debugelf,
-							     cnt), NULL);
-	      if (debugdata == NULL)
-		INTERNAL_ERROR (fname);
-	      debugdata->d_size = newdata->d_size;
-	    }
-	}
-
 	if (shdr_info[cnt].idx == 0 && debug_fname == NULL)
 	  /* Ignore sections which are discarded.  When we are saving a
 	     relocation section in a separate debug file, we must fix up
@@ -2299,7 +2304,7 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 				 * sizeof (Elf32_Word));
 		elf_assert (n_size <= hashd->d_size);
 		hashd->d_size = n_size;
-		update_section_size (hashd);
+		update_section_size (scn, hashd, debugelf, cnt, fname);
 
 		/* Clear the arrays.  */
 		memset (bucket, '\0',
@@ -2361,7 +2366,7 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 				 * sizeof (Elf64_Xword));
 		elf_assert (n_size <= hashd->d_size);
 		hashd->d_size = n_size;
-		update_section_size (hashd);
+		update_section_size (scn, hashd, debugelf, cnt, fname);
 
 		/* Clear the arrays.  */
 		memset (bucket, '\0',
@@ -2435,7 +2440,7 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 				       / gelf_fsize (elf, symd->d_type, 1,
 						     EV_CURRENT),
 				       EV_CURRENT);
-	    update_section_size (verd);
+	    update_section_size (scn, verd, debugelf, cnt, fname);
 	    break;
 
 	  case SHT_GROUP:
-- 
2.26.2

Re: [PATCH 3/4] strip: Pull update_section_size() into file scope

[Mark Wielaard]

Hi Timm,

On Fri, Jan 08, 2021 at 09:04:48AM +0100, Timm Bäder via Elfutils-devel wrote:
> Get rid of a nested function this way.

OK, it is a little messy IMHO that we have to pass 4 extra arguments
to the function now, but it seems straightforward. Added a ChangeLog
entry and pushed.

Thanks,

Mark

[PATCH 4/4] strip: Remove no_symtab_updates() function

[tbaeder]

From: Timm Bäder <tbaeder@redhat.com>

The no_symtab_updates() function was being called at the beginning of
all case labels in this switch, so we can just call it once before the
switch. Then it only has one call-site, so inline this short function
there.

Signed-off-by: Timm Bäder <tbaeder@redhat.com>
---
 src/strip.c | 152 ++++++++++++++++++++++++----------------------------
 1 file changed, 69 insertions(+), 83 deletions(-)

diff --git a/src/strip.c b/src/strip.c
index e608dc5e..dd1e27ac 100644
--- a/src/strip.c
+++ b/src/strip.c
@@ -2175,98 +2175,91 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
     /* Find all relocation sections which use this symbol table.  */
     for (cnt = 1; cnt <= shdridx; ++cnt)
       {
-	if (shdr_info[cnt].idx == 0 && debug_fname == NULL)
+	struct shdr_info *info = &shdr_info[cnt];
+	if (info->idx == 0 && debug_fname == NULL)
 	  /* Ignore sections which are discarded.  When we are saving a
 	     relocation section in a separate debug file, we must fix up
 	     the symbol table references.  */
 	  continue;
 
-	const Elf32_Word symtabidx = shdr_info[cnt].old_sh_link;
+	const Elf32_Word symtabidx = info->old_sh_link;
 	elf_assert (symtabidx < shnum + 2);
 	const Elf32_Word *const newsymidx = shdr_info[symtabidx].newsymidx;
-	switch (shdr_info[cnt].shdr.sh_type)
-	  {
-	    inline bool no_symtab_updates (void)
-	    {
-	      /* If the symbol table hasn't changed, do not do anything.  */
-	      if (shdr_info[symtabidx].newsymidx == NULL)
-		return true;
-
-	      /* If the symbol table is not discarded, but additionally
-		 duplicated in the separate debug file and this section
-		 is discarded, don't adjust anything.  */
-	      return (shdr_info[cnt].idx == 0
-		      && shdr_info[symtabidx].debug_data != NULL);
-	    }
 
+	/* If the symbol table hasn't changed, do not do anything.  */
+	if (newsymidx == NULL)
+	  continue;
+
+	/* If the symbol table is not discarded, but additionally
+	   duplicated in the separate debug file and this section
+	   is discarded, don't adjust anything.  */
+	if (info->idx == 0 && shdr_info[symtabidx].debug_data != NULL)
+	  continue;
+
+	switch (info->shdr.sh_type)
+	  {
 	  case SHT_REL:
 	  case SHT_RELA:
-	    if (no_symtab_updates ())
-	      break;
-
-	    Elf_Data *d = elf_getdata (shdr_info[cnt].idx == 0
-				       ? elf_getscn (debugelf, cnt)
-				       : elf_getscn (newelf,
-						     shdr_info[cnt].idx),
-				       NULL);
-	    elf_assert (d != NULL && d->d_buf != NULL
-			&& shdr_info[cnt].shdr.sh_entsize != 0);
-	    size_t nrels = (shdr_info[cnt].shdr.sh_size
-			    / shdr_info[cnt].shdr.sh_entsize);
-
-	    size_t symsize = gelf_fsize (elf, ELF_T_SYM, 1, EV_CURRENT);
-	    const Elf32_Word symidxn = (shdr_info[symtabidx].data->d_size
-					/ symsize);
-	    if (shdr_info[cnt].shdr.sh_type == SHT_REL)
-	      for (size_t relidx = 0; relidx < nrels; ++relidx)
-		{
-		  GElf_Rel rel_mem;
-		  if (gelf_getrel (d, relidx, &rel_mem) == NULL)
-		    INTERNAL_ERROR (fname);
+	    {
+	      Elf_Data *d = elf_getdata (info->idx == 0
+					 ? elf_getscn (debugelf, cnt)
+					 : elf_getscn (newelf, info->idx),
+					 NULL);
+	      elf_assert (d != NULL && d->d_buf != NULL
+			  && info->shdr.sh_entsize != 0);
+	      size_t nrels = (info->shdr.sh_size / info->shdr.sh_entsize);
+
+	      size_t symsize = gelf_fsize (elf, ELF_T_SYM, 1, EV_CURRENT);
+	      const Elf32_Word symidxn = (shdr_info[symtabidx].data->d_size
+					  / symsize);
+	      if (info->shdr.sh_type == SHT_REL)
+		for (size_t relidx = 0; relidx < nrels; ++relidx)
+		  {
+		    GElf_Rel rel_mem;
+		    if (gelf_getrel (d, relidx, &rel_mem) == NULL)
+		      INTERNAL_ERROR (fname);
 
-		  size_t symidx = GELF_R_SYM (rel_mem.r_info);
-		  elf_assert (symidx < symidxn);
-		  if (newsymidx[symidx] != symidx)
-		    {
-		      rel_mem.r_info
-			= GELF_R_INFO (newsymidx[symidx],
-				       GELF_R_TYPE (rel_mem.r_info));
+		    size_t symidx = GELF_R_SYM (rel_mem.r_info);
+		    elf_assert (symidx < symidxn);
+		    if (newsymidx[symidx] != symidx)
+		      {
+			rel_mem.r_info
+			  = GELF_R_INFO (newsymidx[symidx],
+					 GELF_R_TYPE (rel_mem.r_info));
 
-		      if (gelf_update_rel (d, relidx, &rel_mem) == 0)
-			INTERNAL_ERROR (fname);
-		    }
-		}
-	    else
-	      for (size_t relidx = 0; relidx < nrels; ++relidx)
-		{
-		  GElf_Rela rel_mem;
-		  if (gelf_getrela (d, relidx, &rel_mem) == NULL)
-		    INTERNAL_ERROR (fname);
+			if (gelf_update_rel (d, relidx, &rel_mem) == 0)
+			  INTERNAL_ERROR (fname);
+		      }
+		  }
+	      else
+		for (size_t relidx = 0; relidx < nrels; ++relidx)
+		  {
+		    GElf_Rela rel_mem;
+		    if (gelf_getrela (d, relidx, &rel_mem) == NULL)
+		      INTERNAL_ERROR (fname);
 
-		  size_t symidx = GELF_R_SYM (rel_mem.r_info);
-		  elf_assert (symidx < symidxn);
-		  if (newsymidx[symidx] != symidx)
-		    {
-		      rel_mem.r_info
-			= GELF_R_INFO (newsymidx[symidx],
-				       GELF_R_TYPE (rel_mem.r_info));
+		    size_t symidx = GELF_R_SYM (rel_mem.r_info);
+		    elf_assert (symidx < symidxn);
+		    if (newsymidx[symidx] != symidx)
+		      {
+			rel_mem.r_info
+			  = GELF_R_INFO (newsymidx[symidx],
+					 GELF_R_TYPE (rel_mem.r_info));
 
-		      if (gelf_update_rela (d, relidx, &rel_mem) == 0)
-			INTERNAL_ERROR (fname);
-		    }
-		}
+			if (gelf_update_rela (d, relidx, &rel_mem) == 0)
+			  INTERNAL_ERROR (fname);
+		      }
+		  }
+	      }
 	    break;
 
 	  case SHT_HASH:
-	    if (no_symtab_updates ())
-	      break;
-
 	    /* We have to recompute the hash table.  */
 
-	    elf_assert (shdr_info[cnt].idx > 0);
+	    elf_assert (info->idx > 0);
 
 	    /* The hash section in the new file.  */
-	    scn = elf_getscn (newelf, shdr_info[cnt].idx);
+	    scn = elf_getscn (newelf, info->idx);
 
 	    /* The symbol table data.  */
 	    Elf_Data *symd = elf_getdata (elf_getscn (newelf,
@@ -2278,7 +2271,7 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 	    Elf_Data *hashd = elf_getdata (scn, NULL);
 	    elf_assert (hashd != NULL && hashd->d_buf != NULL);
 
-	    if (shdr_info[cnt].shdr.sh_entsize == sizeof (Elf32_Word))
+	    if (info->shdr.sh_entsize == sizeof (Elf32_Word))
 	      {
 		/* Sane arches first.  */
 		elf_assert (hashd->d_size >= 2 * sizeof (Elf32_Word));
@@ -2339,8 +2332,7 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 	    else
 	      {
 		/* Alpha and S390 64-bit use 64-bit SHT_HASH entries.  */
-		elf_assert (shdr_info[cnt].shdr.sh_entsize
-			    == sizeof (Elf64_Xword));
+		elf_assert (info->shdr.sh_entsize == sizeof (Elf64_Xword));
 
 		Elf64_Xword *bucket = (Elf64_Xword *) hashd->d_buf;
 
@@ -2402,13 +2394,10 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 
 	  case SHT_GNU_versym:
 	    /* If the symbol table changed we have to adjust the entries.  */
-	    if (no_symtab_updates ())
-	      break;
-
-	    elf_assert (shdr_info[cnt].idx > 0);
+	    elf_assert (info->idx > 0);
 
 	    /* The symbol version section in the new file.  */
-	    scn = elf_getscn (newelf, shdr_info[cnt].idx);
+	    scn = elf_getscn (newelf, info->idx);
 
 	    /* The symbol table data.  */
 	    symd = elf_getdata (elf_getscn (newelf, shdr_info[symtabidx].idx),
@@ -2444,12 +2433,9 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 	    break;
 
 	  case SHT_GROUP:
-	    if (no_symtab_updates ())
-	      break;
-
 	    /* Yes, the symbol table changed.
 	       Update the section header of the section group.  */
-	    scn = elf_getscn (newelf, shdr_info[cnt].idx);
+	    scn = elf_getscn (newelf, info->idx);
 	    GElf_Shdr shdr_mem;
 	    GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);
 	    elf_assert (shdr != NULL);
-- 
2.26.2

Re: [PATCH 4/4] strip: Remove no_symtab_updates() function

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]

Hi Timm,

On Fri, Jan 08, 2021 at 09:04:49AM +0100, Timm Bäder via Elfutils-devel wrote:
> The no_symtab_updates() function was being called at the beginning of
> all case labels in this switch, so we can just call it once before the
> switch. Then it only has one call-site, so inline this short function
> there.

Yes, this is actually nicer to read.  I added a ChangeLog entry and
made one tiny change so we don't need to add brackets to scope the
case label, preventing extra indentation which made the patch harder
to read.

Pushed as attached,

Mark

[-- Attachment #2: 0001-strip-Remove-no_symtab_updates-function.patch --]
[-- Type: text/x-diff, Size: 6441 bytes --]

From e29965f401896a3652394df8e28cc14979fedc91 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder@redhat.com>
Date: Fri, 8 Jan 2021 09:04:49 +0100
Subject: [PATCH] strip: Remove no_symtab_updates() function
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The no_symtab_updates() function was being called at the beginning of
all case labels in this switch, so we can just call it once before the
switch. Then it only has one call-site, so inline this short function
there.

Signed-off-by: Timm Bäder <tbaeder@redhat.com>
---
 src/ChangeLog |  6 +++++
 src/strip.c   | 74 ++++++++++++++++++++-------------------------------
 2 files changed, 35 insertions(+), 45 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index d1d9a8bf..4fc54ce7 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,9 @@
+2021-01-08  Timm Bäder  <tbaeder@redhat.com>
+
+	* strip.c (handle_elf): Remove no_symtab_updates function and
+	calls inside the switch statement. Add checks and (possibly)
+	continue, before switch statement is called.
+
 2021-01-08  Timm Bäder  <tbaeder@redhat.com>
 
 	* strip.c (handle_elf): Move inlined update_section_size function
diff --git a/src/strip.c b/src/strip.c
index e608dc5e..7a5d4e4c 100644
--- a/src/strip.c
+++ b/src/strip.c
@@ -2175,49 +2175,43 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
     /* Find all relocation sections which use this symbol table.  */
     for (cnt = 1; cnt <= shdridx; ++cnt)
       {
-	if (shdr_info[cnt].idx == 0 && debug_fname == NULL)
+	struct shdr_info *info = &shdr_info[cnt];
+	if (info->idx == 0 && debug_fname == NULL)
 	  /* Ignore sections which are discarded.  When we are saving a
 	     relocation section in a separate debug file, we must fix up
 	     the symbol table references.  */
 	  continue;
 
-	const Elf32_Word symtabidx = shdr_info[cnt].old_sh_link;
+	const Elf32_Word symtabidx = info->old_sh_link;
 	elf_assert (symtabidx < shnum + 2);
 	const Elf32_Word *const newsymidx = shdr_info[symtabidx].newsymidx;
-	switch (shdr_info[cnt].shdr.sh_type)
-	  {
-	    inline bool no_symtab_updates (void)
-	    {
-	      /* If the symbol table hasn't changed, do not do anything.  */
-	      if (shdr_info[symtabidx].newsymidx == NULL)
-		return true;
-
-	      /* If the symbol table is not discarded, but additionally
-		 duplicated in the separate debug file and this section
-		 is discarded, don't adjust anything.  */
-	      return (shdr_info[cnt].idx == 0
-		      && shdr_info[symtabidx].debug_data != NULL);
-	    }
 
+	/* If the symbol table hasn't changed, do not do anything.  */
+	if (newsymidx == NULL)
+	  continue;
+
+	/* If the symbol table is not discarded, but additionally
+	   duplicated in the separate debug file and this section
+	   is discarded, don't adjust anything.  */
+	if (info->idx == 0 && shdr_info[symtabidx].debug_data != NULL)
+	  continue;
+
+	switch (info->shdr.sh_type)
+	  {
 	  case SHT_REL:
 	  case SHT_RELA:
-	    if (no_symtab_updates ())
-	      break;
-
-	    Elf_Data *d = elf_getdata (shdr_info[cnt].idx == 0
-				       ? elf_getscn (debugelf, cnt)
-				       : elf_getscn (newelf,
-						     shdr_info[cnt].idx),
-				       NULL);
+	    scn = (info->idx == 0
+		   ? elf_getscn (debugelf, cnt)
+		   : elf_getscn (newelf, info->idx));
+	    Elf_Data *d = elf_getdata (scn, NULL);
 	    elf_assert (d != NULL && d->d_buf != NULL
-			&& shdr_info[cnt].shdr.sh_entsize != 0);
-	    size_t nrels = (shdr_info[cnt].shdr.sh_size
-			    / shdr_info[cnt].shdr.sh_entsize);
+			&& info->shdr.sh_entsize != 0);
+	    size_t nrels = (info->shdr.sh_size / info->shdr.sh_entsize);
 
 	    size_t symsize = gelf_fsize (elf, ELF_T_SYM, 1, EV_CURRENT);
 	    const Elf32_Word symidxn = (shdr_info[symtabidx].data->d_size
 					/ symsize);
-	    if (shdr_info[cnt].shdr.sh_type == SHT_REL)
+	    if (info->shdr.sh_type == SHT_REL)
 	      for (size_t relidx = 0; relidx < nrels; ++relidx)
 		{
 		  GElf_Rel rel_mem;
@@ -2258,15 +2252,12 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 	    break;
 
 	  case SHT_HASH:
-	    if (no_symtab_updates ())
-	      break;
-
 	    /* We have to recompute the hash table.  */
 
-	    elf_assert (shdr_info[cnt].idx > 0);
+	    elf_assert (info->idx > 0);
 
 	    /* The hash section in the new file.  */
-	    scn = elf_getscn (newelf, shdr_info[cnt].idx);
+	    scn = elf_getscn (newelf, info->idx);
 
 	    /* The symbol table data.  */
 	    Elf_Data *symd = elf_getdata (elf_getscn (newelf,
@@ -2278,7 +2269,7 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 	    Elf_Data *hashd = elf_getdata (scn, NULL);
 	    elf_assert (hashd != NULL && hashd->d_buf != NULL);
 
-	    if (shdr_info[cnt].shdr.sh_entsize == sizeof (Elf32_Word))
+	    if (info->shdr.sh_entsize == sizeof (Elf32_Word))
 	      {
 		/* Sane arches first.  */
 		elf_assert (hashd->d_size >= 2 * sizeof (Elf32_Word));
@@ -2339,8 +2330,7 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 	    else
 	      {
 		/* Alpha and S390 64-bit use 64-bit SHT_HASH entries.  */
-		elf_assert (shdr_info[cnt].shdr.sh_entsize
-			    == sizeof (Elf64_Xword));
+		elf_assert (info->shdr.sh_entsize == sizeof (Elf64_Xword));
 
 		Elf64_Xword *bucket = (Elf64_Xword *) hashd->d_buf;
 
@@ -2402,13 +2392,10 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 
 	  case SHT_GNU_versym:
 	    /* If the symbol table changed we have to adjust the entries.  */
-	    if (no_symtab_updates ())
-	      break;
-
-	    elf_assert (shdr_info[cnt].idx > 0);
+	    elf_assert (info->idx > 0);
 
 	    /* The symbol version section in the new file.  */
-	    scn = elf_getscn (newelf, shdr_info[cnt].idx);
+	    scn = elf_getscn (newelf, info->idx);
 
 	    /* The symbol table data.  */
 	    symd = elf_getdata (elf_getscn (newelf, shdr_info[symtabidx].idx),
@@ -2444,12 +2431,9 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 	    break;
 
 	  case SHT_GROUP:
-	    if (no_symtab_updates ())
-	      break;
-
 	    /* Yes, the symbol table changed.
 	       Update the section header of the section group.  */
-	    scn = elf_getscn (newelf, shdr_info[cnt].idx);
+	    scn = elf_getscn (newelf, info->idx);
 	    GElf_Shdr shdr_mem;
 	    GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);
 	    elf_assert (shdr != NULL);
-- 
2.20.1