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

[Indu Bhagat]

On 2/3/26 11:07 PM, Jan Beulich wrote:
> 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.
> 

OK.

> 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.
> 

You're right.  I was overthinking perhaps, this doesnt vouch for a code 
comment.  The explanation in commit log should suffice for posterity on 
what not to do.

>> --- /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.
> 

SFrame is not supported for x32.  gas_sframe_check will fail for x32, 
and the test is reported as XFAIL.  May be its better to use '#notarget: 
x86_64-*-gnux32' instead of '#xfail: ![gas_sframe_check]' ?

>> --- 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?
> 

Looks to be a carry over from a template from which this was copied in 
the first place.  The current users of sframe-func.s 
(binutils-all/x86-64/readelf-sframe-01.d and 
binutils-all/x86-64/objdump-sframe-01.d) are explicitly invoking 
--gsframe, like the objdump-sframe-2.d above, so the '.cfi_sections 
.sframe' was redundant for existing tests.

Thanks for reviewing