Remove remaining nested functions from libdwfl

Remove remaining nested functions from libdwfl

[tbaeder]

Here are the three patches to remove the three remaining nested
functions from libdwfl/dwfl_segment_report_module.c.

[PATCH 1/3] segment_report_module: Pull finish_portion() into file scope

[tbaeder]

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

Use a read_state struct here to minimize the amount of parameters we
pass.
---
 libdwfl/dwfl_segment_report_module.c | 38 ++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c
index 7e747184..391fd761 100644
--- a/libdwfl/dwfl_segment_report_module.c
+++ b/libdwfl/dwfl_segment_report_module.c
@@ -61,6 +61,14 @@ struct elf_build_id
   GElf_Addr vaddr;
 };
 
+struct read_state
+{
+  Dwfl *dwfl;
+  Dwfl_Memory_Callback *memory_callback;
+  void *memory_callback_arg;
+  void **buffer;
+  size_t *buffer_available;
+};
 
 /* Return user segment index closest to ADDR but not above it.
    If NEXT, return the closest to ADDR but not below it.  */
@@ -239,6 +247,15 @@ invalid_elf (Elf *elf, bool disk_file_has_build_id,
   return false;
 }
 
+static void
+finish_portion (struct read_state *read_state,
+		void **data, size_t *data_size)
+{
+  if (*data_size != 0 && *data != NULL)
+    (*read_state->memory_callback) (read_state->dwfl, -1, data, data_size,
+				    0, 0, read_state->memory_callback_arg);
+}
+
 int
 dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
 			    Dwfl_Memory_Callback *memory_callback,
@@ -249,6 +266,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
 			    const struct r_debug_info *r_debug_info)
 {
   size_t segment = ndx;
+  struct read_state read_state;
 
   if (segment >= dwfl->lookup_elts)
     segment = dwfl->lookup_elts - 1;
@@ -271,6 +289,12 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
   Elf *elf = NULL;
   int fd = -1;
 
+  read_state.dwfl = dwfl;
+  read_state.memory_callback = memory_callback;
+  read_state.memory_callback_arg = memory_callback_arg;
+  read_state.buffer = &buffer;
+  read_state.buffer_available = &buffer_available;
+
   /* We might have to reserve some memory for the phdrs.  Set to NULL
      here so we can always safely free it.  */
   void *phdrsp = NULL;
@@ -306,12 +330,6 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
     return false;
   }
 
-  inline void finish_portion (void **data, size_t *data_size)
-  {
-    if (*data_size != 0 && *data != NULL)
-      (*memory_callback) (dwfl, -1, data, data_size, 0, 0, memory_callback_arg);
-  }
-
   /* Extract the information we need from the file header.  */
   const unsigned char *e_ident;
   unsigned char ei_class;
@@ -519,7 +537,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
   done:
     if (notes != data)
       free (notes);
-    finish_portion (&data, &data_size);
+    finish_portion (&read_state, &data, &data_size);
   }
 
   Elf32_Phdr *p32 = phdrsp;
@@ -609,7 +627,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
         }
     }
 
-  finish_portion (&ph_buffer, &ph_buffer_size);
+  finish_portion (&read_state, &ph_buffer, &ph_buffer_size);
 
   /* We must have seen the segment covering offset 0, or else the ELF
      header we read at START was not produced by these program headers.  */
@@ -787,7 +805,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
         }
       free (dyns);
     }
-  finish_portion (&dyn_data, &dyn_data_size);
+  finish_portion (&read_state, &dyn_data, &dyn_data_size);
 
   /* We'll use the name passed in or a stupid default if not DT_SONAME.  */
   if (name == NULL)
@@ -848,7 +866,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
   /* At this point we do not need BUILD_ID or NAME any more.
      They have been copied.  */
   free (build_id.memory);
-  finish_portion (&soname, &soname_size);
+  finish_portion (&read_state, &soname, &soname_size);
 
   if (unlikely (mod == NULL))
     {
-- 
2.26.2

[PATCH 2/3] segment_report_module: Pull read_portion() into file scope

[tbaeder]

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

---
 libdwfl/dwfl_segment_report_module.c | 65 +++++++++++++++-------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c
index 391fd761..a082886a 100644
--- a/libdwfl/dwfl_segment_report_module.c
+++ b/libdwfl/dwfl_segment_report_module.c
@@ -256,6 +256,35 @@ finish_portion (struct read_state *read_state,
 				    0, 0, read_state->memory_callback_arg);
 }
 
+static inline bool
+read_portion (struct read_state *read_state,
+	      void **data, size_t *data_size,
+	      GElf_Addr start, size_t segment,
+	      GElf_Addr vaddr, size_t filesz)
+{
+  /* Check whether we will have to read the segment data, or if it
+     can be returned from the existing buffer.  */
+  if (filesz > *read_state->buffer_available
+      || vaddr - start > *read_state->buffer_available - filesz
+      /* If we're in string mode, then don't consider the buffer we have
+	 sufficient unless it contains the terminator of the string.  */
+      || (filesz == 0 && memchr (vaddr - start + *read_state->buffer, '\0',
+				 *read_state->buffer_available - (vaddr - start)) == NULL))
+    {
+      *data = NULL;
+      *data_size = filesz;
+      return ! (*read_state->memory_callback) (read_state->dwfl,
+					       addr_segndx (read_state->dwfl, segment, vaddr, false),
+					       data, data_size, vaddr, filesz,
+					       read_state->memory_callback_arg);
+    }
+
+  /* We already have this whole note segment from our initial read.  */
+  *data = vaddr - start + (*read_state->buffer);
+  *data_size = 0;
+  return false;
+}
+
 int
 dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
 			    Dwfl_Memory_Callback *memory_callback,
@@ -304,32 +333,6 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
       || memcmp (buffer, ELFMAG, SELFMAG) != 0)
     goto out;
 
-  inline bool read_portion (void **data, size_t *data_size,
-			    GElf_Addr vaddr, size_t filesz)
-  {
-    /* Check whether we will have to read the segment data, or if it
-       can be returned from the existing buffer.  */
-    if (filesz > buffer_available
-	|| vaddr - start > buffer_available - filesz
-	/* If we're in string mode, then don't consider the buffer we have
-	   sufficient unless it contains the terminator of the string.  */
-	|| (filesz == 0 && memchr (vaddr - start + buffer, '\0',
-				   buffer_available - (vaddr - start)) == NULL))
-      {
-	*data = NULL;
-	*data_size = filesz;
-	return ! (*memory_callback) (dwfl, addr_segndx (dwfl, segment,
-							vaddr, false),
-				     data, data_size, vaddr, filesz,
-				     memory_callback_arg);
-      }
-
-    /* We already have this whole note segment from our initial read.  */
-    *data = vaddr - start + buffer;
-    *data_size = 0;
-    return false;
-  }
-
   /* Extract the information we need from the file header.  */
   const unsigned char *e_ident;
   unsigned char ei_class;
@@ -410,7 +413,8 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
 
   void *ph_buffer = NULL;
   size_t ph_buffer_size = 0;
-  if (read_portion (&ph_buffer, &ph_buffer_size,
+  if (read_portion (&read_state, &ph_buffer, &ph_buffer_size,
+		    start, segment,
 		    start + phoff, xlatefrom.d_size))
     goto out;
 
@@ -469,7 +473,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
 
     void *data;
     size_t data_size;
-    if (read_portion (&data, &data_size, vaddr, filesz))
+    if (read_portion (&read_state, &data, &data_size, start, segment, vaddr, filesz))
       return;
 
     /* data_size will be zero if we got everything from the initial
@@ -756,7 +760,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
   void *dyn_data = NULL;
   size_t dyn_data_size = 0;
   if (dyn_filesz != 0 && dyn_filesz % dyn_entsize == 0
-      && ! read_portion (&dyn_data, &dyn_data_size, dyn_vaddr, dyn_filesz))
+      && ! read_portion (&read_state, &dyn_data, &dyn_data_size, start, segment, dyn_vaddr, dyn_filesz))
     {
       /* dyn_data_size will be zero if we got everything from the initial
          buffer, otherwise it will be the size of the new buffer that
@@ -837,7 +841,8 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
 
       /* Try to get the DT_SONAME string.  */
       if (soname_stroff != 0 && soname_stroff + 1 < dynstrsz
-	  && ! read_portion (&soname, &soname_size,
+	  && ! read_portion (&read_state, &soname, &soname_size,
+			     start, segment,
 			     dynstr_vaddr + soname_stroff, 0))
 	name = soname;
     }
-- 
2.26.2

[PATCH 3/3] segment_report_module: Inline consider_notes() into only caller

[tbaeder]

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

Get rid of a nested function this way.
---
 libdwfl/dwfl_segment_report_module.c | 162 +++++++++++++--------------
 1 file changed, 80 insertions(+), 82 deletions(-)

diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c
index a082886a..c48d9ab7 100644
--- a/libdwfl/dwfl_segment_report_module.c
+++ b/libdwfl/dwfl_segment_report_module.c
@@ -463,87 +463,6 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
   build_id.len = 0;
   build_id.vaddr =0;
 
-  /* Consider a PT_NOTE we've found in the image.  */
-  inline void consider_notes (GElf_Addr vaddr, GElf_Xword filesz,
-			      GElf_Xword align)
-  {
-    /* If we have already seen a build ID, we don't care any more.  */
-    if (build_id.memory != NULL || filesz == 0)
-      return;
-
-    void *data;
-    size_t data_size;
-    if (read_portion (&read_state, &data, &data_size, start, segment, vaddr, filesz))
-      return;
-
-    /* data_size will be zero if we got everything from the initial
-       buffer, otherwise it will be the size of the new buffer that
-       could be read.  */
-    if (data_size != 0)
-      filesz = data_size;
-
-    assert (sizeof (Elf32_Nhdr) == sizeof (Elf64_Nhdr));
-
-    void *notes;
-    if (ei_data == MY_ELFDATA)
-      notes = data;
-    else
-      {
-	notes = malloc (filesz);
-	if (unlikely (notes == NULL))
-	  return;
-	xlatefrom.d_type = xlateto.d_type = (align == 8
-					     ? ELF_T_NHDR8 : ELF_T_NHDR);
-	xlatefrom.d_buf = (void *) data;
-	xlatefrom.d_size = filesz;
-	xlateto.d_buf = notes;
-	xlateto.d_size = filesz;
-	if (elf32_xlatetom (&xlateto, &xlatefrom,
-			    ehdr.e32.e_ident[EI_DATA]) == NULL)
-	  goto done;
-      }
-
-    const GElf_Nhdr *nh = notes;
-    size_t len = 0;
-    while (filesz > len + sizeof (*nh))
-      {
-	const void *note_name;
-	const void *note_desc;
-
-	len += sizeof (*nh);
-	note_name = notes + len;
-
-	len += nh->n_namesz;
-	len = align == 8 ? NOTE_ALIGN8 (len) : NOTE_ALIGN4 (len);
-	note_desc = notes + len;
-
-	if (unlikely (filesz < len + nh->n_descsz))
-	  break;
-
-        if (nh->n_type == NT_GNU_BUILD_ID
-	    && nh->n_descsz > 0
-	    && nh->n_namesz == sizeof "GNU"
-	    && !memcmp (note_name, "GNU", sizeof "GNU"))
-	  {
-	    build_id.vaddr = note_desc - (const void *) notes + vaddr;
-	    build_id.len = nh->n_descsz;
-	    build_id.memory = malloc (build_id.len);
-	    if (likely (build_id.memory != NULL))
-	      memcpy (build_id.memory, note_desc, build_id.len);
-	    break;
-	  }
-
-	len += nh->n_descsz;
-	len = align == 8 ? NOTE_ALIGN8 (len) : NOTE_ALIGN4 (len);
-	nh = (void *) notes + len;
-      }
-
-  done:
-    if (notes != data)
-      free (notes);
-    finish_portion (&read_state, &data, &data_size);
-  }
-
   Elf32_Phdr *p32 = phdrsp;
   Elf64_Phdr *p64 = phdrsp;
   if ((ei_class == ELFCLASS32
@@ -573,9 +492,88 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
             }
           else if (type == PT_NOTE)
             {
+              /* If we have already seen a build ID, we don't care any more.  */
+              if (build_id.memory != NULL || filesz == 0)
+                continue; /* Next header */
+
               /* We calculate from the p_offset of the note segment,
                because we don't yet know the bias for its p_vaddr.  */
-              consider_notes ( start + offset, filesz, align);
+              const size_t note_vaddr = start + offset;
+              void *data;
+              size_t data_size;
+              if (read_portion (&read_state, &data, &data_size, start, segment, note_vaddr, filesz))
+                continue; /* Next header */
+
+              /* data_size will be zero if we got everything from the initial
+                 buffer, otherwise it will be the size of the new buffer that
+                 could be read.  */
+              if (data_size != 0)
+                filesz = data_size;
+
+              assert (sizeof (Elf32_Nhdr) == sizeof (Elf64_Nhdr));
+
+              void *notes;
+              if (ei_data == MY_ELFDATA)
+                notes = data;
+              else
+                {
+                  const unsigned int xencoding = ehdr.e32.e_ident[EI_DATA];
+
+                  notes = malloc (filesz);
+                  if (unlikely (notes == NULL))
+                    continue; /* Next header */
+                  xlatefrom.d_type = xlateto.d_type = (align == 8
+                                                       ? ELF_T_NHDR8 : ELF_T_NHDR);
+                  xlatefrom.d_buf = (void *) data;
+                  xlatefrom.d_size = filesz;
+                  xlateto.d_buf = notes;
+                  xlateto.d_size = filesz;
+                  if (elf32_xlatetom (&xlateto, &xlatefrom, xencoding) == NULL)
+                    {
+                      free (notes);
+                      finish_portion (&read_state, &data, &data_size);
+                      continue;
+                    }
+                }
+
+              const GElf_Nhdr *nh = notes;
+              size_t len = 0;
+              while (filesz > len + sizeof (*nh))
+                {
+                  const void *note_name;
+                  const void *note_desc;
+
+                  len += sizeof (*nh);
+                  note_name = notes + len;
+
+                  len += nh->n_namesz;
+                  len = align == 8 ? NOTE_ALIGN8 (len) : NOTE_ALIGN4 (len);
+                  note_desc = notes + len;
+
+                  if (unlikely (filesz < len + nh->n_descsz))
+                    break;
+
+                  if (nh->n_type == NT_GNU_BUILD_ID
+                      && nh->n_descsz > 0
+                      && nh->n_namesz == sizeof "GNU"
+                      && !memcmp (note_name, "GNU", sizeof "GNU"))
+                    {
+                      build_id.vaddr = note_desc - (const void *) notes + note_vaddr;
+                      build_id.len = nh->n_descsz;
+                      build_id.memory = malloc (build_id.len);
+                      if (likely (build_id.memory != NULL))
+                        memcpy (build_id.memory, note_desc, build_id.len);
+                      break;
+                    }
+
+                  len += nh->n_descsz;
+                  len = align == 8 ? NOTE_ALIGN8 (len) : NOTE_ALIGN4 (len);
+                  nh = (void *) notes + len;
+                }
+
+              if (notes != data)
+                free (notes);
+              finish_portion (&read_state, &data, &data_size);
             }
           else if (type == PT_LOAD)
             {
-- 
2.26.2

Re: Remove remaining nested functions from libdwfl

[Mark Wielaard]

On Thu, 2020-11-26 at 15:10 +0100, Timm Bäder via Elfutils-devel wrote:
> Here are the three patches to remove the three remaining nested
> functions from libdwfl/dwfl_segment_report_module.c.

Thanks those look good. Pushed.

I did add ChangeLog entries and made sure all lines were < 80 chars.

Cheers,

Mark