[PATCH] objdump: sframe: fix multiple emission of SFrame and DWARF Frame

[Jan Beulich]

On 03.02.2026 23:37, Indu Bhagat via Binutils wrote:
> Currently, when a user specifies both -WF (to dump DWARF Frame) and
> --sframe, the dump_dwarf () call is triggered twice,
> 
> First, in dump_bfd ():
>   if (dump_dwarf_section_info)
>     dump_dwarf (abfd, is_mainfile);
> 
> And then again in dump_bfd () via:
>    if (dump_sframe_section_info)
>      dump_sframe_section (abfd, dump_sframe_section_name, is_mainfile);
> Where dump_sframe_section () itself invokes dump_dwarf () to emit SFrame
> section.
> 
> The original intent of making dump_sframe_section () to invoke
> dump_dwarf () was to reuse the existing (DWARF) machinery for relocation
> and emission of SFrame section (along with DWARF sections).  But this
> may cause duplicate emission as noted above (PR 33806).
> 
> So, carve out the necessary work into dump_sframe_section () to make it
> not invoke dump_dwarf (): explicitly load the SFrame section and then
> invoke the associated display function directly.  This ensures that the
> SFrame textual dump is emitted exactly once as intended.
> 
> Additionally, this patch removes:
>   - the temporary "sframe-internal-only" debug option because it not
>     needed anymore, now that we dont use dump_dwarf () for SFrame dumping
>     anymore.
>   - stubs from dump_dwarf_section () specific to SFrame, as it is not
>     called for it anymore.
> 
> While at it, xstrdup consistently and free the dump_sframe_section_name
> var.
> 
> Update the existing sframe-func.s to ensure .eh_frame section is emitted
> as well.  Add new test objdump-sframe-2.d to verify that textual dump
> using objdump contains single textual dumps of .eh_frame and .sframe (if
> both --sframe and -WF are specified).
> 
> PR libsframe/33806
> 
> binutils/
> 	PR libsframe/33806
> 	* dwarf.c (debug_option_table): Remove sframe-internal-only.
> 	* objdump.c (dump_sframe_section): Call display function
> 	directly instead of dump_dwarf.
> 	(dump_bfd): Update call to dump_sframe_section.
> 	(main): Remove 'sframe-internal-only' hack.
> binutils/testsuite/
> 	* binutils-all/x86-64/sframe-func.s: Emit .eh_frame section as
> 	well.
> 	* binutils-all/x86-64/objdump-sframe-2.d: New test.
> ---
>  binutils/dwarf.c                              |  1 -
>  binutils/objdump.c                            | 69 +++++++++----------
>  .../binutils-all/x86-64/objdump-sframe-2.d    | 33 +++++++++
>  .../binutils-all/x86-64/sframe-func.s         |  1 -
>  4 files changed, 66 insertions(+), 38 deletions(-)
>  create mode 100644 binutils/testsuite/binutils-all/x86-64/objdump-sframe-2.d

Largely okay, and even a negative diffstat for objdump.c alone. A nit and a few
remarks below, though.

> @@ -4983,33 +4979,40 @@ dump_ctf (bfd *abfd ATTRIBUTE_UNUSED, const char *sect_name ATTRIBUTE_UNUSED,
>  #endif
>  
>  static void
> -dump_sframe_section (bfd *abfd, const char *sect_name, bool is_mainfile)
> +dump_sframe_section (bfd *abfd, const char *sect_name)
>  
>  {
>    /* Error checking for user provided SFrame section name, if any.  */
> -  if (sect_name)
> +  asection *sec = bfd_get_section_by_name (abfd, sect_name);
> +  if (sec == NULL)
>      {
> -      asection *sec = bfd_get_section_by_name (abfd, sect_name);
> -      if (sec == NULL)
> -	{
> -	  printf (_("No %s section present\n\n"), sanitize_string (sect_name));
> -	  return;
> -	}
> -      /* Starting with Binutils 2.45, SFrame sections have section type
> -	 SHT_GNU_SFRAME.  For SFrame sections from Binutils 2.44 or earlier,
> -	 check explcitly for SFrame sections of type SHT_PROGBITS and name
> -	 ".sframe" to allow them.  */
> -      else if (bfd_get_flavour (abfd) != bfd_target_elf_flavour
> -	       || (elf_section_type (sec) != SHT_GNU_SFRAME
> -		   && !(elf_section_type (sec) == SHT_PROGBITS
> -			&& strcmp (sect_name, ".sframe") == 0)))
> -	{
> -	  printf (_("Section %s does not contain SFrame data\n\n"),
> -		  sanitize_string (sect_name));
> -	  return;
> -	}
> +      printf (_("No %s section present\n\n"), sanitize_string (sect_name));
> +      return;
> +    }
> +  /* Starting with Binutils 2.45, SFrame sections have section type
> +     SHT_GNU_SFRAME.  For SFrame sections from Binutils 2.44 or earlier,
> +     check explcitly for SFrame sections of type SHT_PROGBITS and name
> +     ".sframe" to allow them.  */
> +  else if (bfd_get_flavour (abfd) != bfd_target_elf_flavour
> +	   || (elf_section_type (sec) != SHT_GNU_SFRAME
> +	       && !(elf_section_type (sec) == SHT_PROGBITS
> +		    && strcmp (sect_name, ".sframe") == 0)))
> +    {
> +      printf (_("Section %s does not contain SFrame data\n\n"),
> +	      sanitize_string (sect_name));
> +      return;
>      }
> -  dump_dwarf (abfd, is_mainfile);
> +
> +  /* Instead of invoking dump_dwarf () here, load the section and invoke the
> +     display function directly.  Otherwise, the textual dump may contain
> +     duplicate output due to two invocations of dump_dwarf ().  See PR
> +     libsframe/33806.  */
> +  if (!load_specific_debug_section (sframe, sec, (void*) abfd))

Nit: Blank after * please.

I'm also uncertain about the comment. With the call to dump_dwarf() gone, I see
no reason to mention that function at all. SFrame isn't Dwarf, after all. Using
it was an attempt to take a shortcut, but as both the bug report and the
diffstat show, that wasn't really the way to go. As a result, I don't see a
need for a comment (to the effect it presently has) here at all.

> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/x86-64/objdump-sframe-2.d
> @@ -0,0 +1,33 @@
> +#name: objdump sframe with eh_frame
> +#source: sframe-func.s
> +#as: --gsframe
> +#objdump: --sframe -WF
> +#target: x86_64-*-*
> +#xfail: ![gas_sframe_check]

Why xfail? A target not supporting SFrame will never pass this test. It thus
shouldn't be a valid target for this test in the first place.

> --- a/binutils/testsuite/binutils-all/x86-64/sframe-func.s
> +++ b/binutils/testsuite/binutils-all/x86-64/sframe-func.s
> @@ -1,4 +1,3 @@
> -	.cfi_sections .sframe
>  	.cfi_startproc
>  	.long 8
>  	.cfi_def_cfa_offset 16

You must be altering an existing test here. If that line can be easily removed
without any compensation, why was it there in the first place?

Jan