public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Indu Bhagat <indu.bhagat@oracle.com>
To: binutils@sourceware.org
Cc: Indu Bhagat <indu.bhagat@oracle.com>
Subject: [PATCH] sframe: fix the defined SFRAME_FRE_TYPE_*_LIMIT constants
Date: Thu,  5 Jan 2023 22:40:53 -0800	[thread overview]
Message-ID: <20230106064053.984817-1-indu.bhagat@oracle.com> (raw)

Hello,

This patch fixes an issue created by a previous commit (of mine) which
did not define the constant values correctly.  Without this fix in this
patch, the size of the SFrame section as generated by the assembler
(and hence, the final linked artifact generated by the linker) will be
larger than necessary.  More details in the commit log below.

This patch touches more than just libsframe, and will need an OK for
trunk.  I would like this patch to be included in trunk and
binutils-2_40-branch branches.

Testing notes:
- Tested the size of the SFrame sections in the generated binutils programs
  (host: aarch64) generated using -Wa,--gsframe.  The size of SFrame
  sections is now as expected.
- Reg tested native and cross builds on x86_64 and aarch64 (checked binutils,
  ld, gas, libctf, libsframe).
- try bot shows no new regressions.

Thanks,
------------------------------

An earlier commit 3f107464 defined the SFRAME_FRE_TYPE_*_LIMIT
constants.  These constants are used (by gas and libsframe) to pick an
SFrame FRE type based on the function size.  Those constants, however,
were buggy, causing the generated SFrame sections to be bloated as
SFRAME_FRE_TYPE_ADDR2/SFRAME_FRE_TYPE_ADDR4 got chosen more often than
necessary.

gas/
	* sframe-opt.c (sframe_estimate_size_before_relax): Use
	typecast.
	(sframe_convert_frag): Likewise.

libsframe/
	* sframe.c (sframe_calc_fre_type): Use a more
	appropriate type for argument.  Adjust the check for
	SFRAME_FRE_TYPE_ADDR4_LIMIT to keep it warning-free but
	meaningful.

include/
	* sframe-api.h (sframe_calc_fre_type): Use a more appropriate
	type for the argument.
	* sframe.h (SFRAME_FRE_TYPE_ADDR1_LIMIT): Correct the constant.
	(SFRAME_FRE_TYPE_ADDR2_LIMIT): Likewise.
	(SFRAME_FRE_TYPE_ADDR4_LIMIT): Likewise.
---
 gas/sframe-opt.c     | 12 ++++++------
 include/sframe-api.h |  2 +-
 include/sframe.h     |  9 ++++++---
 libsframe/sframe.c   |  6 ++++--
 4 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/gas/sframe-opt.c b/gas/sframe-opt.c
index 01138f28deb..ec0509f8977 100644
--- a/gas/sframe-opt.c
+++ b/gas/sframe-opt.c
@@ -53,9 +53,9 @@ sframe_estimate_size_before_relax (fragS *frag)
       widthS = exp->X_op_symbol;
       width = resolve_symbol_value (widthS);
 
-      if (width < SFRAME_FRE_TYPE_ADDR1_LIMIT)
+      if (width < (offsetT) SFRAME_FRE_TYPE_ADDR1_LIMIT)
 	ret = 1;
-      else if (width < SFRAME_FRE_TYPE_ADDR2_LIMIT)
+      else if (width < (offsetT) SFRAME_FRE_TYPE_ADDR2_LIMIT)
 	ret = 2;
       else
 	ret = 4;
@@ -123,9 +123,9 @@ sframe_convert_frag (fragS *frag)
       /* Calculate the applicable fre_type.  */
       fsizeS = exp->X_op_symbol;
       fsize = resolve_symbol_value (fsizeS);
-      if (fsize < SFRAME_FRE_TYPE_ADDR1_LIMIT)
+      if (fsize < (offsetT) SFRAME_FRE_TYPE_ADDR1_LIMIT)
 	fre_type = SFRAME_FRE_TYPE_ADDR1;
-      else if (fsize < SFRAME_FRE_TYPE_ADDR2_LIMIT)
+      else if (fsize < (offsetT) SFRAME_FRE_TYPE_ADDR2_LIMIT)
 	fre_type = SFRAME_FRE_TYPE_ADDR2;
       else
 	fre_type = SFRAME_FRE_TYPE_ADDR4;
@@ -150,11 +150,11 @@ sframe_convert_frag (fragS *frag)
       switch (frag->fr_subtype & 7)
 	{
 	case 1:
-	  gas_assert (fsize < SFRAME_FRE_TYPE_ADDR1_LIMIT);
+	  gas_assert (fsize < (offsetT) SFRAME_FRE_TYPE_ADDR1_LIMIT);
 	  frag->fr_literal[frag->fr_fix] = diff;
 	  break;
 	case 2:
-	  gas_assert (fsize < SFRAME_FRE_TYPE_ADDR2_LIMIT);
+	  gas_assert (fsize < (offsetT) SFRAME_FRE_TYPE_ADDR2_LIMIT);
 	  md_number_to_chars (frag->fr_literal + frag->fr_fix, diff, 2);
 	  break;
 	case 4:
diff --git a/include/sframe-api.h b/include/sframe-api.h
index cdffc25d2cd..405e30c27e8 100644
--- a/include/sframe-api.h
+++ b/include/sframe-api.h
@@ -96,7 +96,7 @@ sframe_fde_create_func_info (unsigned int fre_type, unsigned int fde_type);
 /* Gather the FRE type given the function size.  */
 
 extern unsigned int
-sframe_calc_fre_type (unsigned int func_size);
+sframe_calc_fre_type (size_t func_size);
 
 /* The SFrame Decoder.  */
 
diff --git a/include/sframe.h b/include/sframe.h
index 7e7840b605c..58ef07dcc21 100644
--- a/include/sframe.h
+++ b/include/sframe.h
@@ -304,7 +304,8 @@ typedef struct sframe_frame_row_entry_addr1
 
 /* Upper limit of start address in sframe_frame_row_entry_addr1
    is 0x100 (not inclusive).  */
-#define SFRAME_FRE_TYPE_ADDR1_LIMIT  ((SFRAME_FRE_TYPE_ADDR1 + 1) * 8)
+#define SFRAME_FRE_TYPE_ADDR1_LIMIT   \
+  (1ULL << ((SFRAME_FRE_TYPE_ADDR1 + 1) * 8))
 
 /* Used when SFRAME_FRE_TYPE_ADDR2 is specified as FRE type.  */
 typedef struct sframe_frame_row_entry_addr2
@@ -317,7 +318,8 @@ typedef struct sframe_frame_row_entry_addr2
 
 /* Upper limit of start address in sframe_frame_row_entry_addr2
    is 0x10000 (not inclusive).  */
-#define SFRAME_FRE_TYPE_ADDR2_LIMIT  ((SFRAME_FRE_TYPE_ADDR2 * 2) * 8)
+#define SFRAME_FRE_TYPE_ADDR2_LIMIT   \
+  (1ULL << ((SFRAME_FRE_TYPE_ADDR2 * 2) * 8))
 
 /* Used when SFRAME_FRE_TYPE_ADDR4 is specified as FRE type.  */
 typedef struct sframe_frame_row_entry_addr4
@@ -330,7 +332,8 @@ typedef struct sframe_frame_row_entry_addr4
 
 /* Upper limit of start address in sframe_frame_row_entry_addr2
    is 0x100000000 (not inclusive).  */
-#define SFRAME_FRE_TYPE_ADDR4_LIMIT  ((SFRAME_FRE_TYPE_ADDR4 * 2) * 8)
+#define SFRAME_FRE_TYPE_ADDR4_LIMIT   \
+  (1ULL << ((SFRAME_FRE_TYPE_ADDR4 * 2) * 8))
 
 #ifdef	__cplusplus
 }
diff --git a/libsframe/sframe.c b/libsframe/sframe.c
index 4aada1a25e0..6c778244c48 100644
--- a/libsframe/sframe.c
+++ b/libsframe/sframe.c
@@ -580,14 +580,16 @@ sframe_fde_create_func_info (unsigned int fre_type,
 /* FIXME API for linker.  Revisit if its better placed somewhere else?  */
 
 unsigned int
-sframe_calc_fre_type (unsigned int func_size)
+sframe_calc_fre_type (size_t func_size)
 {
   unsigned int fre_type = 0;
   if (func_size < SFRAME_FRE_TYPE_ADDR1_LIMIT)
     fre_type = SFRAME_FRE_TYPE_ADDR1;
   else if (func_size < SFRAME_FRE_TYPE_ADDR2_LIMIT)
     fre_type = SFRAME_FRE_TYPE_ADDR2;
-  else if (func_size < SFRAME_FRE_TYPE_ADDR4_LIMIT)
+  /* Adjust the check a bit so that it remains warning-free but meaningful
+     on 32-bit systems.  */
+  else if (func_size <= (size_t) (SFRAME_FRE_TYPE_ADDR4_LIMIT - 1))
     fre_type = SFRAME_FRE_TYPE_ADDR4;
   return fre_type;
 }
-- 
2.37.2


             reply	other threads:[~2023-01-06  6:41 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-06  6:40 Indu Bhagat [this message]
2023-01-06 11:05 ` Nick Clifton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230106064053.984817-1-indu.bhagat@oracle.com \
    --to=indu.bhagat@oracle.com \
    --cc=binutils@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).