link_map: Remove nested functions

link_map: Remove nested functions

[tbaeder]

Hi,

the attached patches get rid of nested functions in libdwfl/link_map.c.

I wrote these a while back and just looked at them again and we could
use the same read_state struct here as well. I just quickly checked,
but it seems a bit more involved due to the integrated_memory_callback
handling. I can look into that anyway if required.


Thanks,
Timm

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

[tbaeder]

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

Get rid of another nested function
---
 libdwfl/link_map.c | 47 +++++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 15 deletions(-)

diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
index 29307c74..5c39c631 100644
--- a/libdwfl/link_map.c
+++ b/libdwfl/link_map.c
@@ -225,6 +225,18 @@ addrsize (uint_fast8_t elfclass)
   return elfclass * 4;
 }
 
+static inline int
+release_buffer (Dwfl *dwfl,
+                Dwfl_Memory_Callback *memory_callback, void *memory_callback_arg,
+                void **buffer, size_t *buffer_available,
+                int result)
+{
+  if (buffer != NULL)
+    (void) (*memory_callback) (dwfl, -1, buffer, buffer_available, 0, 0,
+                               memory_callback_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,13 +261,6 @@ 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];
   inline bool read_addrs (GElf_Addr vaddr, size_t n)
@@ -267,7 +272,8 @@ 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 (dwfl, memory_callback, memory_callback_arg,
+                        &buffer, &buffer_available, 0);
 
 	read_vaddr = vaddr;
 	int segndx = INTUSE(dwfl_addrsegment) (dwfl, vaddr, NULL);
@@ -304,7 +310,9 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
   }
 
   if (unlikely (read_addrs (read_vaddr, 1)))
-    return release_buffer (-1);
+    release_buffer (dwfl, memory_callback, memory_callback_arg,
+                    &buffer, &buffer_available, -1);
+
 
   GElf_Addr next = addrs[0];
 
@@ -319,7 +327,8 @@ 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 (dwfl, memory_callback, memory_callback_arg,
+                               &buffer, &buffer_available, -1);
 
       /* 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 +354,9 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
 	name = l_name - read_vaddr + buffer;
       else
 	{
-	  release_buffer (0);
+          release_buffer (dwfl, memory_callback, memory_callback_arg,
+                          &buffer, &buffer_available, 0);
+
 	  read_vaddr = l_name;
 	  int segndx = INTUSE(dwfl_addrsegment) (dwfl, l_name, NULL);
 	  if (likely (segndx >= 0)
@@ -372,7 +383,9 @@ 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);
+            return release_buffer (dwfl, memory_callback, memory_callback_arg,
+                                   &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,9 @@ 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 (dwfl, memory_callback, memory_callback_arg,
+                                      &buffer, &buffer_available, 0);
+
 		      int segndx = INTUSE(dwfl_addrsegment) (dwfl,
 							     build_id_vaddr,
 							     NULL);
@@ -432,7 +447,8 @@ 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 (dwfl, memory_callback, memory_callback_arg,
+                                          &buffer, &buffer_available, 0);
 			}
 		    }
 
@@ -498,7 +514,8 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
 	}
     }
 
-  return release_buffer (result);
+  return release_buffer (dwfl, memory_callback, memory_callback_arg,
+                         &buffer, &buffer_available, result);
 }
 \f
 static GElf_Addr
-- 
2.26.2

[PATCH 2/3] link_map: Pull read_addrs() in file scope

[tbaeder]

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

Get rid of another nested function this way
---
 libdwfl/link_map.c | 114 ++++++++++++++++++++++++++-------------------
 1 file changed, 66 insertions(+), 48 deletions(-)

diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
index 5c39c631..64baaec4 100644
--- a/libdwfl/link_map.c
+++ b/libdwfl/link_map.c
@@ -237,6 +237,64 @@ release_buffer (Dwfl *dwfl,
   return result;
 }
 
+static inline bool
+read_addrs (Dwfl *dwfl,
+            Dwfl_Memory_Callback *memory_callback,
+            void *memory_callback_arg,
+            void **buffer,
+            size_t *buffer_available,
+            GElf_Addr vaddr, GElf_Addr *read_vaddr,
+            size_t n,
+            uint_fast8_t elfclass,
+            uint_fast8_t elfdata,
+            GElf_Addr *addrs /* [4] */)
+{
+  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 (dwfl, memory_callback, memory_callback_arg,
+                      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;
+}
+
+
+
 /* 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
@@ -262,54 +320,12 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
   void *buffer = NULL;
   size_t buffer_available = 0;
 
-  GElf_Addr addrs[4];
-  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 (dwfl, memory_callback, memory_callback_arg,
-                        &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;
-  }
+  GElf_Addr addrs[4] = { 0, 0, 0, 0 };
 
-  if (unlikely (read_addrs (read_vaddr, 1)))
+  if (unlikely (read_addrs (dwfl, memory_callback, memory_callback_arg,
+                            &buffer, &buffer_available,
+                            read_vaddr, &read_vaddr, 1,
+                            elfclass, elfdata, addrs)))
     release_buffer (dwfl, memory_callback, memory_callback_arg,
                     &buffer, &buffer_available, -1);
 
@@ -326,7 +342,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 (dwfl, memory_callback, memory_callback_arg,
+                      &buffer, &buffer_available,
+                      next, &read_vaddr, 4, elfclass, elfdata, addrs))
         return release_buffer (dwfl, memory_callback, memory_callback_arg,
                                &buffer, &buffer_available, -1);
 
-- 
2.26.2

[PATCH 3/3] link_map: Inline consider_phdr() into only caller

[tbaeder]

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

This gets rid of the tested function and is shorter.
---
 libdwfl/link_map.c | 66 ++++++++++++++++++----------------------------
 1 file changed, 26 insertions(+), 40 deletions(-)

diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
index 64baaec4..8a19f358 100644
--- a/libdwfl/link_map.c
+++ b/libdwfl/link_map.c
@@ -793,31 +793,6 @@ dwfl_link_map_report (Dwfl *dwfl, const void *auxv, size_t auxv_size,
       GElf_Xword dyn_filesz = 0;
       GElf_Addr dyn_bias = (GElf_Addr) -1;
 
-      inline bool consider_phdr (GElf_Word type,
-				 GElf_Addr vaddr, GElf_Xword filesz)
-      {
-	switch (type)
-	  {
-	  case PT_PHDR:
-	    if (dyn_bias == (GElf_Addr) -1
-		/* Do a sanity check on the putative address.  */
-		&& ((vaddr & (dwfl->segment_align - 1))
-		    == (phdr & (dwfl->segment_align - 1))))
-	      {
-		dyn_bias = phdr - vaddr;
-		return dyn_vaddr != 0;
-	      }
-	    break;
-
-	  case PT_DYNAMIC:
-	    dyn_vaddr = vaddr;
-	    dyn_filesz = filesz;
-	    return dyn_bias != (GElf_Addr) -1;
-	  }
-
-	return false;
-      }
-
       if (phdr != 0 && phnum != 0)
 	{
 	  Dwfl_Module *phdr_mod;
@@ -930,22 +905,33 @@ dwfl_link_map_report (Dwfl *dwfl, const void *auxv, size_t auxv_size,
 			   ? elf32_xlatetom : elf64_xlatetom)
 			  (&out, &in, elfdata) != NULL))
 		{
-		  /* We are looking for PT_DYNAMIC.  */
-		  if (elfclass == ELFCLASS32)
+		  bool is32 = (elfclass == ELFCLASS32);
+		  for (size_t i = 0; i < phnum; ++i)
 		    {
-		      for (size_t i = 0; i < phnum; ++i)
-			if (consider_phdr ((*p32)[i].p_type,
-					   (*p32)[i].p_vaddr,
-					   (*p32)[i].p_filesz))
-			  break;
-		    }
-		  else
-		    {
-		      for (size_t i = 0; i < phnum; ++i)
-			if (consider_phdr ((*p64)[i].p_type,
-					   (*p64)[i].p_vaddr,
-					   (*p64)[i].p_filesz))
-			  break;
+		      GElf_Word type = is32 ? (*p32)[i].p_type : (*p64)[i].p_type;
+		      GElf_Addr vaddr = is32 ? (*p32)[i].p_vaddr : (*p64)[i].p_vaddr;
+		      GElf_Xword filesz = is32 ? (*p32)[i].p_filesz : (*p64)[i].p_filesz;
+
+		      if (type == PT_PHDR)
+			{
+			  if (dyn_bias == (GElf_Addr) -1
+			      /* Do a sanity check on the putative address.  */
+			      && ((vaddr & (dwfl->segment_align - 1))
+				  == (phdr & (dwfl->segment_align - 1))))
+			    {
+			      dyn_bias = phdr - vaddr;
+			      if (dyn_vaddr != 0)
+				break;
+			    }
+
+			}
+		      else if (type == PT_DYNAMIC)
+			{
+			  dyn_vaddr = vaddr;
+			  dyn_filesz = filesz;
+			  if (dyn_bias != (GElf_Addr) -1)
+			    break;
+			}
 		    }
 		}
 
-- 
2.26.2

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

[Mark Wielaard]

Hi Timm,

On Tue, 2020-12-01 at 09:38 +0100, Timm Bäder via Elfutils-devel wrote:
> From: Timm Bäder <tbaeder@redhat.com>
> 
> Get rid of another nested function

It is missing a ChangeLog entry.

> diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
> index 29307c74..5c39c631 100644
> --- a/libdwfl/link_map.c
> +++ b/libdwfl/link_map.c
> @@ -225,6 +225,18 @@ addrsize (uint_fast8_t elfclass)
>    return elfclass * 4;
>  }
>  
> +static inline int
> +release_buffer (Dwfl *dwfl,
> +                Dwfl_Memory_Callback *memory_callback, void *memory_callback_arg,
> +                void **buffer, size_t *buffer_available,
> +                int result)
> +{
> +  if (buffer != NULL)
> +    (void) (*memory_callback) (dwfl, -1, buffer, buffer_available, 0, 0,
> +                               memory_callback_arg);
> +  return result;
> +}

Note that this changes the semantics slightly. Because you now take the
address of the buffer variable before checking it is NULL (it now never
is). Also note that the result is not always used, so it might be
cleaner to not pass it around. It is IMHO better to fully inline it.

> @@ -249,13 +261,6 @@ 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];
>    inline bool read_addrs (GElf_Addr vaddr, size_t n)
> [...]
> @@ -304,7 +310,9 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
>    }
>  
>    if (unlikely (read_addrs (read_vaddr, 1)))
> -    return release_buffer (-1);
> +    release_buffer (dwfl, memory_callback, memory_callback_arg,
> +                    &buffer, &buffer_available, -1);
> +

Note that here the result is used, but the change doesn't use it
anymore and so doesn't return early but just tries to carry on after an
error occurred.

Cheers,

Mark

Re: [PATCH 3/3] link_map: Inline consider_phdr() into only caller

[Mark Wielaard]

Hi Timm,

On Tue, 2020-12-01 at 09:38 +0100, Timm Bäder via Elfutils-devel wrote:
> This gets rid of the tested function and is shorter.

It is slightly shorter indeed. Although I changed a couple of lines to
not exceed 80 chars. And I added a ChangeLog entry to
libdwfl/ChangeLog.

Pushed.

Mark

Re: link_map: Remove nested functions

[Mark Wielaard]

Hi Timm,

On Tue, 2020-12-01 at 09:38 +0100, Timm Bäder via Elfutils-devel wrote:
> the attached patches get rid of nested functions in
> libdwfl/link_map.c.
> 
> I wrote these a while back and just looked at them again and we could
> use the same read_state struct here as well. I just quickly checked,
> but it seems a bit more involved due to the
> integrated_memory_callback
> handling. I can look into that anyway if required.

I had some comments on the first patch, the third patch seems fine and
has been committed. If you have adjust for the changes in the first
patch it would be nice if you could look at making the second patch
read_addrs function take a bit less than 11 arguments,

Note that all patches were missing ChangeLog entries and Signed-off-by
lines.

Cheers,

Mark