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

[Indu Bhagat]

On 2/5/26 2:44 AM, Jens Remus wrote:
> On 2/5/2026 10:04 AM, Indu Bhagat wrote:
>> On 2/4/26 8:08 AM, Jens Remus wrote:
>>> On 2/3/2026 11:37 PM, Indu Bhagat via Binutils wrote:
>>
>>>> diff --git a/binutils/objdump.c b/binutils/objdump.c
>>>
>>>> @@ -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))
>>>> +    return;
>>>> +
>>>> +  struct dwarf_section *section = &debug_displays[sframe].section;
>>>> +  section->name = sect_name;
>>>> +  debug_displays[sframe].display (section, abfd);
>>>
>>> Why not free the loaded section afterwards, as dump_dwarf_section()
>>> would have done?
>>>
>>>     free_debug_section (sframe);
> 
>> free_debug_memory () invocation takes care of free'ing the debug section
>> data (including sframe).  So free_debug_section (sframe) looks unnecessary.
> Thanks!  That was not obvious to me.  Given that dump_dwarf_section()
> does it (and you will likely send a v2 anyway) I wonder whether it would
> still make sense so that others are not confused as well.
> 

I am not convinced that we should, as it looks to me that this is not 
needed.  May be your opinion shifts after the following remarks...
(Meanwhile I posted V2 without it, let me know)

> Tracing objdump --sframe passing multiple objects with .sframe as input
> and could confirm that free_debug_section() is called after processing
> each input.  I was worried, that the section data of all inputs would
> accumulate in a single objdump run.  Especially given it is only done for
> (is_mainfile), while dump_sframe_section() might be called for
> (is_mainfile || process_links).  Can we rule out that the latter may
> trigger dump_sframe_section()?
> 

AFAICT, the opening of the debug link to add to abfd object accounts for 
only debug sections in load_separate_debug_files ().  So .sframe from 
linked debug will not trigger dump_sframe_section ().

> At least I was not able to trick objdump to follow a link to .sframe:
> 
> $ cd path-to-binutils-build/ld/tmpdir
> $ ../ld-new -o sframe-sample sframe-foo.o sframe-bar.o
> $ cp sframe-sample sframe-sample.full
> $ ../../binutils/strip-new --strip-debug -R .sframe sframe-sample
> $ ../../binutils/objcopy --add-gnu-debuglink=sframe-sample.full sframe-sample
> $ ../../binutils/objdump --sframe sframe-sample
> No .sframe section present
> 

Hmm.  SFrame section is allocated, non-debug section, intended to be 
shipped with the binary.  So the above workflow doesnt quite fit SFrame 
section.  But if the users do the above for whatsoever reason, the above 
behaviour is reasonable I think.