[09/12] objdump/readelf: adjust for SFRAME_VERSION_2

Message ID 20230627212028.2138604-10-indu.bhagat@oracle.com
State Unresolved
Headers
Series SFrame Version 2 - definition and support |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Indu Bhagat June 27, 2023, 9:20 p.m. UTC
  Make adjustments to account for a new format version and its name.
Also shuffle some code to make it more readable and use the available
APIs rather than direct access.

objdump/readelf will show the following message to the user if .sframe
section in SFRAME_VERSION_1 format is seen:

 "No further information can be displayed.  SFrame version not
 supported."

In other words, like the rest of the binutils, only the current SFrame
format version, i.e., SFRAME_VERSION_2 is supported by the textual dump
facilities.

libsframe/
	* sframe-dump.c (dump_sframe_header): Add support for
	SFRAME_VERSION_2.
	(dump_sframe): Inform user if SFrame section in SFRAME_VERSION_1
	format is seen.
---
 libsframe/sframe-dump.c | 42 +++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)
  

Comments

Hans-Peter Nilsson June 28, 2023, 11:04 p.m. UTC | #1
On Tue, 27 Jun 2023, Indu Bhagat via Binutils wrote:
> Make adjustments to account for a new format version and its name.
> Also shuffle some code to make it more readable and use the available
> APIs rather than direct access.
> 
> objdump/readelf will show the following message to the user if .sframe
> section in SFRAME_VERSION_1 format is seen:
> 
>  "No further information can be displayed.  SFrame version not
>  supported."
> 
> In other words, like the rest of the binutils, only the current SFrame
> format version, i.e., SFRAME_VERSION_2 is supported by the textual dump
> facilities.

A decision that IMHO is counter to binutils philosophy (AFAIU), 
unless version 1 is actually generated by nobody nowhere.

If there are object files out there generated with version 1, 
support should be kept, at the very least for inspection tools.

brgds, H-P
  
Indu Bhagat June 29, 2023, 6:41 a.m. UTC | #2
On 6/28/23 16:04, Hans-Peter Nilsson wrote:
> On Tue, 27 Jun 2023, Indu Bhagat via Binutils wrote:
>> Make adjustments to account for a new format version and its name.
>> Also shuffle some code to make it more readable and use the available
>> APIs rather than direct access.
>>
>> objdump/readelf will show the following message to the user if .sframe
>> section in SFRAME_VERSION_1 format is seen:
>>
>>   "No further information can be displayed.  SFrame version not
>>   supported."
>>
>> In other words, like the rest of the binutils, only the current SFrame
>> format version, i.e., SFRAME_VERSION_2 is supported by the textual dump
>> facilities.
> 
> A decision that IMHO is counter to binutils philosophy (AFAIU),
> unless version 1 is actually generated by nobody nowhere.
> 
> If there are object files out there generated with version 1,
> support should be kept, at the very least for inspection tools.
> 

SFrame Version 1 format definition had issues related to unaligned 
accesses. In most cases of when there are version 1 .sframe sections out 
there, those applications should ideally be rebuilt to switch to using 
SFRAME_VERSION_2.  When other assemblers and linkers begin to add 
support for SFrame format, SFRAME_VERSION_2 should be used as it is the 
latest - there is little incentive to implement SFRAME_VERSION_1 there.

The trouble with supporting both SFRAME_VERSION_1 and SFRAME_VERSION_2 
in readelf/objdump is that the abstraction sframe_func_desc_entry used 
in libsframe is the same as the binary representation defined in 
include/sframe.h. This means that to support two versions of SFrame FDE, 
libsframe will need to revisit some of its APIs and will be a bit of 
rejig. I tried to do some of this, but it looked very clunky to me.

The choice to keep libsframe's internal representation for SFrame FDE to 
be the same as the on-disk representation has the advantage that an 
unwinder does not need to do any copy operations at run time.  It could 
simply read the SFrame FDE from memory.

Thanks
Indu
  

Patch

diff --git a/libsframe/sframe-dump.c b/libsframe/sframe-dump.c
index 4799652f727..bb83528bc79 100644
--- a/libsframe/sframe-dump.c
+++ b/libsframe/sframe-dump.c
@@ -43,27 +43,33 @@  is_sframe_abi_arch_aarch64 (sframe_decoder_ctx *sfd_ctx)
 static void
 dump_sframe_header (sframe_decoder_ctx *sfd_ctx)
 {
-  const char *verstr = NULL;
+  uint8_t ver;
+  uint8_t flags;
+  char *flags_str;
+  const char *ver_str = NULL;
   const sframe_header *header = &(sfd_ctx->sfd_header);
 
   /* Prepare SFrame section version string.  */
   const char *version_names[]
     = { "NULL",
-	"SFRAME_VERSION_1" };
-  unsigned char ver = header->sfh_preamble.sfp_version;
+	"SFRAME_VERSION_1",
+	"SFRAME_VERSION_2" };
+
+  /* PS: Keep SFRAME_HEADER_FLAGS_STR_MAX_LEN in sync if adding more members to
+     this array.  */
+  const char *flag_names[]
+    = { "SFRAME_F_FDE_SORTED",
+	"SFRAME_F_FRAME_POINTER" };
+
+  ver = sframe_decoder_get_version (sfd_ctx);
   if (ver <= SFRAME_VERSION)
-    verstr = version_names[ver];
+    ver_str = version_names[ver];
 
   /* Prepare SFrame section flags string.  */
-  unsigned char flags = header->sfh_preamble.sfp_flags;
-  char *flags_str
-    = (char*) calloc (sizeof (char), SFRAME_HEADER_FLAGS_STR_MAX_LEN);
+  flags = header->sfh_preamble.sfp_flags;
+  flags_str = (char*) calloc (sizeof (char), SFRAME_HEADER_FLAGS_STR_MAX_LEN);
   if (flags)
     {
-      const char *flag_names[]
-	= { "SFRAME_F_FDE_SORTED",
-	    "SFRAME_F_FRAME_POINTER" };
-      unsigned char flags = header->sfh_preamble.sfp_flags;
       if (flags & SFRAME_F_FDE_SORTED)
 	strcpy (flags_str, flag_names[0]);
       if (flags & SFRAME_F_FRAME_POINTER)
@@ -80,9 +86,9 @@  dump_sframe_header (sframe_decoder_ctx *sfd_ctx)
   printf ("\n");
   printf ("  %s :\n", subsec_name);
   printf ("\n");
-  printf ("    Version: %s\n", verstr);
+  printf ("    Version: %s\n", ver_str);
   printf ("    Flags: %s\n", flags_str);
-  printf ("    Num FDEs: %d\n", header->sfh_num_fdes);
+  printf ("    Num FDEs: %d\n", sframe_decoder_get_num_fidx (sfd_ctx));
   printf ("    Num FREs: %d\n", header->sfh_num_fres);
 
   free (flags_str);
@@ -203,6 +209,14 @@  dump_sframe_functions (sframe_decoder_ctx *sfd_ctx, uint64_t sec_addr)
 void
 dump_sframe (sframe_decoder_ctx *sfd_ctx, uint64_t sec_addr)
 {
+  uint8_t ver;
+
   dump_sframe_header (sfd_ctx);
-  dump_sframe_functions (sfd_ctx, sec_addr);
+
+  ver = sframe_decoder_get_version (sfd_ctx);
+  if (ver == SFRAME_VERSION)
+    dump_sframe_functions (sfd_ctx, sec_addr);
+  else
+    printf ("\n No further information can be displayed.  %s",
+	    "SFrame version not supported\n");
 }