[PATCH 0/2] libsframe: changes to semantics of two existing APIs

[PATCH 0/2] libsframe: changes to semantics of two existing APIs

[Indu Bhagat]

Hello,

This patch set needs review/discussion.

I had previously thought that such a change will necessitate symbol versioning
for the functions in question (Hence, the question towards the end in the
thread https://sourceware.org/pipermail/binutils/2023-May/127441.html).  But
after some thought, I am inclining towards not keeping versionined APIs for the
following functions in the current patch set:

  - extern int32_t
    sframe_fre_get_fp_offset (sframe_decoder_ctx *dctx,
			      sframe_frame_row_entry *fre, int *errp);
  - extern int32_t
    sframe_fre_get_ra_offset (sframe_decoder_ctx *dctx,
			      sframe_frame_row_entry *fre, int *errp);

The reason for not versioning the functions is that the proposed change, in
both the functions, is switching a previous error into non-error.  This should
not break a consumer like stack tracer: The only case of breakage will be when
the stack tracer code relies on the APIs returning specific error code.  The
latter should be unnecessary and rare, IMO.

More details in the respective commit logs. (PS: The tone in the commit logs
is more decisive than the cover letter here, because it is aligned to what the
patches do.)

Can you please take a look and let me know what you think ?  There is some
subjectivity to the matter, hence the request for comments before this is
committed.

Thanks,
Indu Bhagat (2):
  libsframe: update the semantics of sframe_fre_get_ra_offset
  libsframe: update the semantics of sframe_fre_get_fp_offset

 libsframe/sframe-dump.c | 12 ++++++++----
 libsframe/sframe.c      | 31 ++++++++++++++++++++-----------
 2 files changed, 28 insertions(+), 15 deletions(-)

-- 
2.39.2

[PATCH 1/2] libsframe: update the semantics of sframe_fre_get_ra_offset

[Indu Bhagat]

Until now, sframe_fre_get_ra_offset () would return
SFRAME_ERR_FREOFFSET_NOPRESENT if the ABI uses fixed RA offset (e.g.,
AMD64).  A stack tracer, then, will call an explicit
sframe_decoder_get_fixed_ra_offset () to get the RA offset.

On second look, it appears to make sense to hide these details of
whether the RA offset is fixed or not from the consumer.  Now, with the
changed semantics, the call to sframe_fre_get_ra_offset () will fetch
the fixed RA offset if applicable, or get the RA offset from FRE when
there is no fixed RA offset.

This patch changes the behavior of sframe_fre_get_ra_offset: it turns an
error into non-error.  This change is deemed a compatible ABI change,
because it should not break consumers unless they were relying on the
error; the latter seems unlikely.  Hence, this change of semantics does
not require symbol versioning to accompany.

Adjustments need to be made to ensure the textual dump remains the same
as preivous.  Currently, e.g., if RA is not being tracked per FRE,
following is seen with objdump --sframe:

    STARTPC         CFA       FP        RA
    000000000000NNNN  sp+X      u         u

libsframe/
	* sframe.c (sframe_fre_get_ra_offset): Return the fixed offset,
	if applicable.  Else return the RA offset from the FRE.
	* sframe-dump.c (dump_sframe_func_with_fres): Make adjustments
	to keep the textual dump same as previous.
---
 libsframe/sframe-dump.c | 12 ++++++++----
 libsframe/sframe.c      | 14 +++++++++-----
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/libsframe/sframe-dump.c b/libsframe/sframe-dump.c
index 6d266392cd7..2491b4391f3 100644
--- a/libsframe/sframe-dump.c
+++ b/libsframe/sframe-dump.c
@@ -164,11 +164,15 @@ dump_sframe_func_with_fres (sframe_decoder_ctx *sfd_ctx,
 	strcpy (temp, "u");
       printf ("%-10s", temp);
 
-      /* Dump RA info.  */
-      if (err[2] == 0)
-	sprintf (temp, "c%+d", ra_offset);
-      else
+      /* Dump RA info.
+	 If an ABI does not track RA offset, e.g., AMD64, display a 'u',
+	 else display the offset d as 'c+-d'.  */
+      if (sframe_decoder_get_fixed_ra_offset(sfd_ctx)
+	  != SFRAME_CFA_FIXED_RA_INVALID)
 	strcpy (temp, "u");
+      else if (err[2] == 0)
+	sprintf (temp, "c%+d", ra_offset);
+
       /* Mark SFrame FRE's RA information with "[s]" if the RA is mangled
 	 with signature bits.  */
       const char *ra_mangled_p_str
diff --git a/libsframe/sframe.c b/libsframe/sframe.c
index a5f4a7f6519..8d9d194a8a8 100644
--- a/libsframe/sframe.c
+++ b/libsframe/sframe.c
@@ -665,11 +665,15 @@ int32_t
 sframe_fre_get_ra_offset (sframe_decoder_ctx *dctx,
 			  sframe_frame_row_entry *fre, int *errp)
 {
-  sframe_header *dhp = sframe_decoder_get_header (dctx);
-  /* If the RA offset was not being tracked, return an error code so the caller
-     can gather the fixed RA offset from the SFrame header.  */
-  if (dhp->sfh_cfa_fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID)
-    return sframe_set_errno (errp, SFRAME_ERR_FREOFFSET_NOPRESENT);
+  int8_t ra_offset = sframe_decoder_get_fixed_ra_offset (dctx);
+  /* If the RA offset was not being tracked, return the fixed RA offset
+     from the SFrame header.  */
+  if (ra_offset != SFRAME_CFA_FIXED_RA_INVALID)
+    {
+      if (errp)
+	*errp = 0;
+      return ra_offset;
+    }
 
   /* Otherwise, get the RA offset from the FRE.  */
   return sframe_get_fre_offset (fre, SFRAME_FRE_RA_OFFSET_IDX, errp);
-- 
2.39.2

[PATCH 2/2] libsframe: update the semantics of sframe_fre_get_fp_offset

[Indu Bhagat]

Until now, sframe_fre_get_fp_offset () would return
SFRAME_ERR_FREOFFSET_NOPRESENT if the ABI uses fixed FP offset.  A stack
tracer, then, would call an explicit sframe_decoder_get_fixed_fp_offset ()
to get the FP offset.

On second look, it appears to make sense to hide these details of
whether the FP offset is fixed or not in an ABI from the consumer.  Now,
with the changed semantics, the call to sframe_fre_get_fp_offset () will
fetch the fixed FP offset if applicable, or get the FP offset from FRE
when there is no fixed FP offset.

This patch changes the behavior of sframe_fre_get_fp_offset (): it turns
an error into non-error.  This change is deemed a compatible ABI change,
because it should not break consumers unless they were relying on the
error; the latter seems unlikely.  Hence, this change of semantics of
the API does not require symbol versioning to accompany.

libsframe/
	* sframe.c (sframe_fre_get_fp_offset): Return the fixed offset, if
	applicable. Else return the FP offset from the FRE.
---
 libsframe/sframe.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/libsframe/sframe.c b/libsframe/sframe.c
index 8d9d194a8a8..ba70bba294f 100644
--- a/libsframe/sframe.c
+++ b/libsframe/sframe.c
@@ -644,16 +644,21 @@ sframe_fre_get_fp_offset (sframe_decoder_ctx *dctx,
 			  sframe_frame_row_entry *fre, int *errp)
 {
   uint32_t fp_offset_idx = 0;
-  sframe_header *dhp = sframe_decoder_get_header (dctx);
-  /* If the FP offset is not being tracked, return an error code so the caller
-     can gather the fixed FP offset from the SFrame header.  */
-  if (dhp->sfh_cfa_fixed_fp_offset != SFRAME_CFA_FIXED_FP_INVALID)
-    return sframe_set_errno (errp, SFRAME_ERR_FREOFFSET_NOPRESENT);
+  int8_t fp_offset = sframe_decoder_get_fixed_fp_offset (dctx);
+  /* If the FP offset is not being tracked, return the fixed FP offset
+     from the SFrame header.  */
+  if (fp_offset != SFRAME_CFA_FIXED_FP_INVALID)
+    {
+      if (errp)
+	*errp = 0;
+      return fp_offset;
+    }
 
   /* In some ABIs, the stack offset to recover RA (using the CFA) from is
      fixed (like AMD64).  In such cases, the stack offset to recover FP will
      appear at the second index.  */
-  fp_offset_idx = ((dhp->sfh_cfa_fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID)
+  fp_offset_idx = ((sframe_decoder_get_fixed_ra_offset (dctx)
+		    != SFRAME_CFA_FIXED_RA_INVALID)
 		   ? SFRAME_FRE_RA_OFFSET_IDX
 		   : SFRAME_FRE_FP_OFFSET_IDX);
   return sframe_get_fre_offset (fre, fp_offset_idx, errp);
-- 
2.39.2