[RFC,2/4] objdump, readelf: sframe: apply relocations before textual dump

Message ID 20250308073853.78738-3-indu.bhagat@oracle.com
State New
Headers
Series Fix relocatable links with SFrame section |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 fail Patch failed to apply

Commit Message

Indu Bhagat March 8, 2025, 7:38 a.m. UTC
  PR libsframe/32589 - function start address is zero in SFrame section dump

Currently, readelf and objdump display SFrame section in object file
with function start addresses of each function as 0.  This makes it
difficult to correlate SFrame stack trace information with the
individual functions in the object file.

Use the dump_dwarf () interface to dump SFrame section.  The current
infrastructure (for DWARF debug sections) already supports relocating
the section contents before dumping, so lets use that.

As a side effect, objdump now adds two new ways of dumping SFrame sections:
  - objdump -WS <obj>
  - objdump --dwarf=sframe
We do not publicize these options.  The lone advertised user interfacing
option (in --help) remains:
  - objdump --sframe

For objdump, we continue to keep the same error messaging as earlier (by
folding the check for section into the new dump_sframe_section ()
function):

$ objdump --sframe=sframe bubble_sort.o
...
No sframe section present

$ objdump --sframe=.sfram bubble_sort.o
...
No .sfram section present

Similarly for readelf:

$ readelf --sframe= bubble_sort.o
readelf: Error: Section name must be provided
$ readelf --sframe=.sfram bubble_sort.o
readelf: Warning: Section '.sfram' was not dumped because it does not exist
$ readelf --sframe=sframe bubble_sort.o
readelf: Warning: Section 'sframe' was not dumped because it does not exist

binutils/
        * dwarf.c (display_sframe): New definition.
        (dwarf_select_sections_all): Enable SFrame section too.
        (struct dwarf_section_display): Add entry for SFrame section.
        * dwarf.h (enum dwarf_section_display_enum): Add enumerator for
	SFrame.
        * objdump.c (dump_section_sframe): Remove.
        (dump_sframe_section): Add new definition.
        (dump_bfd): Use dump_sframe_section.
        * binutils/readelf.c (dump_section_as_sframe): Remove.

gas/testsuite/
        * gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.d: Adjust for
	new output.
---
 binutils/dwarf.c                              | 36 ++++++++++++
 binutils/dwarf.h                              |  1 +
 binutils/objdump.c                            | 55 +++++++------------
 binutils/readelf.c                            | 48 +++-------------
 .../cfi-sframe-aarch64-pac-ab-key-1.d         |  8 +--
 5 files changed, 68 insertions(+), 80 deletions(-)
  

Comments

Jan Beulich March 13, 2025, 2:09 p.m. UTC | #1
On 08.03.2025 08:38, Indu Bhagat wrote:
> PR libsframe/32589 - function start address is zero in SFrame section dump
> 
> Currently, readelf and objdump display SFrame section in object file
> with function start addresses of each function as 0.  This makes it
> difficult to correlate SFrame stack trace information with the
> individual functions in the object file.
> 
> Use the dump_dwarf () interface to dump SFrame section.  The current
> infrastructure (for DWARF debug sections) already supports relocating
> the section contents before dumping, so lets use that.
> 
> As a side effect, objdump now adds two new ways of dumping SFrame sections:
>   - objdump -WS <obj>
>   - objdump --dwarf=sframe
> We do not publicize these options.  The lone advertised user interfacing
> option (in --help) remains:
>   - objdump --sframe

How am I to understand "side effect" here? You don't need to ...

> @@ -12648,6 +12681,7 @@ static const debug_dump_long_opts debug_option_table[] =
>    /* For compatibility with earlier versions of readelf.  */
>    { 'r', "ranges", &do_debug_aranges, 1 },
>    { 's', "str", &do_debug_str, 1 },
> +  { 'S', "sframe", &do_sframe, 1 },
>    { 'T', "trace_aranges", &do_trace_aranges, 1 },
>    { 't', "pubtypes", &do_debug_pubtypes, 1 },
>    { 'U', "trace_info", &do_trace_info, 1 },

... make this and ...

> @@ -12806,6 +12841,7 @@ struct dwarf_section_display debug_displays[] =
>    { { ".debug_weaknames",   ".zdebug_weaknames",     "",	 NO_ABBREVS },	    display_debug_not_supported, NULL,		false },
>    { { ".gdb_index",	    "",			     "",	 NO_ABBREVS },	    display_gdb_index,	    &do_gdb_index,	false },
>    { { ".debug_names",	    "",			     "",	 NO_ABBREVS },	    display_debug_names,    &do_gdb_index,	false },
> +  { { ".sframe",	    "",			     "",	 NO_ABBREVS },	    display_sframe,	    &do_sframe,		true },
>    { { ".trace_info",	    "",			     "",	 ABBREV (trace_abbrev) }, display_trace_info, &do_trace_info,	true },
>    { { ".trace_abbrev",	    "",			     "",	 NO_ABBREVS },	    display_debug_abbrev,   &do_trace_abbrevs,	false },
>    { { ".trace_aranges",	    "",			     "",	 NO_ABBREVS },	    display_debug_aranges,  &do_trace_aranges,	false },

... this change, do you? Yet then that's a deliberate extra change, not
a side effect. Which in turn make me wonder why it's done, and then
deliberately without documenting the options.

Jan
  
Indu Bhagat March 17, 2025, 4:49 a.m. UTC | #2
On 3/13/25 7:09 AM, Jan Beulich wrote:
> On 08.03.2025 08:38, Indu Bhagat wrote:
>> PR libsframe/32589 - function start address is zero in SFrame section dump
>>
>> Currently, readelf and objdump display SFrame section in object file
>> with function start addresses of each function as 0.  This makes it
>> difficult to correlate SFrame stack trace information with the
>> individual functions in the object file.
>>
>> Use the dump_dwarf () interface to dump SFrame section.  The current
>> infrastructure (for DWARF debug sections) already supports relocating
>> the section contents before dumping, so lets use that.
>>
>> As a side effect, objdump now adds two new ways of dumping SFrame sections:
>>    - objdump -WS <obj>
>>    - objdump --dwarf=sframe
>> We do not publicize these options.  The lone advertised user interfacing
>> option (in --help) remains:
>>    - objdump --sframe
> 
> How am I to understand "side effect" here? You don't need to ...
> 
>> @@ -12648,6 +12681,7 @@ static const debug_dump_long_opts debug_option_table[] =
>>     /* For compatibility with earlier versions of readelf.  */
>>     { 'r', "ranges", &do_debug_aranges, 1 },
>>     { 's', "str", &do_debug_str, 1 },
>> +  { 'S', "sframe", &do_sframe, 1 },
>>     { 'T', "trace_aranges", &do_trace_aranges, 1 },
>>     { 't', "pubtypes", &do_debug_pubtypes, 1 },
>>     { 'U', "trace_info", &do_trace_info, 1 },
> 
> ... make this and ...
> 

This is possible to bypass, but there will be added code. Explanation here:

In the current patch, in objdump.c we invoke the dump_sframe_section () 
(which invokes the dump_dwarf ()), and in readelf.c we invoke the 
display_debug_section ().

Note that, it is the dwarf_select_sections_by_names () that sets the 
do_sframe which are then later relied on by dump_dwarf () and 
display_debug_section ().

It should be possible to instead do something like the following in 
objdump.c:

static void
dump_sframe_section (bfd *abfd, const char *sect_name)

{
   load_debug_section ((enum dwarf_section_display_enum) sframe, abfd);

   struct dwarf_section *sframe_sec = &debug_displays [sframe].section;

   debug_displays [sframe].display (sframe_sec, abfd);

   free_debug_section ((enum dwarf_section_display_enum) sframe);
}

and similar stubs in readelf.c. Is this preferable ?

IOW, that is what I meant as "side effect": Using the available APIs as 
they are (load_debug_section, dump_dwarf, display_debug_section) to dump 
SFrame sections as well, requires us to add entries in the 
debug_option_table [].  Hence, supporting objdump -WS unnecessarily.

>> @@ -12806,6 +12841,7 @@ struct dwarf_section_display debug_displays[] =
>>     { { ".debug_weaknames",   ".zdebug_weaknames",     "",	 NO_ABBREVS },	    display_debug_not_supported, NULL,		false },
>>     { { ".gdb_index",	    "",			     "",	 NO_ABBREVS },	    display_gdb_index,	    &do_gdb_index,	false },
>>     { { ".debug_names",	    "",			     "",	 NO_ABBREVS },	    display_debug_names,    &do_gdb_index,	false },
>> +  { { ".sframe",	    "",			     "",	 NO_ABBREVS },	    display_sframe,	    &do_sframe,		true },
>>     { { ".trace_info",	    "",			     "",	 ABBREV (trace_abbrev) }, display_trace_info, &do_trace_info,	true },
>>     { { ".trace_abbrev",	    "",			     "",	 NO_ABBREVS },	    display_debug_abbrev,   &do_trace_abbrevs,	false },
>>     { { ".trace_aranges",	    "",			     "",	 NO_ABBREVS },	    display_debug_aranges,  &do_trace_aranges,	false },
> 
> ... this change, do you? Yet then that's a deliberate extra change, not
> a side effect. Which in turn make me wonder why it's done, and then
> deliberately without documenting the options.
> 

... but this change cannot be skipped, as far as I can see.

For SFrame display, we would like to use the load_specific_debug_section 
() (or even load_debug_section () will work) somehow.  The interface and 
implementation of these load_(*_)debug_section () is such that they rely 
on debug_displays [] shown above.  So this addition to debug_displays[] 
is necessary, unless there is another preferable way to get relocated 
SFrame section contents (like, by refactoring the 
load_specific_debug_section () ? and duplicating some stubs in specific 
dump_sframe_section () APIs)

Indu
  
Jan Beulich March 17, 2025, 8:05 a.m. UTC | #3
On 17.03.2025 05:49, Indu Bhagat wrote:
> On 3/13/25 7:09 AM, Jan Beulich wrote:
>> On 08.03.2025 08:38, Indu Bhagat wrote:
>>> PR libsframe/32589 - function start address is zero in SFrame section dump
>>>
>>> Currently, readelf and objdump display SFrame section in object file
>>> with function start addresses of each function as 0.  This makes it
>>> difficult to correlate SFrame stack trace information with the
>>> individual functions in the object file.
>>>
>>> Use the dump_dwarf () interface to dump SFrame section.  The current
>>> infrastructure (for DWARF debug sections) already supports relocating
>>> the section contents before dumping, so lets use that.
>>>
>>> As a side effect, objdump now adds two new ways of dumping SFrame sections:
>>>    - objdump -WS <obj>
>>>    - objdump --dwarf=sframe
>>> We do not publicize these options.  The lone advertised user interfacing
>>> option (in --help) remains:
>>>    - objdump --sframe
>>
>> How am I to understand "side effect" here? You don't need to ...
>>
>>> @@ -12648,6 +12681,7 @@ static const debug_dump_long_opts debug_option_table[] =
>>>     /* For compatibility with earlier versions of readelf.  */
>>>     { 'r', "ranges", &do_debug_aranges, 1 },
>>>     { 's', "str", &do_debug_str, 1 },
>>> +  { 'S', "sframe", &do_sframe, 1 },
>>>     { 'T', "trace_aranges", &do_trace_aranges, 1 },
>>>     { 't', "pubtypes", &do_debug_pubtypes, 1 },
>>>     { 'U', "trace_info", &do_trace_info, 1 },
>>
>> ... make this and ...
>>
> 
> This is possible to bypass, but there will be added code. Explanation here:
> 
> In the current patch, in objdump.c we invoke the dump_sframe_section () 
> (which invokes the dump_dwarf ()), and in readelf.c we invoke the 
> display_debug_section ().
> 
> Note that, it is the dwarf_select_sections_by_names () that sets the 
> do_sframe which are then later relied on by dump_dwarf () and 
> display_debug_section ().
> 
> It should be possible to instead do something like the following in 
> objdump.c:
> 
> static void
> dump_sframe_section (bfd *abfd, const char *sect_name)
> 
> {
>    load_debug_section ((enum dwarf_section_display_enum) sframe, abfd);
> 
>    struct dwarf_section *sframe_sec = &debug_displays [sframe].section;
> 
>    debug_displays [sframe].display (sframe_sec, abfd);
> 
>    free_debug_section ((enum dwarf_section_display_enum) sframe);
> }
> 
> and similar stubs in readelf.c. Is this preferable ?

Not sure.

> IOW, that is what I meant as "side effect": Using the available APIs as 
> they are (load_debug_section, dump_dwarf, display_debug_section) to dump 
> SFrame sections as well, requires us to add entries in the 
> debug_option_table [].  Hence, supporting objdump -WS unnecessarily.

So what about using nil instead of 'S' in the table? That'll require a
minimal adjustment in dwarf_select_sections_by_letters(), but would avoid
such a side effect.

>>> @@ -12806,6 +12841,7 @@ struct dwarf_section_display debug_displays[] =
>>>     { { ".debug_weaknames",   ".zdebug_weaknames",     "",	 NO_ABBREVS },	    display_debug_not_supported, NULL,		false },
>>>     { { ".gdb_index",	    "",			     "",	 NO_ABBREVS },	    display_gdb_index,	    &do_gdb_index,	false },
>>>     { { ".debug_names",	    "",			     "",	 NO_ABBREVS },	    display_debug_names,    &do_gdb_index,	false },
>>> +  { { ".sframe",	    "",			     "",	 NO_ABBREVS },	    display_sframe,	    &do_sframe,		true },
>>>     { { ".trace_info",	    "",			     "",	 ABBREV (trace_abbrev) }, display_trace_info, &do_trace_info,	true },
>>>     { { ".trace_abbrev",	    "",			     "",	 NO_ABBREVS },	    display_debug_abbrev,   &do_trace_abbrevs,	false },
>>>     { { ".trace_aranges",	    "",			     "",	 NO_ABBREVS },	    display_debug_aranges,  &do_trace_aranges,	false },
>>
>> ... this change, do you? Yet then that's a deliberate extra change, not
>> a side effect. Which in turn make me wonder why it's done, and then
>> deliberately without documenting the options.
>>
> 
> ... but this change cannot be skipped, as far as I can see.
> 
> For SFrame display, we would like to use the load_specific_debug_section 
> () (or even load_debug_section () will work) somehow.  The interface and 
> implementation of these load_(*_)debug_section () is such that they rely 
> on debug_displays [] shown above.  So this addition to debug_displays[] 
> is necessary, unless there is another preferable way to get relocated 
> SFrame section contents (like, by refactoring the 
> load_specific_debug_section () ? and duplicating some stubs in specific 
> dump_sframe_section () APIs)

Duplicating code is often better avoided. However, somewhat along the lines
of the above suggestion, can't we add another flag to dwarf_section_display
to suppress the use of an entry when matching command line arguments?

Ftaod - neither of the adjustments needs to be done. But imo command line
options that we recognize would better also be documented. Which here means
if you want to avoid documenting some, let's try to avoid recognizing them.

Jan
  
Indu Bhagat March 19, 2025, 6:23 a.m. UTC | #4
On 3/17/25 1:05 AM, Jan Beulich wrote:
> On 17.03.2025 05:49, Indu Bhagat wrote:
>> On 3/13/25 7:09 AM, Jan Beulich wrote:
>>> On 08.03.2025 08:38, Indu Bhagat wrote:
>>>> PR libsframe/32589 - function start address is zero in SFrame section dump
>>>>
>>>> Currently, readelf and objdump display SFrame section in object file
>>>> with function start addresses of each function as 0.  This makes it
>>>> difficult to correlate SFrame stack trace information with the
>>>> individual functions in the object file.
>>>>
>>>> Use the dump_dwarf () interface to dump SFrame section.  The current
>>>> infrastructure (for DWARF debug sections) already supports relocating
>>>> the section contents before dumping, so lets use that.
>>>>
>>>> As a side effect, objdump now adds two new ways of dumping SFrame sections:
>>>>     - objdump -WS <obj>
>>>>     - objdump --dwarf=sframe
>>>> We do not publicize these options.  The lone advertised user interfacing
>>>> option (in --help) remains:
>>>>     - objdump --sframe
>>>
>>> How am I to understand "side effect" here? You don't need to ...
>>>
>>>> @@ -12648,6 +12681,7 @@ static const debug_dump_long_opts debug_option_table[] =
>>>>      /* For compatibility with earlier versions of readelf.  */
>>>>      { 'r', "ranges", &do_debug_aranges, 1 },
>>>>      { 's', "str", &do_debug_str, 1 },
>>>> +  { 'S', "sframe", &do_sframe, 1 },
>>>>      { 'T', "trace_aranges", &do_trace_aranges, 1 },
>>>>      { 't', "pubtypes", &do_debug_pubtypes, 1 },
>>>>      { 'U', "trace_info", &do_trace_info, 1 },
>>>
>>> ... make this and ...
>>>
>>
>> This is possible to bypass, but there will be added code. Explanation here:
>>
>> In the current patch, in objdump.c we invoke the dump_sframe_section ()
>> (which invokes the dump_dwarf ()), and in readelf.c we invoke the
>> display_debug_section ().
>>
>> Note that, it is the dwarf_select_sections_by_names () that sets the
>> do_sframe which are then later relied on by dump_dwarf () and
>> display_debug_section ().
>>
>> It should be possible to instead do something like the following in
>> objdump.c:
>>
>> static void
>> dump_sframe_section (bfd *abfd, const char *sect_name)
>>
>> {
>>     load_debug_section ((enum dwarf_section_display_enum) sframe, abfd);
>>
>>     struct dwarf_section *sframe_sec = &debug_displays [sframe].section;
>>
>>     debug_displays [sframe].display (sframe_sec, abfd);
>>
>>     free_debug_section ((enum dwarf_section_display_enum) sframe);
>> }
>>
>> and similar stubs in readelf.c. Is this preferable ?
> 
> Not sure.
> 
>> IOW, that is what I meant as "side effect": Using the available APIs as
>> they are (load_debug_section, dump_dwarf, display_debug_section) to dump
>> SFrame sections as well, requires us to add entries in the
>> debug_option_table [].  Hence, supporting objdump -WS unnecessarily.
> 
> So what about using nil instead of 'S' in the table? That'll require a
> minimal adjustment in dwarf_select_sections_by_letters(), but would avoid
> such a side effect.
> 

Sure.  This could work.

We could add an entry like the following in debug_option_table[]:

{ '\0', "sframe-internal-only", &do_sframe, 1 }.

And then in objdump.c, we do something like:

         case OPTION_DWARF:
           seenflag = true;
           if (optarg)
             {
-             if (dwarf_select_sections_by_names (optarg))
+             if (strcmp (optarg, "sframe-internal-only") == 0)
+               warn (_("Unrecognized debug option 
'sframe-internal-only'\n"));
+             else if (dwarf_select_sections_by_names (optarg))
                 dump_dwarf_section_info = true;
             }
           else
             {
               dwarf_select_sections_all ();
               dump_dwarf_section_info = true;
             }
           break;


             dump_sframe_section_name = xstrdup (optarg);

and then for OPTION_SFRAME, something like:

         case OPTION_SFRAME:
           dump_sframe_section_info = true;

           if (optarg)
             dump_sframe_section_name = xstrdup (optarg);

           /* Error checking for user-provided section name is done in
-            dump_sframe_section ().  Initialize for now with the 
default name:
-            "sframe".  */
-         dwarf_select_sections_by_names ("sframe");
+            dump_sframe_section ().  Initialize for now with the default
+            internal name: "sframe-internal-only".  */
+         dwarf_select_sections_by_names ("sframe-internal-only");

           seenflag = true;
           break;


Basically we disallow the "sframe-internal-only" explicitly from 
external/user input in --dwarf.  Similarly some stubs for readelf 
disallowing sframe from --debug-dump.

>>>> @@ -12806,6 +12841,7 @@ struct dwarf_section_display debug_displays[] =
>>>>      { { ".debug_weaknames",   ".zdebug_weaknames",     "",	 NO_ABBREVS },	    display_debug_not_supported, NULL,		false },
>>>>      { { ".gdb_index",	    "",			     "",	 NO_ABBREVS },	    display_gdb_index,	    &do_gdb_index,	false },
>>>>      { { ".debug_names",	    "",			     "",	 NO_ABBREVS },	    display_debug_names,    &do_gdb_index,	false },
>>>> +  { { ".sframe",	    "",			     "",	 NO_ABBREVS },	    display_sframe,	    &do_sframe,		true },
>>>>      { { ".trace_info",	    "",			     "",	 ABBREV (trace_abbrev) }, display_trace_info, &do_trace_info,	true },
>>>>      { { ".trace_abbrev",	    "",			     "",	 NO_ABBREVS },	    display_debug_abbrev,   &do_trace_abbrevs,	false },
>>>>      { { ".trace_aranges",	    "",			     "",	 NO_ABBREVS },	    display_debug_aranges,  &do_trace_aranges,	false },
>>>
>>> ... this change, do you? Yet then that's a deliberate extra change, not
>>> a side effect. Which in turn make me wonder why it's done, and then
>>> deliberately without documenting the options.
>>>
>>
>> ... but this change cannot be skipped, as far as I can see.
>>
>> For SFrame display, we would like to use the load_specific_debug_section
>> () (or even load_debug_section () will work) somehow.  The interface and
>> implementation of these load_(*_)debug_section () is such that they rely
>> on debug_displays [] shown above.  So this addition to debug_displays[]
>> is necessary, unless there is another preferable way to get relocated
>> SFrame section contents (like, by refactoring the
>> load_specific_debug_section () ? and duplicating some stubs in specific
>> dump_sframe_section () APIs)
> 
> Duplicating code is often better avoided. However, somewhat along the lines
> of the above suggestion, can't we add another flag to dwarf_section_display
> to suppress the use of an entry when matching command line arguments?
> 

I am not very sure how this will help...

> Ftaod - neither of the adjustments needs to be done. But imo command line
> options that we recognize would better also be documented. Which here means
> if you want to avoid documenting some, let's try to avoid recognizing them.
> 

... but the above suggestion using "sframe-internal-only" should help us 
avoid the said unnecessary options:

$ objdump -WS sort.o
objdump: Warning: Unrecognized debug letter option 'S'
...

$ objdump --dwarf=sframe sort.o
objdump: Warning: Unrecognized debug option 'sframe'
...

$ objdump --dwarf=sframe-internal-only sort.o
objdump: Warning: Unrecognized debug option 'sframe-internal-only'
...

Indu
  

Patch

diff --git a/binutils/dwarf.c b/binutils/dwarf.c
index 08bb623e405..b456871d4b6 100644
--- a/binutils/dwarf.c
+++ b/binutils/dwarf.c
@@ -30,6 +30,7 @@ 
 #include "gdb/gdb-index.h"
 #include "filenames.h"
 #include "safe-ctype.h"
+#include "sframe-api.h"
 #include <assert.h>
 
 #ifdef HAVE_LIBDEBUGINFOD
@@ -102,6 +103,7 @@  int do_debug_str;
 int do_debug_str_offsets;
 int do_debug_loc;
 int do_gdb_index;
+int do_sframe;
 int do_trace_info;
 int do_trace_abbrevs;
 int do_trace_aranges;
@@ -7678,6 +7680,37 @@  display_trace_info (struct dwarf_section *section, void *file)
   return process_debug_info (section, file, section->abbrev_sec, false, true);
 }
 
+static int
+display_sframe (struct dwarf_section *section, void *file ATTRIBUTE_UNUSED)
+{
+  sframe_decoder_ctx *sfd_ctx = NULL;
+  unsigned char *data = section->start;
+  size_t sf_size = section->size;
+  int err = 0;
+
+  if (strcmp (section->name, "") == 0)
+    {
+      error (_("Section name must be provided \n"));
+      return false;
+    }
+
+  /* Decode the contents of the section.  */
+  sfd_ctx = sframe_decode ((const char*)data, sf_size, &err);
+  if (!sfd_ctx || err)
+    {
+      error (_("SFrame decode failure: %s\n"), sframe_errmsg (err));
+      return false;
+    }
+
+  printf (_("Contents of the SFrame section %s:"), section->name);
+  /* Dump the contents as text.  */
+  dump_sframe (sfd_ctx, section->address);
+
+  sframe_decoder_free (&sfd_ctx);
+
+  return true;
+}
+
 static int
 display_debug_aranges (struct dwarf_section *section,
 		       void *file ATTRIBUTE_UNUSED)
@@ -12648,6 +12681,7 @@  static const debug_dump_long_opts debug_option_table[] =
   /* For compatibility with earlier versions of readelf.  */
   { 'r', "ranges", &do_debug_aranges, 1 },
   { 's', "str", &do_debug_str, 1 },
+  { 'S', "sframe", &do_sframe, 1 },
   { 'T', "trace_aranges", &do_trace_aranges, 1 },
   { 't', "pubtypes", &do_debug_pubtypes, 1 },
   { 'U', "trace_info", &do_trace_info, 1 },
@@ -12762,6 +12796,7 @@  dwarf_select_sections_all (void)
   do_debug_str = 1;
   do_debug_loc = 1;
   do_gdb_index = 1;
+  do_sframe = 1;
   do_trace_info = 1;
   do_trace_abbrevs = 1;
   do_trace_aranges = 1;
@@ -12806,6 +12841,7 @@  struct dwarf_section_display debug_displays[] =
   { { ".debug_weaknames",   ".zdebug_weaknames",     "",	 NO_ABBREVS },	    display_debug_not_supported, NULL,		false },
   { { ".gdb_index",	    "",			     "",	 NO_ABBREVS },	    display_gdb_index,	    &do_gdb_index,	false },
   { { ".debug_names",	    "",			     "",	 NO_ABBREVS },	    display_debug_names,    &do_gdb_index,	false },
+  { { ".sframe",	    "",			     "",	 NO_ABBREVS },	    display_sframe,	    &do_sframe,		true },
   { { ".trace_info",	    "",			     "",	 ABBREV (trace_abbrev) }, display_trace_info, &do_trace_info,	true },
   { { ".trace_abbrev",	    "",			     "",	 NO_ABBREVS },	    display_debug_abbrev,   &do_trace_abbrevs,	false },
   { { ".trace_aranges",	    "",			     "",	 NO_ABBREVS },	    display_debug_aranges,  &do_trace_aranges,	false },
diff --git a/binutils/dwarf.h b/binutils/dwarf.h
index 3419027ac1e..6f693b1eac2 100644
--- a/binutils/dwarf.h
+++ b/binutils/dwarf.h
@@ -102,6 +102,7 @@  enum dwarf_section_display_enum
   weaknames,
   gdb_index,
   debug_names,
+  sframe,
   trace_info,
   trace_abbrev,
   trace_aranges,
diff --git a/binutils/objdump.c b/binutils/objdump.c
index 8fdbe03c4fb..87695a31d67 100644
--- a/binutils/objdump.c
+++ b/binutils/objdump.c
@@ -4981,44 +4981,20 @@  dump_ctf (bfd *abfd ATTRIBUTE_UNUSED, const char *sect_name ATTRIBUTE_UNUSED,
 #endif
 
 static void
-dump_section_sframe (bfd *abfd ATTRIBUTE_UNUSED,
-		     const char * sect_name)
-{
-  asection *sec;
-  sframe_decoder_ctx *sfd_ctx = NULL;
-  bfd_size_type sf_size;
-  bfd_byte *sframe_data;
-  bfd_vma sf_vma;
-  int err = 0;
-
-  if (sect_name == NULL)
-    sect_name = ".sframe";
+dump_sframe_section (bfd *abfd, const char *sect_name, bool is_mainfile)
 
-  sec = read_section (abfd, sect_name, &sframe_data);
-  if (sec == NULL)
-    {
-      my_bfd_nonfatal (bfd_get_filename (abfd));
-      return;
-    }
-  sf_size = bfd_section_size (sec);
-  sf_vma = bfd_section_vma (sec);
-
-  /* Decode the contents of the section.  */
-  sfd_ctx = sframe_decode ((const char*)sframe_data, sf_size, &err);
-  if (!sfd_ctx)
+{
+  /* Error checking for user provided SFrame section name, if any.  */
+  if (sect_name)
     {
-      my_bfd_nonfatal (bfd_get_filename (abfd));
-      free (sframe_data);
-      return;
+      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;
+	}
     }
-
-  printf (_("Contents of the SFrame section %s:"),
-	  sanitize_string (sect_name));
-  /* Dump the contents as text.  */
-  dump_sframe (sfd_ctx, sf_vma);
-
-  sframe_decoder_free (&sfd_ctx);
-  free (sframe_data);
+  dump_dwarf (abfd, is_mainfile);
 }
 
 
@@ -5840,7 +5816,7 @@  dump_bfd (bfd *abfd, bool is_mainfile)
 	dump_ctf (abfd, dump_ctf_section_name, dump_ctf_parent_name,
 		  dump_ctf_parent_section_name);
       if (dump_sframe_section_info)
-	dump_section_sframe (abfd, dump_sframe_section_name);
+	dump_sframe_section (abfd, dump_sframe_section_name, is_mainfile);
       if (dump_stab_section_info)
 	dump_stabs (abfd);
       if (dump_reloc_info && ! disassemble)
@@ -6345,8 +6321,15 @@  main (int argc, char **argv)
 #endif
 	case OPTION_SFRAME:
 	  dump_sframe_section_info = true;
+
 	  if (optarg)
 	    dump_sframe_section_name = xstrdup (optarg);
+
+	  /* Error checking for user-provided section name is done in
+	     dump_sframe_section ().  Initialize for now with the default name:
+	     "sframe".  */
+	  dwarf_select_sections_by_names ("sframe");
+
 	  seenflag = true;
 	  break;
 	case 'G':
diff --git a/binutils/readelf.c b/binutils/readelf.c
index dd1871d8c75..8c2637c25ea 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -6583,9 +6583,15 @@  parse_args (struct dump_data *dumpdata, int argc, char ** argv)
 	  break;
 	case OPTION_SFRAME_DUMP:
 	  do_sframe = true;
+	  /* Fix PR/32589 but keep the error messaging same ?  */
+	  if (optarg != NULL && strcmp (optarg, "") == 0)
+	    {
+	      do_dump = true;
+	      error (_("Section name must be provided\n"));
+	    }
 	  /* Providing section name is optional.  request_dump (), however,
 	     thrives on non NULL optarg.  Handle it explicitly here.  */
-	  if (optarg != NULL)
+	  else if (optarg != NULL)
 	    request_dump (dumpdata, SFRAME_DUMP);
 	  else
 	    {
@@ -17101,44 +17107,6 @@  dump_section_as_ctf (Elf_Internal_Shdr * section, Filedata * filedata)
 }
 #endif
 
-static bool
-dump_section_as_sframe (Elf_Internal_Shdr * section, Filedata * filedata)
-{
-  void *		  data = NULL;
-  sframe_decoder_ctx	  *sfd_ctx = NULL;
-  const char *print_name = printable_section_name (filedata, section);
-
-  bool ret = true;
-  size_t sf_size;
-  int err = 0;
-
-  if (strcmp (print_name, "") == 0)
-    {
-      error (_("Section name must be provided \n"));
-      ret = false;
-      return ret;
-    }
-
-  data = get_section_contents (section, filedata);
-  sf_size = section->sh_size;
-  /* Decode the contents of the section.  */
-  sfd_ctx = sframe_decode ((const char*)data, sf_size, &err);
-  if (!sfd_ctx)
-    {
-      ret = false;
-      error (_("SFrame decode failure: %s\n"), sframe_errmsg (err));
-      goto fail;
-    }
-
-  printf (_("Contents of the SFrame section %s:"), print_name);
-  /* Dump the contents as text.  */
-  dump_sframe (sfd_ctx, section->sh_addr);
-
- fail:
-  free (data);
-  return ret;
-}
-
 static bool
 load_specific_debug_section (enum dwarf_section_display_enum  debug,
 			     const Elf_Internal_Shdr *        sec,
@@ -17706,7 +17674,7 @@  process_section_contents (Filedata * filedata)
 #endif
       if (dump & SFRAME_DUMP)
 	{
-	  if (! dump_section_as_sframe (section, filedata))
+	  if (! display_debug_section (i, section, filedata))
 	    res = false;
 	}
     }
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.d
index 599d4c4e795..7e3fd8ab622 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.d
@@ -18,10 +18,10 @@  Contents of the SFrame section .sframe:
     0+0004 +sp\+0 +u +u\[s\] +
     0+0008 +sp\+16 +c-16 +c-8\[s\] +
 
-    func idx \[1\]: pc = 0x0, size = 20 bytes, pauth = B key
+    func idx \[1\]: pc = 0xc, size = 20 bytes, pauth = B key
     STARTPC + CFA + FP +  RA +
-    0+0000 +sp\+0 +u +u +
-    0+0004 +sp\+0 +u +u\[s\] +
-    0+0008 +sp\+16 +c-16 +c-8\[s\] +
+    0+000c +sp\+0 +u +u +
+    0+0010 +sp\+0 +u +u\[s\] +
+    0+0014 +sp\+16 +c-16 +c-8\[s\] +
 
 #pass