Remove nested functions from libdwfl V2
Hi again, version 2 of this patch set. I removed segmend_read() entirely now, which meant modifying a bunch of later patches. Other than that, they are the same. Hope the --from to git send-email worked out, too. Thanks, Timm
[PATCH 01/12] segment_report_module: Get rid of segment_read()
From: Timm Bäder Just inline the memory_callback call everywhere segmenty_read was used. Signed-off-by: Timm Bäder --- libdwfl/dwfl_segment_report_module.c | 24 +--- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index c7725002..c587efd7 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -257,18 +257,11 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, GElf_Addr start = dwfl->lookup_addr[segment]; - inline bool segment_read (int segndx, - void **buffer, size_t *buffer_available, - GElf_Addr addr, size_t minread) - { -return ! (*memory_callback) (dwfl, segndx, buffer, buffer_available, -addr, minread, memory_callback_arg); - } - inline void release_buffer (void **buffer, size_t *buffer_available) { if (*buffer != NULL) - (void) segment_read (-1, buffer, buffer_available, 0, 0); + (*memory_callback) (dwfl, -1, buffer, buffer_available, 0, 0, + memory_callback_arg); } /* First read in the file header and check its sanity. */ @@ -282,8 +275,8 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, here so we can always safely free it. */ void *phdrsp = NULL; - if (segment_read (ndx, &buffer, &buffer_available, - start, sizeof (Elf64_Ehdr)) + if (! (*memory_callback) (dwfl, ndx, &buffer, &buffer_available, +start, sizeof (Elf64_Ehdr), memory_callback_arg) || memcmp (buffer, ELFMAG, SELFMAG) != 0) goto out; @@ -301,8 +294,8 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, { *data = NULL; *data_size = filesz; - return segment_read (addr_segndx (dwfl, segment, vaddr, false), -data, data_size, vaddr, 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. */ @@ -908,8 +901,9 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, { void *into = contents + offset; size_t read_size = size; - (void) segment_read (addr_segndx (dwfl, segment, vaddr, false), -&into, &read_size, vaddr, size); +(*memory_callback) (dwfl, addr_segndx (dwfl, segment, vaddr, false), +&into, &read_size, vaddr, size, +memory_callback_arg); } if (contiguous < file_trimmed_end) -- 2.26.2
[PATCH 02/12] segment_report_module: Remove nested release_buffer() function
From: Timm Bäder Signed-off-by: Timm Bäder --- libdwfl/dwfl_segment_report_module.c | 16 ++-- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index c587efd7..848c3bec 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -257,13 +257,6 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, GElf_Addr start = dwfl->lookup_addr[segment]; - inline void release_buffer (void **buffer, size_t *buffer_available) - { -if (*buffer != NULL) - (*memory_callback) (dwfl, -1, buffer, buffer_available, 0, 0, - memory_callback_arg); - } - /* First read in the file header and check its sanity. */ void *buffer = NULL; @@ -306,8 +299,8 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, inline void finish_portion (void **data, size_t *data_size) { -if (*data_size != 0) - release_buffer (data, 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. */ @@ -960,7 +953,10 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, out: free (phdrsp); - release_buffer (&buffer, &buffer_available); + if (buffer != NULL) +(*memory_callback) (dwfl, -1, &buffer, &buffer_available, 0, 0, +memory_callback_arg); + if (elf != NULL) elf_end (elf); if (fd != -1) -- 2.26.2
[PATCH 05/12] segment_report_module: Use a struct for build id information
From: Timm Bäder Keep the three build id fields in a struct. This will be an important clean up later. Signed-off-by: Timm Bäder --- libdwfl/dwfl_segment_report_module.c | 54 1 file changed, 31 insertions(+), 23 deletions(-) diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index 01adfe9e..6abbf992 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -54,6 +54,13 @@ # define MY_ELFDATAELFDATA2MSB #endif +struct elf_build_id +{ + void *memory; + size_t len; + GElf_Addr vaddr; +}; + /* Return user segment index closest to ADDR but not above it. If NEXT, return the closest to ADDR but not below it. */ @@ -206,16 +213,16 @@ handle_file_note (GElf_Addr module_start, GElf_Addr module_end, static bool invalid_elf (Elf *elf, bool disk_file_has_build_id, -const void *build_id, size_t build_id_len) + struct elf_build_id *build_id) { - if (! disk_file_has_build_id && build_id_len > 0) + if (! disk_file_has_build_id && build_id->len > 0) { /* Module found in segments with build-id is more reliable than a module found via DT_DEBUG on disk without any build-id. */ return true; } - if (disk_file_has_build_id && build_id_len > 0) + if (disk_file_has_build_id && build_id->len > 0) { const void *elf_build_id; ssize_t elf_build_id_len; @@ -224,8 +231,8 @@ invalid_elf (Elf *elf, bool disk_file_has_build_id, elf_build_id_len = INTUSE(dwelf_elf_gnu_build_id) (elf, &elf_build_id); if (elf_build_id_len > 0) { - if (build_id_len != (size_t) elf_build_id_len - || memcmp (build_id, elf_build_id, build_id_len) != 0) + if (build_id->len != (size_t) elf_build_id_len + || memcmp (build_id->memory, elf_build_id, build_id->len) != 0) return true; } } @@ -442,16 +449,17 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, GElf_Xword dyn_filesz = 0; /* Collect the build ID bits here. */ - void *build_id = NULL; - size_t build_id_len = 0; - GElf_Addr build_id_vaddr = 0; + struct elf_build_id build_id; + build_id.memory = NULL; + 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 != NULL || filesz == 0) +if (build_id.memory != NULL || filesz == 0) return; void *data; @@ -510,11 +518,11 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, && 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 = malloc (nh->n_descsz); - if (likely (build_id != NULL)) - memcpy (build_id, note_desc, build_id_len); +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; } @@ -629,7 +637,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, header we read at START was not produced by these program headers. */ if (unlikely (!found_bias)) { - free (build_id); + free (build_id.memory); goto out; } @@ -684,7 +692,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, { if (module->elf != NULL && invalid_elf (module->elf, module->disk_file_has_build_id, - build_id, build_id_len)) +&build_id)) { elf_end (module->elf); close (module->fd); @@ -700,7 +708,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, } if (skip_this_module) { - free (build_id); + free (build_id.memory); goto out; } } @@ -719,7 +727,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, Dwfl_Error error = __libdw_open_file (&fd, &elf, true, false); if (error == DWFL_E_NOERROR) invalid = invalid_elf (elf, true /* disk_file_has_build_id */, - build_id, build_id_len); + &build_id); } if (invalid) { @@ -867,11 +875,11 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, if (mod != NULL && (execlike || ehdr.e32.e_type == ET_EXEC)) mod->is_execut
[PATCH 06/12] segment_report_module: Pull consider_notes() into file scope
From: Timm Bäder Signed-off-by: Timm Bäder --- libdwfl/dwfl_segment_report_module.c | 187 +++ 1 file changed, 101 insertions(+), 86 deletions(-) diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index 6abbf992..6c6f9f37 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -282,6 +282,99 @@ read_portion (Dwfl *dwfl, return false; } + +/* Consider a PT_NOTE we've found in the image. */ +static inline void +consider_notes (Dwfl *dwfl, +Dwfl_Memory_Callback *memory_callback, void *memory_callback_arg, +GElf_Addr vaddr, GElf_Xword filesz, +GElf_Xword align, +void *buffer, size_t buffer_available, +GElf_Addr start, +size_t segment, +unsigned char ei_data, +struct elf_build_id *build_id, +Elf_Data *xlatefrom, +Elf_Data *xlateto, +unsigned int xencoding) +{ + if (build_id->memory != NULL || filesz == 0) +return; + + void *data; + size_t data_size; + if (read_portion (dwfl, memory_callback, memory_callback_arg, +&data, &data_size, vaddr, filesz, +buffer, buffer_available, start, segment)) +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, xencoding) == 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 (dwfl, memory_callback, memory_callback_arg, &data, &data_size); +} + int dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, Dwfl_Memory_Callback *memory_callback, @@ -454,89 +547,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 (dwfl, memory_callback, memory_callback_arg, - &data, &data_size, vaddr, filesz, - buffer, buffer_available, start, segment)) - 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[E
[PATCH 07/12] segment_report_module: Get rid of nested final_read() function
From: Timm Bäder Signed-off-by: Timm Bäder --- libdwfl/dwfl_segment_report_module.c | 25 ++--- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index 6c6f9f37..a69ead8f 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -934,15 +934,6 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, if (unlikely (contents == NULL)) goto out; - inline void final_read (size_t offset, GElf_Addr vaddr, size_t size) - { - void *into = contents + offset; - size_t read_size = size; -(*memory_callback) (dwfl, addr_segndx (dwfl, segment, vaddr, false), -&into, &read_size, vaddr, size, -memory_callback_arg); - } - if (contiguous < file_trimmed_end) { /* We can't use the memory image verbatim as the file image. @@ -952,7 +943,13 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, GElf_Off offset, GElf_Xword filesz) { if (type == PT_LOAD) - final_read (offset, vaddr + bias, filesz); + { +void *into = contents + offset; +size_t read_size = filesz; +(*memory_callback) (dwfl, addr_segndx (dwfl, segment, vaddr + bias, false), +&into, &read_size, vaddr + bias, read_size, +memory_callback_arg); + } } if (ei_class == ELFCLASS32) @@ -973,7 +970,13 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, memcpy (contents, buffer, have); if (have < file_trimmed_end) - final_read (have, start + have, file_trimmed_end - have); +{ + void *into = contents + have; + size_t read_size = file_trimmed_end - have; +(*memory_callback) (dwfl, addr_segndx (dwfl, segment, start + have, false), +&into, &read_size, start + have, read_size, +memory_callback_arg); +} } elf = elf_memory (contents, file_trimmed_end); -- 2.26.2
[PATCH 09/12] segment_report_module: Inline read_phdr() into only caller
From: Timm Bäder There is now only one caller for this nested function, so get rid of it by just inlining it there. Signed-off-by: Timm Bäder --- libdwfl/dwfl_segment_report_module.c | 24 +--- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index 276e9117..10212964 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -938,29 +938,23 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, { /* We can't use the memory image verbatim as the file image. So we'll be reading into a local image of the virtual file. */ - - inline void read_phdr (GElf_Word type, GElf_Addr vaddr, -GElf_Off offset, GElf_Xword filesz) - { - if (type == PT_LOAD) - { -void *into = contents + offset; -size_t read_size = filesz; -(*memory_callback) (dwfl, addr_segndx (dwfl, segment, vaddr + bias, false), -&into, &read_size, vaddr + bias, read_size, -memory_callback_arg); - } - } - for (uint_fast16_t i = 0; i < phnum; ++i) { bool is32 = (ei_class == ELFCLASS32); GElf_Word type = is32 ? p32[i].p_type : p64[i].p_type; + + if (type != PT_LOAD) +continue; + GElf_Addr vaddr = is32 ? p32[i].p_vaddr : p64[i].p_vaddr; GElf_Off offset = is32 ? p32[i].p_offset : p64[i].p_offset; GElf_Xword filesz = is32 ? p32[i].p_filesz : p64[i].p_filesz; - read_phdr (type, vaddr, offset, filesz); + void *into = contents + offset; + size_t read_size = filesz; + (*memory_callback) (dwfl, addr_segndx (dwfl, segment, vaddr + bias, false), + &into, &read_size, vaddr + bias, read_size, + memory_callback_arg); } } else -- 2.26.2
[PATCH 12/12] segment_report_module: Inline consider_phdr() into only caller
From: Timm Bäder Get rid of the nested function this way Signed-off-by: Timm Bäder --- libdwfl/dwfl_segment_report_module.c | 142 +-- 1 file changed, 66 insertions(+), 76 deletions(-) diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index 046d5381..26d4e80a 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -461,7 +461,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, /* NOTE if the number of sections is > 0xff00 then e_shnum is zero and the actual number would come from the section zero sh_size field. We ignore this here because getting shdrs -is just a nice bonus (see below in consider_phdr PT_LOAD +is just a nice bonus (see below in the type == PT_LOAD case where we trim the last segment). */ shdrs_end = ehdr.e32.e_shoff + ehdr.e32.e_shnum * ehdr.e32.e_shentsize; break; @@ -547,80 +547,6 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, build_id.len = 0; build_id.vaddr =0; - /* Consider each of the program headers we've read from the image. */ - inline void consider_phdr (GElf_Word type, -GElf_Addr vaddr, GElf_Xword memsz, -GElf_Off offset, GElf_Xword filesz, -GElf_Xword align) - { -switch (type) - { - case PT_DYNAMIC: - dyn_vaddr = vaddr; - dyn_filesz = filesz; - break; - - case PT_NOTE: - /* We calculate from the p_offset of the note segment, - because we don't yet know the bias for its p_vaddr. */ - consider_notes (dwfl, memory_callback, memory_callback_arg, - start + offset, filesz, align, - buffer, buffer_available, start, segment, - ei_data, &build_id, - &xlatefrom, &xlateto, - ehdr.e32.e_ident[EI_DATA]); - break; - - case PT_LOAD: - align = dwfl->segment_align > 1 ? dwfl->segment_align : align ?: 1; - - GElf_Addr vaddr_end = (vaddr + memsz + align - 1) & -align; - GElf_Addr filesz_vaddr = filesz < memsz ? vaddr + filesz : vaddr_end; - GElf_Off filesz_offset = filesz_vaddr - vaddr + offset; - - if (file_trimmed_end < offset + filesz) - { - file_trimmed_end = offset + filesz; - - /* Trim the last segment so we don't bother with zeros - in the last page that are off the end of the file. - However, if the extra bit in that page includes the - section headers, keep them. */ - if (shdrs_end <= filesz_offset && shdrs_end > file_trimmed_end) - { - filesz += shdrs_end - file_trimmed_end; - file_trimmed_end = shdrs_end; - } - } - - total_filesz += filesz; - - if (file_end < filesz_offset) - { - file_end = filesz_offset; - if (filesz_vaddr - start == filesz_offset) - contiguous = file_end; - } - - if (!found_bias && (offset & -align) == 0 - && likely (filesz_offset >= phoff + phnum * phentsize)) - { - bias = start - vaddr; - found_bias = true; - } - - if ((vaddr & -align) < module_start) - { - module_start = vaddr & -align; - module_address_sync = vaddr + memsz; - } - - if (module_end < vaddr_end) - module_end = vaddr_end; - break; - } - } - Elf32_Phdr *p32 = phdrsp; Elf64_Phdr *p64 = phdrsp; if ((ei_class == ELFCLASS32 @@ -632,6 +558,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, } else { + /* Consider each of the program headers we've read from the image. */ for (uint_fast16_t i = 0; i < phnum; ++i) { bool is32 = (ei_class == ELFCLASS32); @@ -642,7 +569,70 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, GElf_Xword filesz = is32 ? p32[i].p_filesz : p64[i].p_filesz; GElf_Xword align = is32 ? p32[i].p_align : p64[i].p_align; - consider_phdr (type, vaddr, memsz, offset, filesz, align); + if (type == PT_DYNAMIC) +{ + dyn_vaddr = vaddr; + dyn_filesz = filesz; +} + else if (type == PT_NOTE) +{ + /* We calculate from the p_offset of the note segment, + because we don't yet know the bias for its p_vaddr. */ + consider_notes (dwfl, memory_callback, memory_callback_arg, + start + offset, filesz, align, + buffer, buffer_available, start, segment, + ei_data, &build_id, + &xlatefrom, &xlateto, +
[PATCH 11/12] segment_report_module: Inline consider_dyn() into only caller
From: Timm Bäder Signed-off-by: Timm Bäder --- libdwfl/dwfl_segment_report_module.c | 40 +--- 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index 8613ce21..046d5381 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -770,33 +770,6 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, GElf_Addr dynstr_vaddr = 0; GElf_Xword dynstrsz = 0; bool execlike = false; - inline bool consider_dyn (GElf_Sxword tag, GElf_Xword val) - { -switch (tag) - { - default: - return false; - - case DT_DEBUG: - execlike = true; - break; - - case DT_SONAME: - soname_stroff = val; - break; - - case DT_STRTAB: - dynstr_vaddr = val; - break; - - case DT_STRSZ: - dynstrsz = val; - break; - } - -return soname_stroff != 0 && dynstr_vaddr != 0 && dynstrsz != 0; - } - const size_t dyn_entsize = (ei_class == ELFCLASS32 ? sizeof (Elf32_Dyn) : sizeof (Elf64_Dyn)); void *dyn_data = NULL; @@ -834,7 +807,18 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, GElf_Sxword tag = is32 ? d32[i].d_tag : d64[i].d_tag; GElf_Xword val = is32 ? d32[i].d_un.d_val : d64[i].d_un.d_val; - if (consider_dyn (tag, val)) + if (tag == DT_DEBUG) +execlike = true; + else if (tag == DT_SONAME) +soname_stroff = val; + else if (tag == DT_STRTAB) +dynstr_vaddr = val; + else if (tag == DT_STRSZ) +dynstrsz = val; + else +continue; + + if (soname_stroff != 0 && dynstr_vaddr != 0 && dynstrsz != 0) break; } } -- 2.26.2
[PATCH 03/12] segment_report_module: Pull finish_portion() info file scope
From: Timm Bäder Signed-off-by: Timm Bäder --- libdwfl/dwfl_segment_report_module.c | 24 ++-- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index 848c3bec..c55168ed 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -232,6 +232,16 @@ invalid_elf (Elf *elf, bool disk_file_has_build_id, return false; } +static inline void +finish_portion (Dwfl *dwfl, +Dwfl_Memory_Callback *memory_callback, +void *memory_callback_arg, +void **data, size_t *data_size) +{ + if (*data_size != 0 && *data != NULL) +(*memory_callback) (dwfl, -1, data, data_size, 0, 0, memory_callback_arg); +} + int dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, Dwfl_Memory_Callback *memory_callback, @@ -297,12 +307,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; @@ -509,7 +513,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 (dwfl, memory_callback, memory_callback_arg, &data, &data_size); } /* Consider each of the program headers we've read from the image. */ @@ -606,7 +610,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, p64[i].p_align); } - finish_portion (&ph_buffer, &ph_buffer_size); + finish_portion (dwfl, memory_callback, memory_callback_arg, &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. */ @@ -798,7 +802,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, } free (dyns); } - finish_portion (&dyn_data, &dyn_data_size); + finish_portion (dwfl, memory_callback, memory_callback_arg, &dyn_data, &dyn_data_size); /* We'll use the name passed in or a stupid default if not DT_SONAME. */ if (name == NULL) @@ -859,7 +863,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); - finish_portion (&soname, &soname_size); + finish_portion (dwfl, memory_callback, memory_callback_arg, &soname, &soname_size); if (unlikely (mod == NULL)) { -- 2.26.2
[PATCH 04/12] segment_report_module: Pull read_portion() into file scope
From: Timm Bäder Signed-off-by: Timm Bäder --- libdwfl/dwfl_segment_report_module.c | 77 +--- 1 file changed, 47 insertions(+), 30 deletions(-) diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index c55168ed..01adfe9e 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -242,6 +242,39 @@ finish_portion (Dwfl *dwfl, (*memory_callback) (dwfl, -1, data, data_size, 0, 0, memory_callback_arg); } + +static inline bool +read_portion (Dwfl *dwfl, + Dwfl_Memory_Callback *memory_callback, + void *memory_callback_arg, + void **data, size_t *data_size, + GElf_Addr vaddr, size_t filesz, + void *buffer, + size_t buffer_available, + GElf_Addr start, + size_t segment) +{ + /* 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; +} + int dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, Dwfl_Memory_Callback *memory_callback, @@ -283,30 +316,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; @@ -387,8 +396,10 @@ 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, - start + phoff, xlatefrom.d_size)) + if (read_portion (dwfl, memory_callback, memory_callback_arg, +&ph_buffer, &ph_buffer_size, +start + phoff, xlatefrom.d_size, +buffer, buffer_available, start, segment)) goto out; /* ph_buffer_size will be zero if we got everything from the initial @@ -445,7 +456,9 @@ 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 (dwfl, memory_callback, memory_callback_arg, + &data, &data_size, vaddr, filesz, + buffer, buffer_available, start, segment)) return; /* data_size will be zero if we got everything from the initial @@ -766,7 +779,9 @@ 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 (dwfl, memory_callback, memory_callback_arg, + &dyn_data, &dyn_data_size, dyn_vaddr, dyn_filesz, + buffer, buffer_available, start, segment)) { /* 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 @@ -834,8 +849,10 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const
[PATCH 08/12] segment_report_module: Use one loop for p32/p64 arrays
From: Timm Bäder Do one loop check for 32/64 bit inside the loop, instead of outside. This way we have only one call site for the function called in the loop body. Signed-off-by: Timm Bäder --- libdwfl/dwfl_segment_report_module.c | 52 +++- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index a69ead8f..276e9117 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -623,27 +623,27 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, Elf32_Phdr *p32 = phdrsp; Elf64_Phdr *p64 = phdrsp; - if (ei_class == ELFCLASS32) + if ((ei_class == ELFCLASS32 + && elf32_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL) + || (ei_class == ELFCLASS64 + && elf64_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL)) { - if (elf32_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL) - found_bias = false; /* Trigger error check. */ - else - for (uint_fast16_t i = 0; i < phnum; ++i) - consider_phdr (p32[i].p_type, -p32[i].p_vaddr, p32[i].p_memsz, -p32[i].p_offset, p32[i].p_filesz, -p32[i].p_align); + found_bias = false; /* Trigger error check */ } else { - if (elf64_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL) - found_bias = false; /* Trigger error check. */ - else - for (uint_fast16_t i = 0; i < phnum; ++i) - consider_phdr (p64[i].p_type, -p64[i].p_vaddr, p64[i].p_memsz, -p64[i].p_offset, p64[i].p_filesz, -p64[i].p_align); + for (uint_fast16_t i = 0; i < phnum; ++i) +{ + bool is32 = (ei_class == ELFCLASS32); + 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 memsz = is32 ? p32[i].p_memsz : p64[i].p_memsz; + GElf_Off offset = is32 ? p32[i].p_offset : p64[i].p_offset; + GElf_Xword filesz = is32 ? p32[i].p_filesz : p64[i].p_filesz; + GElf_Xword align = is32 ? p32[i].p_align : p64[i].p_align; + + consider_phdr (type, vaddr, memsz, offset, filesz, align); +} } finish_portion (dwfl, memory_callback, memory_callback_arg, &ph_buffer, &ph_buffer_size); @@ -952,14 +952,16 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, } } - if (ei_class == ELFCLASS32) - for (uint_fast16_t i = 0; i < phnum; ++i) - read_phdr (p32[i].p_type, p32[i].p_vaddr, -p32[i].p_offset, p32[i].p_filesz); - else - for (uint_fast16_t i = 0; i < phnum; ++i) - read_phdr (p64[i].p_type, p64[i].p_vaddr, -p64[i].p_offset, p64[i].p_filesz); + for (uint_fast16_t i = 0; i < phnum; ++i) +{ + bool is32 = (ei_class == ELFCLASS32); + 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_Off offset = is32 ? p32[i].p_offset : p64[i].p_offset; + GElf_Xword filesz = is32 ? p32[i].p_filesz : p64[i].p_filesz; + + read_phdr (type, vaddr, offset, filesz); +} } else { -- 2.26.2
[PATCH 10/12] segment_report_module: Unify d32/d64 loops
From: Timm Bäder Just like we did before, use only one loop here and check for 32/64 bit in the loop body. This way we only have one call site for consider_dyn Signed-off-by: Timm Bäder --- libdwfl/dwfl_segment_report_module.c | 28 ++-- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index 10212964..8613ce21 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -824,20 +824,20 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, xlateto.d_buf = dyns; xlateto.d_size = dyn_filesz; - if (ei_class == ELFCLASS32) - { - if (elf32_xlatetom (&xlateto, &xlatefrom, ei_data) != NULL) - for (size_t i = 0; i < dyn_filesz / sizeof (Elf32_Dyn); ++i) - if (consider_dyn (d32[i].d_tag, d32[i].d_un.d_val)) - break; - } - else - { - if (elf64_xlatetom (&xlateto, &xlatefrom, ei_data) != NULL) - for (size_t i = 0; i < dyn_filesz / sizeof (Elf64_Dyn); ++i) - if (consider_dyn (d64[i].d_tag, d64[i].d_un.d_val)) - break; - } + bool is32 = (ei_class == ELFCLASS32); + if ((is32 && elf32_xlatetom (&xlateto, &xlatefrom, ei_data) != NULL) + || (!is32 && elf64_xlatetom (&xlateto, &xlatefrom, ei_data) != NULL)) +{ + size_t n = is32 ? (dyn_filesz / sizeof (Elf32_Dyn)) : (dyn_filesz / sizeof (Elf64_Dyn)); + for (size_t i = 0; i < n; ++i) +{ + GElf_Sxword tag = is32 ? d32[i].d_tag : d64[i].d_tag; + GElf_Xword val = is32 ? d32[i].d_un.d_val : d64[i].d_un.d_val; + + if (consider_dyn (tag, val)) +break; +} +} free (dyns); } finish_portion (dwfl, memory_callback, memory_callback_arg, &dyn_data, &dyn_data_size); -- 2.26.2
Re: [PATCH] debuginfod-client: Add debuginfod_set_verbose_fd and DEBUGINFOD_VERBOSE
Hi, On Wed, 2020-11-11 at 22:18 +0100, Mark Wielaard wrote: > Introduce a new function debuginfod_set_verbose_fd which will produce > verbose output on a given file descriptor (STDERR_FILENO if the > environment variable DEBUGINFOD_VERBOSE is set) showing how the search > for a particular client query is going. I didn't get more comments on list, but there was some positive feedback on irc. So I have pushed it now. But please let me know if there are any concerns with this new interface. Cheers, Mark
Buildbot failure in Wildebeest Builder on whole buildset
The Buildbot has detected a failed build on builder whole buildset while building elfutils. Full details are available at: https://builder.wildebeest.org/buildbot/#builders/1/builds/630 Buildbot URL: https://builder.wildebeest.org/buildbot/ Worker for this Build: centos-x86_64 Build Reason: Blamelist: Mark Wielaard BUILD FAILED: failed test (failure) Sincerely, -The Buildbot
Re: Buildbot failure in Wildebeest Builder on whole buildset
On Mon, 2020-11-23 at 17:50 +, build...@builder.wildebeest.org wrote: > The Buildbot has detected a failed build on builder whole buildset > while building elfutils. > Full details are available at: > https://builder.wildebeest.org/buildbot/#builders/1/builds/630 > > Buildbot URL: https://builder.wildebeest.org/buildbot/ > > Worker for this Build: centos-x86_64 > > Build Reason: > Blamelist: Mark Wielaard > > BUILD FAILED: failed test (failure) Interestingly only one buildbot worker found this issue, but I could replicate it under valgrind locally. And indeed, valgrind is right. We forgot to initialize the new handler_data errbuf field. Fixed as attached. Cheers, Mark From 6b82ac67d8a71c14f64fe4932ca7fe822ff75231 Mon Sep 17 00:00:00 2001 From: Mark Wielaard Date: Mon, 23 Nov 2020 17:52:02 +0100 Subject: [PATCH] debuginfod-client: Initialize struct handle_data errbuf to the empty string. Signed-off-by: Mark Wielaard --- debuginfod/ChangeLog | 5 + debuginfod/debuginfod-client.c | 1 + 2 files changed, 6 insertions(+) diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog index bd4dbf40..cd009fd2 100644 --- a/debuginfod/ChangeLog +++ b/debuginfod/ChangeLog @@ -1,3 +1,8 @@ +2020-11-23 Mark Wielaard + + * debuginfod-client.c (debuginfod_query_server): Initialize + struct handle_data errbuf to the empty string. + 2020-11-11 Mark Wielaard * debuginfod-client.c (debuginfod_set_verbose_fd): New function. diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c index 2bf1543a..a99f3c14 100644 --- a/debuginfod/debuginfod-client.c +++ b/debuginfod/debuginfod-client.c @@ -756,6 +756,7 @@ debuginfod_query_server (debuginfod_client *c, { data[i].handle = NULL; data[i].fd = -1; + data[i].errbuf[0] = '\0'; } char *strtok_saveptr; -- 2.18.4
patch: debuginfod sqlite3 metrics
Hi - >From 0b61f4c758a507fcc2243357363e44140bd13d82 Mon Sep 17 00:00:00 2001 ?!??!! From: "Frank Ch. Eigler" Date: Mon, 23 Nov 2020 19:58:10 -0500 Subject: [PATCH] debuginfod: sqlite3 metrics Add metrics for tracking sqlite3 error counts and query performance. The former looks like a new sibling of the "error_count" family, and is tested by dd-corrupting a live database file then triggering some debuginfod activity. error_count{sqlite3="file is not a database"} 1 The latter looks like _count/_sum pairs for each type of sqlite prepared-statement used in the code, and is grep smoke-tested. They should assist a sysadmin in tuning db storage. This example shows a 6.4 ms/operation cost: sqlite3_milliseconds_count{step-done="rpm-file-intern"} 318 sqlite3_milliseconds_sum{reset="rpm-file-intern"} 2033 Signed-off-by: Frank Ch. Eigler --- debuginfod/ChangeLog | 6 ++ debuginfod/debuginfod.cxx| 31 --- tests/ChangeLog | 4 tests/run-debuginfod-find.sh | 16 4 files changed, 54 insertions(+), 3 deletions(-) diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog index bd4dbf403f30..a81e3781986d 100644 --- a/debuginfod/ChangeLog +++ b/debuginfod/ChangeLog @@ -1,3 +1,9 @@ +2020-11-23 Frank Ch. Eigler + + * debuginfod.cxx (tmp_ms_metric): New class for RAII timing metrics. + (sqlite_ps::reset, step*): Call it to track sqlite3 performance. + (sqlite_exception ctor): Increment sqlite3 error_count. + 2020-11-11 Mark Wielaard * debuginfod-client.c (debuginfod_set_verbose_fd): New function. diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx index 61c778b10893..618620b4c478 100644 --- a/debuginfod/debuginfod.cxx +++ b/debuginfod/debuginfod.cxx @@ -435,6 +435,27 @@ class tmp_inc_metric { // a RAII style wrapper for exception-safe scoped increme } }; +class tmp_ms_metric { // a RAII style wrapper for exception-safe scoped timing + string m, n, v; + struct timeval tv_start; +public: + tmp_ms_metric(const string& mname, const string& lname, const string& lvalue): +m(mname), n(lname), v(lvalue) + { +gettimeofday (& tv_start, NULL); + } + ~tmp_ms_metric() + { +struct timeval tv_end; +gettimeofday (& tv_end, NULL); +double deltas = (tv_end.tv_sec - tv_start.tv_sec) + + (tv_end.tv_usec - tv_start.tv_usec)*0.01; + +add_metric (m + "_milliseconds_sum", n, v, (deltas*1000)); +inc_metric (m + "_milliseconds_count", n, v); + } +}; + /* Handle program arguments. */ static error_t @@ -559,7 +580,9 @@ struct reportable_exception struct sqlite_exception: public reportable_exception { sqlite_exception(int rc, const string& msg): -reportable_exception(string("sqlite3 error: ") + msg + ": " + string(sqlite3_errstr(rc) ?: "?")) {} +reportable_exception(string("sqlite3 error: ") + msg + ": " + string(sqlite3_errstr(rc) ?: "?")) { +inc_metric("error_count","sqlite3",sqlite3_errstr(rc)); + } }; struct libc_exception: public reportable_exception @@ -755,6 +778,7 @@ struct sqlite_ps public: sqlite_ps (sqlite3* d, const string& n, const string& s): db(d), nickname(n), sql(s) { +// tmp_ms_metric tick("sqlite3","prep",nickname); if (verbose > 4) obatched(clog) << nickname << " prep " << sql << endl; int rc = sqlite3_prepare_v2 (db, sql.c_str(), -1 /* to \0 */, & this->pp, NULL); @@ -764,6 +788,7 @@ struct sqlite_ps sqlite_ps& reset() { +tmp_ms_metric tick("sqlite3","reset",nickname); sqlite3_reset(this->pp); return *this; } @@ -800,6 +825,7 @@ struct sqlite_ps void step_ok_done() { +tmp_ms_metric tick("sqlite3","step-done",nickname); int rc = sqlite3_step (this->pp); if (verbose > 4) obatched(clog) << nickname << " step-ok-done(" << sqlite3_errstr(rc) << ") " << sql << endl; @@ -810,14 +836,13 @@ struct sqlite_ps int step() { +tmp_ms_metric tick("sqlite3","step",nickname); int rc = sqlite3_step (this->pp); if (verbose > 4) obatched(clog) << nickname << " step(" << sqlite3_errstr(rc) << ") " << sql << endl; return rc; } - - ~sqlite_ps () { sqlite3_finalize (this->pp); } operator sqlite3_stmt* () { return this->pp; } }; diff --git a/tests/ChangeLog b/tests/ChangeLog index c130ce049caa..f5adc315ae38 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,7 @@ +2020-11-23 Frank Ch. Eigler + + * run-debuginfod-find.sh: Add sqlite error injection & stats. + 2020-11-02 Mark Wielaard * run-debuginfod-find.sh: Create bogus R/nothing.rpm with cyclic diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh index 28aa4263c500..7fd3420ab20a 100755 --- a/tests/run-debuginfod-find.sh +++ b/tests/run-debuginfod-find.sh @@ -490,6 +490,22 @@ curl -s http://127.0.0.1:$PORT2/badapi > /dev/null || true curl -s http://127.0.0.1:$PORT2/buildid/deadbeef/debuginfo >
[announce] Void Linux debuginfod server
Hi! I am happy to announce that Void Linux is now hosting a debuginfod server in https://debuginfod.s.voidlinux.org ! It includes all packages for which we have debug packages (some packages, like chromium, have debug package generation disabled), for all of official architectures and libcs which had build-id enabled in their toolchains (!), which are: - x86_64 glibc and musl - i686 glibc - aarch64 glibc - armv7l glibc - armv6l glibc During this effort, I found out the targets below didn't have build-id's enabled, and will be getting that fixed as soon as possible. Due to musl's time64 transition, we will likely do a full repository rebuild at least for armv6l and armv7l, which will help in getting debug files for those two up to par with the others: - aarch64 musl - armv7l musl - armv6l musl At the moment, our gdb package is built with libdebuginfod support, and we are looking into adding it to binutils and systemtap as well. I was very interested when I first read about the project, and am very happy I could drive its adoption in Void. Thank you all for having the idea, for the development work, and for helping me with the compatibility patches as well! Érico Nogueira