Hi Aaron,
On Thu, 2025-05-22 at 18:28 -0400, Aaron Merey wrote:
> Safely handle stdout output during concurrent calls to print_debug_*
> functions.
>
> For any print_debug_* function and any function that could be called
> from print_debug_* which also prints to stdout: add a FILE * argument
> and replace all printf, puts, putchar with fprintf. All printing
> to stdout will now be written to this FILE instead.
>
> The FILE * is an interface to a per-thread dynamically-sized buffer.
> libthread.a manages the allocation, printing and deallocation of
> these buffers.
I would have expected this patch to come before the libthread
enhancements. Just passing stdout around. And then add the per-thread
FILE* object.
In general this looks OK. It is just adding and extra argument to lots
of functions and using it.
But I don't fully understand why NULL is passed around in some cases.
It seems to be for the non-debug-section handling. But in those cases
why not just pass stdout instead of having to special case NULL?
> Signed-off-by: Aaron Merey <ame...@redhat.com>
> ---
> v2:
> Minor change to print_dwarf_addr declaration so patch applies cleanly
> to main branch.
>
> src/readelf.c | 2258 +++++++++++++++++++++++++------------------------
> 1 file changed, 1157 insertions(+), 1101 deletions(-)
>
> diff --git a/src/readelf.c b/src/readelf.c
> index 38381156..8f233a77 100644
> --- a/src/readelf.c
> +++ b/src/readelf.c
> @@ -361,7 +361,7 @@ static void dump_strings (Ebl *ebl);
> static void print_strings (Ebl *ebl);
> static void dump_archive_index (Elf *, const char *);
> static void print_dwarf_addr (Dwfl_Module *dwflmod, int address_size,
> - Dwarf_Addr address, Dwarf_Addr raw);
> + Dwarf_Addr address, Dwarf_Addr raw, FILE *out);
> static void print_flag_info(void);
>
> enum dyn_idx
> @@ -2609,7 +2609,7 @@ handle_relocs_relr (Ebl *ebl, Dwfl_Module *mod, Elf_Scn
> *scn, GElf_Shdr *shdr)
> if ((entry & 1) == 0)
> {
> printf (" ");
> - print_dwarf_addr (mod, 4, entry, entry);
> + print_dwarf_addr (mod, 4, entry, entry, NULL);
> printf (" *\n");
>
> base = entry + 4;
> @@ -2622,7 +2622,7 @@ handle_relocs_relr (Ebl *ebl, Dwfl_Module *mod, Elf_Scn
> *scn, GElf_Shdr *shdr)
> if ((entry & 1) != 0)
> {
> printf (" ");
> - print_dwarf_addr (mod, 4, addr, addr);
> + print_dwarf_addr (mod, 4, addr, addr, NULL);
> printf ("\n");
> }
> base += 4 * (4 * 8 - 1);
> @@ -2648,7 +2648,7 @@ handle_relocs_relr (Ebl *ebl, Dwfl_Module *mod, Elf_Scn
> *scn, GElf_Shdr *shdr)
> if ((entry & 1) == 0)
> {
> printf (" ");
> - print_dwarf_addr (mod, 8, entry, entry);
> + print_dwarf_addr (mod, 8, entry, entry, NULL);
> printf (" *\n");
>
> base = entry + 8;
> @@ -2661,7 +2661,7 @@ handle_relocs_relr (Ebl *ebl, Dwfl_Module *mod, Elf_Scn
> *scn, GElf_Shdr *shdr)
> if ((entry & 1) != 0)
> {
> printf (" ");
> - print_dwarf_addr (mod, 8, addr, addr);
> + print_dwarf_addr (mod, 8, addr, addr, NULL);
> printf ("\n");
> }
> base += 8 * (8 * 8 - 1);
Like in these cases, I would have expected to pass around stdout.
> @@ -4369,8 +4369,12 @@ get_debug_elf_data (Dwarf *dbg, Ebl *ebl, int idx,
> Elf_Scn *scn)
>
> static void
> print_dwarf_addr (Dwfl_Module *dwflmod,
> - int address_size, Dwarf_Addr address, Dwarf_Addr raw)
> + int address_size, Dwarf_Addr address, Dwarf_Addr raw,
> + FILE *out)
> {
> + if (out == NULL)
> + out = stdout;
> +
So you don't have to special case it here.
>
> @@ -4985,13 +4991,17 @@ get_indexed_addr (Dwarf_CU *cu, Dwarf_Word idx,
> Dwarf_Addr *addr)
> static void
> print_ops (Dwfl_Module *dwflmod, Dwarf *dbg, int indent, int indentrest,
> unsigned int vers, unsigned int addrsize, unsigned int offset_size,
> - struct Dwarf_CU *cu, Dwarf_Word len, const unsigned char *data)
> + struct Dwarf_CU *cu, Dwarf_Word len, const unsigned char *data,
> + FILE *out)
> {
> + if (out == NULL)
> + out = stdout;
> +
Or here. I cannot find any invocation of print_ops with out being NULL.
Everything else looks obviously correct.
Thanks,
Mark
