Remove nested functions from link_map.c V2

Remove nested functions from link_map.c V2

[tbaeder]

Same as the old thread. I used a memory_closure struct this time.
I quickly looked over the patches and I think we could even save
buffer and buffer_available in there to reduce the parameter count
further but then we should rename the struct (but read_data isn't the
best name either).

Hope the changelog entries are fine, I'll try to add them from now on.



Thanks,
Timm

[PATCH 1/2] link_map: Pull release_buffer() into file scope

[tbaeder]

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

Get rid of a nested function this way. Add a memory_closure struct to
keep the functions clean.

Signed-off-by: Timm Bäder <tbaeder@redhat.com>
---
 libdwfl/ChangeLog  |  5 +++++
 libdwfl/link_map.c | 48 ++++++++++++++++++++++++++++++----------------
 2 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index f11abb80..a5e6a4a7 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2020-12-07  Timm Bäder  <tbaeder@redhat.com>
+
+	* link_map.c (report_r_debug): Pull release_buffer() function into
+	file scope. Add memory_closure struct.
+
 2020-12-01  Timm Bäder  <tbaeder@redhat.com>
 
 	* link_map.c (dwfl_link_map_report): Removed consider_phdr function
diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
index bcff8db5..f0119a97 100644
--- a/libdwfl/link_map.c
+++ b/libdwfl/link_map.c
@@ -225,6 +225,24 @@ addrsize (uint_fast8_t elfclass)
   return elfclass * 4;
 }
 
+struct memory_closure
+{
+  Dwfl *dwfl;
+  Dwfl_Memory_Callback *callback;
+  void *arg;
+};
+
+static inline int
+release_buffer (struct memory_closure *closure,
+                void **buffer, size_t *buffer_available, int result)
+{
+  if (*buffer != NULL)
+    (*closure->callback) (closure->dwfl, -1, buffer, buffer_available, 0, 0,
+                          closure->arg);
+
+  return result;
+}
+
 /* Report a module for each struct link_map in the linked list at r_map
    in the struct r_debug at R_DEBUG_VADDR.  For r_debug_info description
    see dwfl_link_map_report in libdwflP.h.  If R_DEBUG_INFO is not NULL then no
@@ -249,15 +267,9 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
 
   void *buffer = NULL;
   size_t buffer_available = 0;
-  inline int release_buffer (int result)
-  {
-    if (buffer != NULL)
-      (void) (*memory_callback) (dwfl, -1, &buffer, &buffer_available, 0, 0,
-				 memory_callback_arg);
-    return result;
-  }
-
   GElf_Addr addrs[4];
+  struct memory_closure memory_closure = { dwfl, memory_callback,
+                                           memory_callback_arg };
   inline bool read_addrs (GElf_Addr vaddr, size_t n)
   {
     size_t nb = n * addrsize (elfclass); /* Address words -> bytes to read.  */
@@ -267,7 +279,7 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
 	|| vaddr < read_vaddr
 	|| vaddr - read_vaddr + nb > buffer_available)
       {
-	release_buffer (0);
+	release_buffer (&memory_closure, &buffer, &buffer_available, 0);
 
 	read_vaddr = vaddr;
 	int segndx = INTUSE(dwfl_addrsegment) (dwfl, vaddr, NULL);
@@ -304,7 +316,7 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
   }
 
   if (unlikely (read_addrs (read_vaddr, 1)))
-    return release_buffer (-1);
+    return release_buffer (&memory_closure, &buffer, &buffer_available, -1);
 
   GElf_Addr next = addrs[0];
 
@@ -319,7 +331,7 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
   while (next != 0 && ++iterations < dwfl->lookup_elts)
     {
       if (read_addrs (next, 4))
-	return release_buffer (-1);
+	return release_buffer (&memory_closure, &buffer, &buffer_available, 0);
 
       /* Unused: l_addr is the difference between the address in memory
          and the ELF file when the core was created. We need to
@@ -345,7 +357,7 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
 	name = l_name - read_vaddr + buffer;
       else
 	{
-	  release_buffer (0);
+	  release_buffer (&memory_closure, &buffer, &buffer_available, 0);
 	  read_vaddr = l_name;
 	  int segndx = INTUSE(dwfl_addrsegment) (dwfl, l_name, NULL);
 	  if (likely (segndx >= 0)
@@ -372,7 +384,8 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
 	  r_debug_info_module = malloc (sizeof (*r_debug_info_module)
 					+ strlen (name1) + 1);
 	  if (unlikely (r_debug_info_module == NULL))
-	    return release_buffer (result);
+	    release_buffer (&memory_closure, &buffer,
+                            &buffer_available, result);
 	  r_debug_info_module->fd = -1;
 	  r_debug_info_module->elf = NULL;
 	  r_debug_info_module->l_ld = l_ld;
@@ -413,7 +426,8 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
 		      GElf_Addr build_id_vaddr = (build_id_elfaddr
 						  - elf_dynamic_vaddr + l_ld);
 
-		      release_buffer (0);
+		      release_buffer (&memory_closure, &buffer,
+				      &buffer_available, 0);
 		      int segndx = INTUSE(dwfl_addrsegment) (dwfl,
 							     build_id_vaddr,
 							     NULL);
@@ -432,7 +446,9 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
 			    /* File has valid build-id which does not match
 			       the one in memory.  */
 			    valid = false;
-			  release_buffer (0);
+			  release_buffer (&memory_closure, &buffer,
+					  &buffer_available, 0);
+
 			}
 		    }
 
@@ -498,7 +514,7 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
 	}
     }
 
-  return release_buffer (result);
+  return release_buffer (&memory_closure, &buffer, &buffer_available, result);
 }
 \f
 static GElf_Addr
-- 
2.26.2

[PATCH 2/2] link_map: Pull read_addrs() 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>
---
 libdwfl/ChangeLog  |   5 +++
 libdwfl/link_map.c | 104 +++++++++++++++++++++++++--------------------
 2 files changed, 62 insertions(+), 47 deletions(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index a5e6a4a7..d4e77f55 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2020-12-07  Timm Bäder  <tbaeder@redhat.com>
+
+	* link_map.c (report_r_debug): Pull read_addrs() function into
+	file scope.
+
 2020-12-07  Timm Bäder  <tbaeder@redhat.com>
 
 	* link_map.c (report_r_debug): Pull release_buffer() function into
diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
index f0119a97..5a6a4dd9 100644
--- a/libdwfl/link_map.c
+++ b/libdwfl/link_map.c
@@ -243,6 +243,57 @@ release_buffer (struct memory_closure *closure,
   return result;
 }
 
+static inline bool
+read_addrs (struct memory_closure *closure,
+	    uint_fast8_t elfclass, uint_fast8_t elfdata,
+	    void **buffer, size_t *buffer_available,
+	    GElf_Addr vaddr, GElf_Addr *read_vaddr,
+	    size_t n, GElf_Addr *addrs /* [4] */)
+{
+  size_t nb = n * addrsize (elfclass); /* Address words -> bytes to read.  */
+  Dwfl *dwfl = closure->dwfl;
+
+  /* Read a new buffer if the old one doesn't cover these words.  */
+  if (buffer == NULL
+      || vaddr < *read_vaddr
+      || vaddr - (*read_vaddr) + nb > *buffer_available)
+    {
+      release_buffer (closure, buffer, buffer_available, 0);
+
+      *read_vaddr = vaddr;
+      int segndx = INTUSE(dwfl_addrsegment) (dwfl, vaddr, NULL);
+      if (unlikely (segndx < 0)
+	  || unlikely (! (*closure->callback) (dwfl, segndx,
+					       buffer, buffer_available,
+					       vaddr, nb, closure->arg)))
+	return true;
+    }
+
+  Elf32_Addr (*a32)[n] = vaddr - (*read_vaddr) + (*buffer);
+  Elf64_Addr (*a64)[n] = (void *) a32;
+
+  if (elfclass == ELFCLASS32)
+    {
+      if (elfdata == ELFDATA2MSB)
+	for (size_t i = 0; i < n; ++i)
+	  addrs[i] = BE32 (read_4ubyte_unaligned_noncvt (&(*a32)[i]));
+      else
+	for (size_t i = 0; i < n; ++i)
+	  addrs[i] = LE32 (read_4ubyte_unaligned_noncvt (&(*a32)[i]));
+    }
+  else
+    {
+      if (elfdata == ELFDATA2MSB)
+	for (size_t i = 0; i < n; ++i)
+	  addrs[i] = BE64 (read_8ubyte_unaligned_noncvt (&(*a64)[i]));
+      else
+	for (size_t i = 0; i < n; ++i)
+	  addrs[i] = LE64 (read_8ubyte_unaligned_noncvt (&(*a64)[i]));
+    }
+
+  return false;
+}
+
 /* Report a module for each struct link_map in the linked list at r_map
    in the struct r_debug at R_DEBUG_VADDR.  For r_debug_info description
    see dwfl_link_map_report in libdwflP.h.  If R_DEBUG_INFO is not NULL then no
@@ -270,52 +321,9 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
   GElf_Addr addrs[4];
   struct memory_closure memory_closure = { dwfl, memory_callback,
                                            memory_callback_arg };
-  inline bool read_addrs (GElf_Addr vaddr, size_t n)
-  {
-    size_t nb = n * addrsize (elfclass); /* Address words -> bytes to read.  */
-
-    /* Read a new buffer if the old one doesn't cover these words.  */
-    if (buffer == NULL
-	|| vaddr < read_vaddr
-	|| vaddr - read_vaddr + nb > buffer_available)
-      {
-	release_buffer (&memory_closure, &buffer, &buffer_available, 0);
-
-	read_vaddr = vaddr;
-	int segndx = INTUSE(dwfl_addrsegment) (dwfl, vaddr, NULL);
-	if (unlikely (segndx < 0)
-	    || unlikely (! (*memory_callback) (dwfl, segndx,
-					       &buffer, &buffer_available,
-					       vaddr, nb, memory_callback_arg)))
-	  return true;
-      }
-
-    Elf32_Addr (*a32)[n] = vaddr - read_vaddr + buffer;
-    Elf64_Addr (*a64)[n] = (void *) a32;
-
-    if (elfclass == ELFCLASS32)
-      {
-	if (elfdata == ELFDATA2MSB)
-	  for (size_t i = 0; i < n; ++i)
-	    addrs[i] = BE32 (read_4ubyte_unaligned_noncvt (&(*a32)[i]));
-	else
-	  for (size_t i = 0; i < n; ++i)
-	    addrs[i] = LE32 (read_4ubyte_unaligned_noncvt (&(*a32)[i]));
-      }
-    else
-      {
-	if (elfdata == ELFDATA2MSB)
-	  for (size_t i = 0; i < n; ++i)
-	    addrs[i] = BE64 (read_8ubyte_unaligned_noncvt (&(*a64)[i]));
-	else
-	  for (size_t i = 0; i < n; ++i)
-	    addrs[i] = LE64 (read_8ubyte_unaligned_noncvt (&(*a64)[i]));
-      }
-
-    return false;
-  }
-
-  if (unlikely (read_addrs (read_vaddr, 1)))
+  if (unlikely (read_addrs (&memory_closure, elfclass, elfdata,
+			    &buffer, &buffer_available, read_vaddr, &read_vaddr,
+			    1, addrs)))
     return release_buffer (&memory_closure, &buffer, &buffer_available, -1);
 
   GElf_Addr next = addrs[0];
@@ -330,7 +338,9 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
   size_t iterations = 0;
   while (next != 0 && ++iterations < dwfl->lookup_elts)
     {
-      if (read_addrs (next, 4))
+      if (read_addrs (&memory_closure, elfclass, elfdata,
+		      &buffer, &buffer_available, next, &read_vaddr,
+		      4, addrs))
 	return release_buffer (&memory_closure, &buffer, &buffer_available, 0);
 
       /* Unused: l_addr is the difference between the address in memory
-- 
2.26.2

Re: [PATCH 1/2] link_map: Pull release_buffer() into file scope

[Mark Wielaard]

Hi Timm,

On Mon, 2020-12-07 at 12:11 +0100, Timm Bäder via Elfutils-devel wrote:
> Get rid of a nested function this way. Add a memory_closure struct to
> keep the functions clean.

Pushed with one fix.

> @@ -319,7 +331,7 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
>    while (next != 0 && ++iterations < dwfl->lookup_elts)
>      {
>        if (read_addrs (next, 4))
> -	return release_buffer (-1);
> +	return release_buffer (&memory_closure, &buffer, &buffer_available, 0);

The last argument must be -1.

Cheers,

Mark

Re: [PATCH 2/2] link_map: Pull read_addrs() into file scope

[Mark Wielaard]

Hi Timm,

On Mon, 2020-12-07 at 12:11 +0100, Timm Bäder via Elfutils-devel wrote:
> Get rid of a nested function this way.

The whole idea behind using nested functions is that we don't need to
pass around 9 arguments... But ok, it looks harmless. Pushed.

Cheers,

Mark