* [PATCH 1/3] segment_report_module: Pull finish_portion() into file scope
2020-11-26 14:10 Remove remaining nested functions from libdwfl tbaeder
@ 2020-11-26 14:10 ` tbaeder
2020-11-26 14:10 ` [PATCH 2/3] segment_report_module: Pull read_portion() " tbaeder
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: tbaeder @ 2020-11-26 14:10 UTC (permalink / raw)
To: elfutils-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 2/3] segment_report_module: Pull read_portion() into file scope
2020-11-26 14:10 Remove remaining nested functions from libdwfl tbaeder
2020-11-26 14:10 ` [PATCH 1/3] segment_report_module: Pull finish_portion() into file scope tbaeder
@ 2020-11-26 14:10 ` tbaeder
2020-11-26 14:10 ` [PATCH 3/3] segment_report_module: Inline consider_notes() into only caller tbaeder
2020-11-28 1:02 ` Remove remaining nested functions from libdwfl Mark Wielaard
3 siblings, 0 replies; 5+ messages in thread
From: tbaeder @ 2020-11-26 14:10 UTC (permalink / raw)
To: elfutils-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 3/3] segment_report_module: Inline consider_notes() into only caller
2020-11-26 14:10 Remove remaining nested functions from libdwfl tbaeder
2020-11-26 14:10 ` [PATCH 1/3] segment_report_module: Pull finish_portion() into file scope tbaeder
2020-11-26 14:10 ` [PATCH 2/3] segment_report_module: Pull read_portion() " tbaeder
@ 2020-11-26 14:10 ` tbaeder
2020-11-28 1:02 ` Remove remaining nested functions from libdwfl Mark Wielaard
3 siblings, 0 replies; 5+ messages in thread
From: tbaeder @ 2020-11-26 14:10 UTC (permalink / raw)
To: elfutils-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: Remove remaining nested functions from libdwfl
2020-11-26 14:10 Remove remaining nested functions from libdwfl tbaeder
` (2 preceding siblings ...)
2020-11-26 14:10 ` [PATCH 3/3] segment_report_module: Inline consider_notes() into only caller tbaeder
@ 2020-11-28 1:02 ` Mark Wielaard
3 siblings, 0 replies; 5+ messages in thread
From: Mark Wielaard @ 2020-11-28 1:02 UTC (permalink / raw)
To: tbaeder, elfutils-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread