Remove nested functions from elf-from-memory.c

Remove nested functions from elf-from-memory.c

[tbaeder]

Hey,

these are relatively straight-forward I think. Let me know if that's not
the case. :)


- Timm

[PATCH 1/2] elf-from-memory: Restructure code to get rid of nested handle_segment()

[tbaeder]

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

Use one loop for both 32 and 64  bit case. This allows for only one call
site of the old handle_segment(), which we can then inline into the for
loop.

Signed-off-by: Timm Bäder <tbaeder@redhat.com>
---
 libdwfl/elf-from-memory.c | 81 ++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 47 deletions(-)

diff --git a/libdwfl/elf-from-memory.c b/libdwfl/elf-from-memory.c
index c54c1b99..933ab864 100644
--- a/libdwfl/elf-from-memory.c
+++ b/libdwfl/elf-from-memory.c
@@ -223,61 +223,48 @@ elf_from_remote_memory (GElf_Addr ehdr_vma,
   bool found_base = false;
   Elf32_Phdr (*p32)[phnum] = phdrsp;
   Elf64_Phdr (*p64)[phnum] = phdrsp;
-  switch (ehdr.e32.e_ident[EI_CLASS])
+
+  if (class32)
     {
-      /* Sanity checks segments and calculates segment_end,
-	 segments_end, segments_end_mem and loadbase (if not
-	 found_base yet).  Returns true if sanity checking failed,
-	 false otherwise.  */
-      inline bool handle_segment (GElf_Addr vaddr, GElf_Off offset,
-				  GElf_Xword filesz, GElf_Xword memsz)
-	{
-	  /* Sanity check the segment load aligns with the pagesize.  */
-	  if (((vaddr - offset) & (pagesize - 1)) != 0)
-	    return true;
+      if (! elf32_xlatetom (&xlateto, &xlatefrom, ehdr.e32.e_ident[EI_DATA]))
+        goto libelf_error;
+    }
+  else
+    {
+      if (! elf64_xlatetom (&xlateto, &xlatefrom, ehdr.e64.e_ident[EI_DATA]))
+        goto libelf_error;
+    }
 
-	  GElf_Off segment_end = ((offset + filesz + pagesize - 1)
-				  & -pagesize);
+  for (uint_fast16_t i = 0; i < phnum; ++i)
+    {
+      GElf_Word type = class32 ? (*p32)[i].p_type : (*p64)[i].p_type;
 
-	  if (segment_end > (GElf_Off) contents_size)
-	    contents_size = segment_end;
+      if (type != PT_LOAD)
+        continue;
 
-	  if (!found_base && (offset & -pagesize) == 0)
-	    {
-	      loadbase = ehdr_vma - (vaddr & -pagesize);
-	      found_base = true;
-	    }
+      GElf_Addr vaddr = class32 ? (*p32)[i].p_vaddr : (*p64)[i].p_vaddr;
+      GElf_Xword memsz = class32 ? (*p32)[i].p_memsz : (*p64)[i].p_memsz;
+      GElf_Off offset = class32 ? (*p32)[i].p_offset : (*p64)[i].p_offset;
+      GElf_Xword filesz = class32 ? (*p32)[i].p_filesz : (*p64)[i].p_filesz;
 
-	  segments_end = offset + filesz;
-	  segments_end_mem = offset + memsz;
-	  return false;
-	}
+      /* Sanity check the segment load aligns with the pagesize.  */
+      if (((vaddr - offset) & (pagesize - 1)) != 0)
+        goto bad_elf;
 
-    case ELFCLASS32:
-      if (elf32_xlatetom (&xlateto, &xlatefrom,
-			  ehdr.e32.e_ident[EI_DATA]) == NULL)
-	goto libelf_error;
-      for (uint_fast16_t i = 0; i < phnum; ++i)
-	if ((*p32)[i].p_type == PT_LOAD)
-	  if (handle_segment ((*p32)[i].p_vaddr, (*p32)[i].p_offset,
-			      (*p32)[i].p_filesz, (*p32)[i].p_memsz))
-	    goto bad_elf;
-      break;
+      GElf_Off segment_end = ((offset + filesz + pagesize - 1)
+                              & -pagesize);
 
-    case ELFCLASS64:
-      if (elf64_xlatetom (&xlateto, &xlatefrom,
-			  ehdr.e64.e_ident[EI_DATA]) == NULL)
-	goto libelf_error;
-      for (uint_fast16_t i = 0; i < phnum; ++i)
-	if ((*p64)[i].p_type == PT_LOAD)
-	  if (handle_segment ((*p64)[i].p_vaddr, (*p64)[i].p_offset,
-			      (*p64)[i].p_filesz, (*p64)[i].p_memsz))
-	    goto bad_elf;
-      break;
+      if (segment_end > (GElf_Off) contents_size)
+        contents_size = segment_end;
 
-    default:
-      abort ();
-      break;
+      if (!found_base && (offset & -pagesize) == 0)
+        {
+          loadbase = ehdr_vma - (vaddr & -pagesize);
+          found_base = true;
+        }
+
+      segments_end = offset + filesz;
+      segments_end_mem = offset + memsz;
     }
 
   /* Trim the last segment so we don't bother with zeros in the last page
-- 
2.26.2

[PATCH 2/2] elf-from-memory: Refactor to get rid of nested function

[tbaeder]

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

Try to unify the 32/64 bit code paths and get rid of the nested
handle_segment() this way.

Signed-off-by: Timm Bäder <tbaeder@redhat.com>
---
 libdwfl/elf-from-memory.c | 115 +++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 65 deletions(-)

diff --git a/libdwfl/elf-from-memory.c b/libdwfl/elf-from-memory.c
index 933ab864..a0ef0014 100644
--- a/libdwfl/elf-from-memory.c
+++ b/libdwfl/elf-from-memory.c
@@ -292,80 +292,65 @@ elf_from_remote_memory (GElf_Addr ehdr_vma,
       goto no_memory;
     }
 
-  switch (ehdr.e32.e_ident[EI_CLASS])
+  for (uint_fast16_t i = 0; i < phnum; ++i)
     {
-      /* Reads the given segment.  Returns true if reading fails,
-	 false otherwise.  */
-      inline bool handle_segment (GElf_Addr vaddr, GElf_Off offset,
-				  GElf_Xword filesz)
-	{
-	  GElf_Off start = offset & -pagesize;
-	  GElf_Off end = (offset + filesz + pagesize - 1) & -pagesize;
-	  if (end > (GElf_Off) contents_size)
-	    end = contents_size;
-	  nread = (*read_memory) (arg, buffer + start,
-				  (loadbase + vaddr) & -pagesize,
-				  end - start, end - start);
-	  return nread <= 0;
-	}
+      GElf_Word type = class32 ? (*p32)[i].p_type : (*p64)[i].p_type;
 
-    case ELFCLASS32:
-      for (uint_fast16_t i = 0; i < phnum; ++i)
-	if ((*p32)[i].p_type == PT_LOAD)
-	  if (handle_segment ((*p32)[i].p_vaddr, (*p32)[i].p_offset,
-			      (*p32)[i].p_filesz))
-	    goto read_error;
-
-      /* If the segments visible in memory didn't include the section
-	 headers, then clear them from the file header.  */
-      if (contents_size < shdrs_end)
-	{
-	  ehdr.e32.e_shoff = 0;
-	  ehdr.e32.e_shnum = 0;
-	  ehdr.e32.e_shstrndx = 0;
-	}
+      if (type != PT_LOAD)
+        continue;
+
+      GElf_Addr vaddr = class32 ? (*p32)[i].p_vaddr : (*p64)[i].p_vaddr;
+      GElf_Off offset = class32 ? (*p32)[i].p_offset : (*p64)[i].p_offset;
+      GElf_Xword filesz = class32 ? (*p32)[i].p_filesz : (*p64)[i].p_filesz;
+
+      GElf_Off start = offset & -pagesize;
+      GElf_Off end = (offset + filesz + pagesize - 1) & -pagesize;
+      if (end > (GElf_Off) contents_size)
+        end = contents_size;
+      nread = (*read_memory) (arg, buffer + start,
+                              (loadbase + vaddr) & -pagesize,
+                              end - start, end - start);
+      if (nread <= 0)
+        goto read_error;
+    }
+
+  /* If the segments visible in memory didn't include the section
+     headers, then clear them from the file header.  */
+  if (contents_size < shdrs_end)
+    {
+      if (class32)
+        {
+          ehdr.e32.e_shoff = 0;
+          ehdr.e32.e_shnum = 0;
+          ehdr.e32.e_shstrndx = 0;
+        }
+      else
+        {
+          ehdr.e64.e_shoff = 0;
+          ehdr.e64.e_shnum = 0;
+          ehdr.e64.e_shstrndx = 0;
+        }
+    }
 
-      /* This will normally have been in the first PT_LOAD segment.  But it
-	 conceivably could be missing, and we might have just changed it.  */
-      xlatefrom.d_type = xlateto.d_type = ELF_T_EHDR;
+  /* This will normally have been in the first PT_LOAD segment.  But it
+     conceivably could be missing, and we might have just changed it.  */
+  xlatefrom.d_type = xlateto.d_type = ELF_T_EHDR;
+  xlateto.d_buf = buffer;
+  if (class32)
+    {
       xlatefrom.d_size = xlateto.d_size = sizeof ehdr.e32;
       xlatefrom.d_buf = &ehdr.e32;
-      xlateto.d_buf = buffer;
       if (elf32_xlatetof (&xlateto, &xlatefrom,
-			  ehdr.e32.e_ident[EI_DATA]) == NULL)
-	goto libelf_error;
-      break;
-
-    case ELFCLASS64:
-      for (uint_fast16_t i = 0; i < phnum; ++i)
-	if ((*p64)[i].p_type == PT_LOAD)
-	  if (handle_segment ((*p64)[i].p_vaddr, (*p64)[i].p_offset,
-			      (*p64)[i].p_filesz))
-	    goto read_error;
-
-      /* If the segments visible in memory didn't include the section
-	 headers, then clear them from the file header.  */
-      if (contents_size < shdrs_end)
-	{
-	  ehdr.e64.e_shoff = 0;
-	  ehdr.e64.e_shnum = 0;
-	  ehdr.e64.e_shstrndx = 0;
-	}
-
-      /* This will normally have been in the first PT_LOAD segment.  But it
-	 conceivably could be missing, and we might have just changed it.  */
-      xlatefrom.d_type = xlateto.d_type = ELF_T_EHDR;
+                          ehdr.e32.e_ident[EI_DATA]) == NULL)
+        goto libelf_error;
+    }
+  else
+    {
       xlatefrom.d_size = xlateto.d_size = sizeof ehdr.e64;
       xlatefrom.d_buf = &ehdr.e64;
-      xlateto.d_buf = buffer;
       if (elf64_xlatetof (&xlateto, &xlatefrom,
-			  ehdr.e64.e_ident[EI_DATA]) == NULL)
-	goto libelf_error;
-      break;
-
-    default:
-      abort ();
-      break;
+                          ehdr.e64.e_ident[EI_DATA]) == NULL)
+        goto libelf_error;
     }
 
   free (phdrsp);
-- 
2.26.2

Re: [PATCH 1/2] elf-from-memory: Restructure code to get rid of nested handle_segment()

[Mark Wielaard]

Hi Timm,

On Fri, Jan 08, 2021 at 09:09:55AM +0100, Timm Bäder via Elfutils-devel wrote:
> Use one loop for both 32 and 64  bit case. This allows for only one call
> site of the old handle_segment(), which we can then inline into the for
> loop.

It is a bit hard to see because of the reindenting, but the new code
does indeed do the same as the old code. Added a ChangeLog entry and
pushed.

Thanks,

Mark

Re: [PATCH 2/2] elf-from-memory: Refactor to get rid of nested function

[Mark Wielaard]

Hi Timm,

On Fri, Jan 08, 2021 at 09:09:56AM +0100, Timm Bäder via Elfutils-devel wrote:
> Try to unify the 32/64 bit code paths and get rid of the nested
> handle_segment() this way.

The new code does seem to do the same thing as the old code.
Added a ChangeLog entry and pushed.

Thanks,

Mark