[PATCH 3/3 v2] src/readelf.c: Add support for print_debug_* output buffering

[Mark Wielaard]

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 <amerey@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