* [PATCH 1/3] link_map: Pull release_buffer() into file scope
2020-12-01 8:38 link_map: Remove nested functions tbaeder
@ 2020-12-01 8:38 ` tbaeder
2020-12-06 13:20 ` Mark Wielaard
2020-12-01 8:38 ` [PATCH 2/3] link_map: Pull read_addrs() in " tbaeder
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: tbaeder @ 2020-12-01 8:38 UTC (permalink / raw)
To: elfutils-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/3] link_map: Pull release_buffer() into file scope
2020-12-01 8:38 ` [PATCH 1/3] link_map: Pull release_buffer() into file scope tbaeder
@ 2020-12-06 13:20 ` Mark Wielaard
0 siblings, 0 replies; 7+ messages in thread
From: Mark Wielaard @ 2020-12-06 13:20 UTC (permalink / raw)
To: tbaeder, elfutils-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/3] link_map: Pull read_addrs() in file scope
2020-12-01 8:38 link_map: Remove nested functions tbaeder
2020-12-01 8:38 ` [PATCH 1/3] link_map: Pull release_buffer() into file scope tbaeder
@ 2020-12-01 8:38 ` tbaeder
2020-12-01 8:38 ` [PATCH 3/3] link_map: Inline consider_phdr() into only caller tbaeder
2020-12-06 13:41 ` link_map: Remove nested functions Mark Wielaard
3 siblings, 0 replies; 7+ messages in thread
From: tbaeder @ 2020-12-01 8:38 UTC (permalink / raw)
To: elfutils-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 3/3] link_map: Inline consider_phdr() into only caller
2020-12-01 8:38 link_map: Remove nested functions tbaeder
2020-12-01 8:38 ` [PATCH 1/3] link_map: Pull release_buffer() into file scope tbaeder
2020-12-01 8:38 ` [PATCH 2/3] link_map: Pull read_addrs() in " tbaeder
@ 2020-12-01 8:38 ` tbaeder
2020-12-06 13:39 ` Mark Wielaard
2020-12-06 13:41 ` link_map: Remove nested functions Mark Wielaard
3 siblings, 1 reply; 7+ messages in thread
From: tbaeder @ 2020-12-01 8:38 UTC (permalink / raw)
To: elfutils-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: link_map: Remove nested functions
2020-12-01 8:38 link_map: Remove nested functions tbaeder
` (2 preceding siblings ...)
2020-12-01 8:38 ` [PATCH 3/3] link_map: Inline consider_phdr() into only caller tbaeder
@ 2020-12-06 13:41 ` Mark Wielaard
3 siblings, 0 replies; 7+ messages in thread
From: Mark Wielaard @ 2020-12-06 13:41 UTC (permalink / raw)
To: tbaeder, elfutils-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread