* [PATCH v4 01/15] x86: Remove unused SFrame CFI RA register variable
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-24 14:51 ` Jan Beulich
2024-06-24 14:23 ` [PATCH v4 02/15] gas: Enhance arch-specific SFrame configuration descriptions Jens Remus
` (15 subsequent siblings)
16 siblings, 1 reply; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat
Cc: Jens Remus, Andreas Krebbel, Jan Beulich, Jan Hubicka,
Andreas Jaeger, H.J. Lu
gas/
* config/tc-i386.c: Remove unused SFrame CFI RA register
variable.
Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v2 -> v3:
- Corrected formatting of ChangeLog in commit message.
gas/config/tc-i386.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index e94b93701552..343848ad052d 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -639,7 +639,6 @@ static int shared = 0;
unsigned int x86_sframe_cfa_sp_reg;
/* The other CFA base register for SFrame stack trace info. */
unsigned int x86_sframe_cfa_fp_reg;
-unsigned int x86_sframe_cfa_ra_reg;
#endif
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v4 01/15] x86: Remove unused SFrame CFI RA register variable
2024-06-24 14:23 ` [PATCH v4 01/15] x86: Remove unused SFrame CFI RA register variable Jens Remus
@ 2024-06-24 14:51 ` Jan Beulich
2024-06-24 16:13 ` Jens Remus
0 siblings, 1 reply; 32+ messages in thread
From: Jan Beulich @ 2024-06-24 14:51 UTC (permalink / raw)
To: Jens Remus
Cc: Andreas Krebbel, Jan Hubicka, Andreas Jaeger, H.J. Lu, binutils,
Indu Bhagat
On 24.06.2024 16:23, Jens Remus wrote:
> gas/
> * config/tc-i386.c: Remove unused SFrame CFI RA register
> variable.
Nit: The more "canonical" way would be
* config/tc-i386.c (x86_sframe_cfa_ra_reg): Remove.
> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
As to these tags, besides them wanting to be in chronological order,
would you mind pointing out where these were taken from? I've checked
and I have no record of having offered an Acked-by:, and I also
couldn't find any Reviewed-by anywhere on the list. I don't know how
bad it really is (so please forgive if the wording is stronger than
actually needed here), but I don't think tags should be falsified. I
may be entirely wrong, though.
With the (tag free) replies you've got, I don't think there would
have been a need to re-post anyway - you could simply have committed
the adjusted form.
Finally the title says 01/15, yet by now (about 20 min after the mail
arrived) I didn't get any further parts of such a 15-patch series.
Oddly enough [1] doesn't even have this one, yet I don't know how
often it would be refreshed.
I'm sorry for the ranting,
Jan
[1] https://sourceware.org/pipermail/binutils/2024-June/thread.html
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 01/15] x86: Remove unused SFrame CFI RA register variable
2024-06-24 14:51 ` Jan Beulich
@ 2024-06-24 16:13 ` Jens Remus
2024-06-25 5:56 ` Jan Beulich
2024-06-25 23:56 ` Indu Bhagat
0 siblings, 2 replies; 32+ messages in thread
From: Jens Remus @ 2024-06-24 16:13 UTC (permalink / raw)
To: Jan Beulich
Cc: Andreas Krebbel, Jan Hubicka, Andreas Jaeger, H.J. Lu, binutils,
Indu Bhagat
Hello Jan!
Am 24.06.2024 um 16:51 schrieb Jan Beulich:
> On 24.06.2024 16:23, Jens Remus wrote:
>> gas/
>> * config/tc-i386.c: Remove unused SFrame CFI RA register
>> variable.
>
> Nit: The more "canonical" way would be
>
> * config/tc-i386.c (x86_sframe_cfa_ra_reg): Remove.
>
Thanks! I am still learning how to get those GNU Changelog entries in
the commit message correct. While this series evolved later added
patches do have the function names. Somehow I did not consider to reword
them all.
I will reword the commit message as you suggested. Do you want me to
send a v5 (after Indu reviewed the whole series)?
>> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
>> Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>
> As to these tags, besides them wanting to be in chronological order,
> would you mind pointing out where these were taken from? I've checked
> and I have no record of having offered an Acked-by:, and I also
> couldn't find any Reviewed-by anywhere on the list. I don't know how
> bad it really is (so please forgive if the wording is stronger than
> actually needed here), but I don't think tags should be falsified. I
> may be entirely wrong, though.
You concerns are fair. Let me try to clarify. I am used to the Linux
kernel style commit message trailers from my other short prior
open-source development work experience. To best adhere to the GNU
Binutils push after approval rule my intention was to record any reviews
and the required approval. Given those tailers are not widely used on
the GNU Binutils mailing list (this seems different for GDB [2]) I took
some freedom translating the feedback received to my patches. If this
was wrong I apologize for my naive approach.
Andreas is my co-worker and GNU Binutils maintainer for s390. He had
reviewed an early version of this patch series including this specific
patch as part of an internal-review. This is the source of his Reviewed-by.
Indu had reviewed the patch in
<ebeb4ed6-23df-468c-985b-fb47a3fede00@oracle.com>
(https://sourceware.org/pipermail/binutils/2024-February/132653.html). I
took the freedom to translate her "LGTM" to "Reviewed-by".
You reviewed the patch in
<55490c94-bb76-4c23-8214-bfb8a9d193e2@suse.com>
(https://sourceware.org/pipermail/binutils/2024-February/132669.html)
and <a10708f0-62ec-4ecd-a200-baef78102f63@suse.com>
(https://sourceware.org/pipermail/binutils/2024-February/132686.html).
Since you are one of the GNU Binutils maintainers for x86 I translated
your "Okay. So Jens - feel free to put in." from the latter to
"Acked-by", since I assumed this to be you ok to push for this
particular patch.
Somehow I am used to keep my own Signed-off-by trailer last, to denote
that I confirm any above trailers.
>
> With the (tag free) replies you've got, I don't think there would
> have been a need to re-post anyway - you could simply have committed
> the adjusted form.
I can remove all of the trailers and stop adding them, if that is the
preferred approach for GNU Binutils. I honestly thought nobody would
feel hurt if I used them as explained above.
I must admit that I felt a bit uncomfortable not having them. Thinking
that through again, I come to the conclusion that they are not really
adding much value, since they don't include any reference to their
origin. To not loose track of which patches are already
reviewed/approved I could track the progress in my private Git notes
instead.
>
> Finally the title says 01/15, yet by now (about 20 min after the mail
> arrived) I didn't get any further parts of such a 15-patch series.
> Oddly enough [1] doesn't even have this one, yet I don't know how
> often it would be refreshed.
I have the impression my e-mails to the GNU Binutils mailing list are
lately withheld for some reason. At least my recent patch series "[PATCH
0/2] aarch64: Fixes access to struct aarch64_opnd_info members" from
2024-06-21 did only show up the next day on the mailing list for me.
Since you are on Cc only for this one x86-specifc patch, you got it
earlier than the rest of the series, which should arrive soon.
>
> I'm sorry for the ranting,
> Jan
No problem, this is absolutely fine! I prefer to hear any concerns early
and learn and adapt, than to continue doing things wrong.
>
> [1] https://sourceware.org/pipermail/binutils/2024-June/thread.html
>
[2] GDB Contribution Checklist, section "12. Receiving positive reviews",
https://sourceware.org/gdb/wiki/ContributionChecklist#Receiving_positive_reviews
Regards,
Jens
--
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+49-7031-16-1128 Office
jremus@de.ibm.com
IBM
IBM Deutschland Research & Development GmbH; Vorsitzender des
Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der
Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 01/15] x86: Remove unused SFrame CFI RA register variable
2024-06-24 16:13 ` Jens Remus
@ 2024-06-25 5:56 ` Jan Beulich
2024-06-25 23:56 ` Indu Bhagat
1 sibling, 0 replies; 32+ messages in thread
From: Jan Beulich @ 2024-06-25 5:56 UTC (permalink / raw)
To: Jens Remus
Cc: Andreas Krebbel, Jan Hubicka, Andreas Jaeger, H.J. Lu, binutils,
Indu Bhagat
On 24.06.2024 18:13, Jens Remus wrote:
> Am 24.06.2024 um 16:51 schrieb Jan Beulich:
>> On 24.06.2024 16:23, Jens Remus wrote:
>>> gas/
>>> * config/tc-i386.c: Remove unused SFrame CFI RA register
>>> variable.
>>
>> Nit: The more "canonical" way would be
>>
>> * config/tc-i386.c (x86_sframe_cfa_ra_reg): Remove.
>>
>
> Thanks! I am still learning how to get those GNU Changelog entries in
> the commit message correct. While this series evolved later added
> patches do have the function names. Somehow I did not consider to reword
> them all.
>
> I will reword the commit message as you suggested. Do you want me to
> send a v5 (after Indu reviewed the whole series)?
I'd recommend not waiting until the full series is ready, at least for
entirely independent patches like this one. Just put it in.
Jan
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 01/15] x86: Remove unused SFrame CFI RA register variable
2024-06-24 16:13 ` Jens Remus
2024-06-25 5:56 ` Jan Beulich
@ 2024-06-25 23:56 ` Indu Bhagat
1 sibling, 0 replies; 32+ messages in thread
From: Indu Bhagat @ 2024-06-25 23:56 UTC (permalink / raw)
To: Jens Remus, Jan Beulich
Cc: Andreas Krebbel, Jan Hubicka, Andreas Jaeger, H.J. Lu, binutils
On 6/24/24 09:13, Jens Remus wrote:
> Am 24.06.2024 um 16:51 schrieb Jan Beulich:
>> On 24.06.2024 16:23, Jens Remus wrote:
>>> gas/
>>> * config/tc-i386.c: Remove unused SFrame CFI RA register
>>> variable.
>>
>> Nit: The more "canonical" way would be
>>
>> * config/tc-i386.c (x86_sframe_cfa_ra_reg): Remove.
>>
>
> Thanks! I am still learning how to get those GNU Changelog entries in
> the commit message correct. While this series evolved later added
> patches do have the function names. Somehow I did not consider to reword
> them all.
>
If it helps, you can try using contrib/mklog.py script in GCC upstream
for your future patches. The script is certainly helpful to get a first
outline of the ChangeLog text. But it does sometimes mis-attribute
hunks to entity names, so double checking is necessary.
That said, Changelog entries in commit message in Binutils project are
not mandatory anymore. Its just that I find them useful and I appreciate
you keeping them for these SFrame patches.
Thanks
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v4 02/15] gas: Enhance arch-specific SFrame configuration descriptions
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
2024-06-24 14:23 ` [PATCH v4 01/15] x86: Remove unused SFrame CFI RA register variable Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-25 23:56 ` Indu Bhagat
2024-06-24 14:23 ` [PATCH v4 03/15] readelf/objdump: Dump SFrame CFA fixed FP and RA offsets Jens Remus
` (14 subsequent siblings)
16 siblings, 1 reply; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
Explicitly mention "SFrame" in the descriptions for the architecture-
specific SFrame configuration macros, variables, and functions.
Use the term "frame pointer" (FP) instead of "base pointer". This aligns
with the terminology used in the SFrame specification. Additionally it
helps not to confuse "base-pointer register" with the term "BASE_REG"
used in the specification to denote either the SP or FP register.
Specify what the SFRAME_CFA_*_REG register numbers are used for:
- SP (stack pointer): CFA tracking
- FP (frame pointer): CFA and FP tracking
- RA (return address): RA tracking
Align the descriptions for definitions in the source files to the
declarations in the header files.
gas/
* config/tc-aarch64.h: Enhance architecture-specific SFrame
configuration descriptions.
* config/tc-aarch64.c: Likewise.
* config/tc-i386.h: Likewise.
* config/tc-i386.c: Likewise.
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v3 -> v4:
- Removed dash in terms "stack pointer", "frame pointer", and "return
address" as requested by Indu.
Changes v2 -> v3:
- Add "SFrame" to architecture-specific SFrame macro, variable, and
function descriptions as suggested by Indu. Do so for all and not
only those previously touched.
- Reword further SFrame macro, variable, and function descriptions
to align with those previously touched.
- Align description of definition in source with declaration in header.
- Corrected formatting of ChangeLog in commit message.
- Changed commit subject prefix from "sframe" to "gas".
gas/config/tc-aarch64.c | 6 +++---
gas/config/tc-aarch64.h | 12 ++++++------
gas/config/tc-i386.c | 5 +++++
gas/config/tc-i386.h | 10 +++++-----
4 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
index 885ea65b8eb0..6ec883734375 100644
--- a/gas/config/tc-aarch64.c
+++ b/gas/config/tc-aarch64.c
@@ -8984,7 +8984,7 @@ aarch64_support_sframe_p (void)
return (aarch64_abi == AARCH64_ABI_LP64);
}
-/* Specify if RA tracking is needed. */
+/* Whether SFrame return address tracking is needed. */
bool
aarch64_sframe_ra_tracking_p (void)
@@ -8992,8 +8992,8 @@ aarch64_sframe_ra_tracking_p (void)
return true;
}
-/* Specify the fixed offset to recover RA from CFA.
- (useful only when RA tracking is not needed). */
+/* The fixed offset from CFA for SFrame to recover the return address.
+ (useful only when SFrame RA tracking is not needed). */
offsetT
aarch64_sframe_cfa_ra_offset (void)
diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
index 1b8badad9fdc..0063e85a7f13 100644
--- a/gas/config/tc-aarch64.h
+++ b/gas/config/tc-aarch64.h
@@ -267,24 +267,24 @@ extern void aarch64_after_parse_args (void);
extern bool aarch64_support_sframe_p (void);
#define support_sframe_p aarch64_support_sframe_p
-/* The stack-pointer register number for SFrame stack trace info. */
+/* The stack pointer DWARF register number for SFrame CFA tracking. */
extern unsigned int aarch64_sframe_cfa_sp_reg;
#define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
-/* The base-pointer register number for CFA stack trace info. */
+/* The frame pointer DWARF register number for SFrame CFA and FP tracking. */
extern unsigned int aarch64_sframe_cfa_fp_reg;
#define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
-/* The return address register number for CFA stack trace info. */
+/* The return address DWARF register number for SFrame RA tracking. */
extern unsigned int aarch64_sframe_cfa_ra_reg;
#define SFRAME_CFA_RA_REG aarch64_sframe_cfa_ra_reg
-/* Specify if RA tracking is needed. */
+/* Whether SFrame return address tracking is needed. */
extern bool aarch64_sframe_ra_tracking_p (void);
#define sframe_ra_tracking_p aarch64_sframe_ra_tracking_p
-/* Specify the fixed offset to recover RA from CFA.
- (useful only when RA tracking is not needed). */
+/* The fixed offset from CFA for SFrame to recover the return address.
+ (useful only when SFrame RA tracking is not needed). */
extern offsetT aarch64_sframe_cfa_ra_offset (void);
#define sframe_cfa_ra_offset aarch64_sframe_cfa_ra_offset
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 343848ad052d..f2c354108ea5 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -11440,6 +11440,7 @@ x86_cleanup (void)
subseg_set (seg, subseg);
}
+/* Whether SFrame stack trace info is supported. */
bool
x86_support_sframe_p (void)
{
@@ -11447,6 +11448,7 @@ x86_support_sframe_p (void)
return (x86_elf_abi == X86_64_ABI);
}
+/* Whether SFrame return address tracking is needed. */
bool
x86_sframe_ra_tracking_p (void)
{
@@ -11456,6 +11458,8 @@ x86_sframe_ra_tracking_p (void)
return false;
}
+/* The fixed offset from CFA for SFrame to recover the return address.
+ (useful only when SFrame RA tracking is not needed). */
offsetT
x86_sframe_cfa_ra_offset (void)
{
@@ -11463,6 +11467,7 @@ x86_sframe_cfa_ra_offset (void)
return (offsetT) -8;
}
+/* The abi/arch indentifier for SFrame. */
unsigned char
x86_sframe_get_abi_arch (void)
{
diff --git a/gas/config/tc-i386.h b/gas/config/tc-i386.h
index 7aae7a33dc14..8b0fc6398aac 100644
--- a/gas/config/tc-i386.h
+++ b/gas/config/tc-i386.h
@@ -441,20 +441,20 @@ extern bool x86_scfi_callee_saved_p (uint32_t dw2reg_num);
extern bool x86_support_sframe_p (void);
#define support_sframe_p x86_support_sframe_p
-/* The stack-pointer register number for SFrame stack trace info. */
+/* The stack pointer DWARF register number for SFrame CFA tracking. */
extern unsigned int x86_sframe_cfa_sp_reg;
#define SFRAME_CFA_SP_REG x86_sframe_cfa_sp_reg
-/* The frame-pointer register number for SFrame stack trace info. */
+/* The frame pointer DWARF register number for SFrame CFA and FP tracking. */
extern unsigned int x86_sframe_cfa_fp_reg;
#define SFRAME_CFA_FP_REG x86_sframe_cfa_fp_reg
-/* Specify if RA tracking is needed. */
+/* Whether SFrame return address tracking is needed. */
extern bool x86_sframe_ra_tracking_p (void);
#define sframe_ra_tracking_p x86_sframe_ra_tracking_p
-/* Specify the fixed offset to recover RA from CFA.
- (useful only when RA tracking is not needed). */
+/* The fixed offset from CFA for SFrame to recover the return address.
+ (useful only when SFrame RA tracking is not needed). */
extern offsetT x86_sframe_cfa_ra_offset (void);
#define sframe_cfa_ra_offset x86_sframe_cfa_ra_offset
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v4 02/15] gas: Enhance arch-specific SFrame configuration descriptions
2024-06-24 14:23 ` [PATCH v4 02/15] gas: Enhance arch-specific SFrame configuration descriptions Jens Remus
@ 2024-06-25 23:56 ` Indu Bhagat
0 siblings, 0 replies; 32+ messages in thread
From: Indu Bhagat @ 2024-06-25 23:56 UTC (permalink / raw)
To: Jens Remus, binutils; +Cc: Andreas Krebbel
On 6/24/24 07:23, Jens Remus wrote:
> Explicitly mention "SFrame" in the descriptions for the architecture-
> specific SFrame configuration macros, variables, and functions.
>
> Use the term "frame pointer" (FP) instead of "base pointer". This aligns
> with the terminology used in the SFrame specification. Additionally it
> helps not to confuse "base-pointer register" with the term "BASE_REG"
> used in the specification to denote either the SP or FP register.
>
> Specify what the SFRAME_CFA_*_REG register numbers are used for:
> - SP (stack pointer): CFA tracking
> - FP (frame pointer): CFA and FP tracking
> - RA (return address): RA tracking
>
> Align the descriptions for definitions in the source files to the
> declarations in the header files.
>
> gas/
> * config/tc-aarch64.h: Enhance architecture-specific SFrame
> configuration descriptions.
> * config/tc-aarch64.c: Likewise.
> * config/tc-i386.h: Likewise.
> * config/tc-i386.c: Likewise.
>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
LGTM.
Thanks
> ---
>
> Notes (jremus):
> Changes v3 -> v4:
> - Removed dash in terms "stack pointer", "frame pointer", and "return
> address" as requested by Indu.
>
> Changes v2 -> v3:
> - Add "SFrame" to architecture-specific SFrame macro, variable, and
> function descriptions as suggested by Indu. Do so for all and not
> only those previously touched.
> - Reword further SFrame macro, variable, and function descriptions
> to align with those previously touched.
> - Align description of definition in source with declaration in header.
> - Corrected formatting of ChangeLog in commit message.
> - Changed commit subject prefix from "sframe" to "gas".
>
> gas/config/tc-aarch64.c | 6 +++---
> gas/config/tc-aarch64.h | 12 ++++++------
> gas/config/tc-i386.c | 5 +++++
> gas/config/tc-i386.h | 10 +++++-----
> 4 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
> index 885ea65b8eb0..6ec883734375 100644
> --- a/gas/config/tc-aarch64.c
> +++ b/gas/config/tc-aarch64.c
> @@ -8984,7 +8984,7 @@ aarch64_support_sframe_p (void)
> return (aarch64_abi == AARCH64_ABI_LP64);
> }
>
> -/* Specify if RA tracking is needed. */
> +/* Whether SFrame return address tracking is needed. */
>
> bool
> aarch64_sframe_ra_tracking_p (void)
> @@ -8992,8 +8992,8 @@ aarch64_sframe_ra_tracking_p (void)
> return true;
> }
>
> -/* Specify the fixed offset to recover RA from CFA.
> - (useful only when RA tracking is not needed). */
> +/* The fixed offset from CFA for SFrame to recover the return address.
> + (useful only when SFrame RA tracking is not needed). */
>
> offsetT
> aarch64_sframe_cfa_ra_offset (void)
> diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
> index 1b8badad9fdc..0063e85a7f13 100644
> --- a/gas/config/tc-aarch64.h
> +++ b/gas/config/tc-aarch64.h
> @@ -267,24 +267,24 @@ extern void aarch64_after_parse_args (void);
> extern bool aarch64_support_sframe_p (void);
> #define support_sframe_p aarch64_support_sframe_p
>
> -/* The stack-pointer register number for SFrame stack trace info. */
> +/* The stack pointer DWARF register number for SFrame CFA tracking. */
> extern unsigned int aarch64_sframe_cfa_sp_reg;
> #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
>
> -/* The base-pointer register number for CFA stack trace info. */
> +/* The frame pointer DWARF register number for SFrame CFA and FP tracking. */
> extern unsigned int aarch64_sframe_cfa_fp_reg;
> #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
>
> -/* The return address register number for CFA stack trace info. */
> +/* The return address DWARF register number for SFrame RA tracking. */
> extern unsigned int aarch64_sframe_cfa_ra_reg;
> #define SFRAME_CFA_RA_REG aarch64_sframe_cfa_ra_reg
>
> -/* Specify if RA tracking is needed. */
> +/* Whether SFrame return address tracking is needed. */
> extern bool aarch64_sframe_ra_tracking_p (void);
> #define sframe_ra_tracking_p aarch64_sframe_ra_tracking_p
>
> -/* Specify the fixed offset to recover RA from CFA.
> - (useful only when RA tracking is not needed). */
> +/* The fixed offset from CFA for SFrame to recover the return address.
> + (useful only when SFrame RA tracking is not needed). */
> extern offsetT aarch64_sframe_cfa_ra_offset (void);
> #define sframe_cfa_ra_offset aarch64_sframe_cfa_ra_offset
>
> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> index 343848ad052d..f2c354108ea5 100644
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -11440,6 +11440,7 @@ x86_cleanup (void)
> subseg_set (seg, subseg);
> }
>
> +/* Whether SFrame stack trace info is supported. */
> bool
> x86_support_sframe_p (void)
> {
> @@ -11447,6 +11448,7 @@ x86_support_sframe_p (void)
> return (x86_elf_abi == X86_64_ABI);
> }
>
> +/* Whether SFrame return address tracking is needed. */
> bool
> x86_sframe_ra_tracking_p (void)
> {
> @@ -11456,6 +11458,8 @@ x86_sframe_ra_tracking_p (void)
> return false;
> }
>
> +/* The fixed offset from CFA for SFrame to recover the return address.
> + (useful only when SFrame RA tracking is not needed). */
> offsetT
> x86_sframe_cfa_ra_offset (void)
> {
> @@ -11463,6 +11467,7 @@ x86_sframe_cfa_ra_offset (void)
> return (offsetT) -8;
> }
>
> +/* The abi/arch indentifier for SFrame. */
> unsigned char
> x86_sframe_get_abi_arch (void)
> {
> diff --git a/gas/config/tc-i386.h b/gas/config/tc-i386.h
> index 7aae7a33dc14..8b0fc6398aac 100644
> --- a/gas/config/tc-i386.h
> +++ b/gas/config/tc-i386.h
> @@ -441,20 +441,20 @@ extern bool x86_scfi_callee_saved_p (uint32_t dw2reg_num);
> extern bool x86_support_sframe_p (void);
> #define support_sframe_p x86_support_sframe_p
>
> -/* The stack-pointer register number for SFrame stack trace info. */
> +/* The stack pointer DWARF register number for SFrame CFA tracking. */
> extern unsigned int x86_sframe_cfa_sp_reg;
> #define SFRAME_CFA_SP_REG x86_sframe_cfa_sp_reg
>
> -/* The frame-pointer register number for SFrame stack trace info. */
> +/* The frame pointer DWARF register number for SFrame CFA and FP tracking. */
> extern unsigned int x86_sframe_cfa_fp_reg;
> #define SFRAME_CFA_FP_REG x86_sframe_cfa_fp_reg
>
> -/* Specify if RA tracking is needed. */
> +/* Whether SFrame return address tracking is needed. */
> extern bool x86_sframe_ra_tracking_p (void);
> #define sframe_ra_tracking_p x86_sframe_ra_tracking_p
>
> -/* Specify the fixed offset to recover RA from CFA.
> - (useful only when RA tracking is not needed). */
> +/* The fixed offset from CFA for SFrame to recover the return address.
> + (useful only when SFrame RA tracking is not needed). */
> extern offsetT x86_sframe_cfa_ra_offset (void);
> #define sframe_cfa_ra_offset x86_sframe_cfa_ra_offset
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v4 03/15] readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
2024-06-24 14:23 ` [PATCH v4 01/15] x86: Remove unused SFrame CFI RA register variable Jens Remus
2024-06-24 14:23 ` [PATCH v4 02/15] gas: Enhance arch-specific SFrame configuration descriptions Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-24 14:23 ` [PATCH v4 04/15] readelf/objdump: Display SFrame fixed RA offset as 'f' in dump Jens Remus
` (13 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
The SFrame format allows architectures to specify fixed offsets from the
CFA, if any, from which the frame pointer (FP) and/or return address
(RA) may be recovered. These offsets are stored in the SFrame header.
For instance the SFrame generation in the assembler for x86 AMD64
specifies a fixed offset from the CFA, from which the return address
(RA) may be recovered.
When dumping the SFrame header, for instance in readelf/objdump with
option --sframe, do also dump the specified fixed offsets from the CFA,
if any, from which the frame pointer (FP) and return address (RA) may
be recovered.
Update the common SFrame test case verification patterns to allow for
the optional dumping of the CFA fixed FP/RA offsets. Update the x86-
specific SFrame and SCFI test case verification patterns to require a
CFA fixed RA offset of -8.
libsframe/
* sframe-dump.c: Dump CFA fixed FP and RA offsets.
gas/testsuite/
* gas/cfi-sframe/cfi-sframe-common-1.d: Test for optional fixed
FP and RA offsets.
* gas/cfi-sframe/cfi-sframe-common-2.d: Likewise.
* gas/cfi-sframe/cfi-sframe-common-3.d: Likewise.
* gas/cfi-sframe/cfi-sframe-common-4.d: Likewise.
* gas/cfi-sframe/cfi-sframe-common-5.d: Likewise.
* gas/cfi-sframe/cfi-sframe-common-6.d: Likewise.
* gas/cfi-sframe/cfi-sframe-common-7.d: Likewise.
* gas/cfi-sframe/cfi-sframe-common-8.d: Likewise.
* gas/cfi-sframe/cfi-sframe-x86_64-1.d: Test for fixed
RA offset.
* gas/cfi-sframe/common-empty-1.d: Test for optional fixed
FP and RA offsets.
* gas/cfi-sframe/common-empty-2.d: Likewise.
* gas/cfi-sframe/common-empty-3.d: Likewise.
* gas/scfi/x86_64/scfi-cfi-sections-1.d: Test for SFrame fixed
RA offset.
* gas/scfi/x86_64/scfi-dyn-stack-1.d: Likewise.
ld/testsuite/
* ld-x86-64/sframe-plt-1.d: Test for SFrame fixed RA offset.
* ld-x86-64/sframe-simple-1.d: Likewise.
Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v2 -> v3:
- Updated gas synthesized CFI test cases for x86 AMD64 to test for
architecture-specific fixed RA offset instead of using a pattern.
- Updated ld SFrame test cases for x86 AMD64 to test for architecture-
specific fixed RA offset.
- Corrected formatting of ChangeLog in commit message.
gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d | 2 ++
gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d | 2 ++
gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d | 2 ++
gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d | 2 ++
gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d | 2 ++
gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d | 2 ++
gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d | 2 ++
gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d | 2 ++
gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d | 1 +
gas/testsuite/gas/cfi-sframe/common-empty-1.d | 2 ++
gas/testsuite/gas/cfi-sframe/common-empty-2.d | 2 ++
gas/testsuite/gas/cfi-sframe/common-empty-3.d | 2 ++
gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d | 1 +
gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d | 1 +
ld/testsuite/ld-x86-64/sframe-plt-1.d | 1 +
ld/testsuite/ld-x86-64/sframe-simple-1.d | 1 +
libsframe/sframe-dump.c | 10 ++++++++++
17 files changed, 37 insertions(+)
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d
index 32577f31860e..5f4ae00747de 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+#? CFA fixed FP offset: \-?\d+
+#? CFA fixed RA offset: \-?\d+
Num FDEs: 1
Num FREs: 1
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d
index 3e3f74dbe424..ded8c450a942 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+#? CFA fixed FP offset: \-?\d+
+#? CFA fixed RA offset: \-?\d+
Num FDEs: 1
Num FREs: 1
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d
index 6430d463a891..d23fd9790f63 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+#? CFA fixed FP offset: \-?\d+
+#? CFA fixed RA offset: \-?\d+
Num FDEs: 1
Num FREs: 1
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
index 319ff96cce2a..ca559bd0a029 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+#? CFA fixed FP offset: \-?\d+
+#? CFA fixed RA offset: \-?\d+
Num FDEs: 1
Num FREs: 3
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
index 82d34973ddde..ee82053e13db 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+#? CFA fixed FP offset: \-?\d+
+#? CFA fixed RA offset: \-?\d+
Num FDEs: 1
Num FREs: 3
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
index fe6917c70800..9d54b98552bf 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+#? CFA fixed FP offset: \-?\d+
+#? CFA fixed RA offset: \-?\d+
Num FDEs: 1
Num FREs: 3
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
index 39724d9cdf19..2b7fe3aec8f4 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+#? CFA fixed FP offset: \-?\d+
+#? CFA fixed RA offset: \-?\d+
Num FDEs: 1
Num FREs: 3
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
index c0a0e627fad9..d654e1d0bcd4 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+#? CFA fixed FP offset: \-?\d+
+#? CFA fixed RA offset: \-?\d+
Num FDEs: 1
Num FREs: 2
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d
index ae36c21b3b7c..c8b5e6adfea0 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d
@@ -8,6 +8,7 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+ CFA fixed RA offset: \-8
Num FDEs: 1
Num FREs: 4
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
index b133b15b051d..125612ff841f 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-1.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
@@ -9,6 +9,8 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+#? CFA fixed FP offset: \-?\d+
+#? CFA fixed RA offset: \-?\d+
Num FDEs: 0
Num FREs: 0
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-2.d b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
index c5bc8594f1b7..59328fc1033f 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-2.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
@@ -9,6 +9,8 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+#? CFA fixed FP offset: \-?\d+
+#? CFA fixed RA offset: \-?\d+
Num FDEs: 0
Num FREs: 0
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
index df0b19ee1bd1..5914c620760d 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+#? CFA fixed FP offset: \-?\d+
+#? CFA fixed RA offset: \-?\d+
Num FDEs: 0
Num FREs: 0
diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d b/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d
index 5962980256c1..c45933b72edc 100644
--- a/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d
+++ b/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d
@@ -8,6 +8,7 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+ CFA fixed RA offset: \-8
Num FDEs: 1
Num FREs: 5
diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d b/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d
index b51546af1494..6cd0484d5793 100644
--- a/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d
+++ b/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d
@@ -9,6 +9,7 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: NONE
+ CFA fixed RA offset: \-8
Num FDEs: 1
Num FREs: 4
diff --git a/ld/testsuite/ld-x86-64/sframe-plt-1.d b/ld/testsuite/ld-x86-64/sframe-plt-1.d
index ca82cf20ca91..6aea5f424690 100644
--- a/ld/testsuite/ld-x86-64/sframe-plt-1.d
+++ b/ld/testsuite/ld-x86-64/sframe-plt-1.d
@@ -12,6 +12,7 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: SFRAME_F_FDE_SORTED
+ CFA fixed RA offset: \-8
#...
Function Index :
diff --git a/ld/testsuite/ld-x86-64/sframe-simple-1.d b/ld/testsuite/ld-x86-64/sframe-simple-1.d
index adc4b7419307..5ffd0ca78187 100644
--- a/ld/testsuite/ld-x86-64/sframe-simple-1.d
+++ b/ld/testsuite/ld-x86-64/sframe-simple-1.d
@@ -12,6 +12,7 @@ Contents of the SFrame section .sframe:
Version: SFRAME_VERSION_2
Flags: SFRAME_F_FDE_SORTED
+ CFA fixed RA offset: \-8
#...
Function Index :
diff --git a/libsframe/sframe-dump.c b/libsframe/sframe-dump.c
index 42a086a5691f..493d052ce91f 100644
--- a/libsframe/sframe-dump.c
+++ b/libsframe/sframe-dump.c
@@ -47,6 +47,8 @@ dump_sframe_header (sframe_decoder_ctx *sfd_ctx)
uint8_t flags;
char *flags_str;
const char *ver_str = NULL;
+ int8_t cfa_fixed_fp_offset;
+ int8_t cfa_fixed_ra_offset;
const sframe_header *header = &(sfd_ctx->sfd_header);
/* Prepare SFrame section version string. */
@@ -82,12 +84,20 @@ dump_sframe_header (sframe_decoder_ctx *sfd_ctx)
else
strcpy (flags_str, "NONE");
+ /* CFA fixed FP and RA offsets. */
+ cfa_fixed_fp_offset = header->sfh_cfa_fixed_fp_offset;
+ cfa_fixed_ra_offset = header->sfh_cfa_fixed_ra_offset;
+
const char* subsec_name = "Header";
printf ("\n");
printf (" %s :\n", subsec_name);
printf ("\n");
printf (" Version: %s\n", ver_str);
printf (" Flags: %s\n", flags_str);
+ if (cfa_fixed_fp_offset != SFRAME_CFA_FIXED_FP_INVALID)
+ printf (" CFA fixed FP offset: %d\n", cfa_fixed_fp_offset);
+ if (cfa_fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID)
+ printf (" CFA fixed RA offset: %d\n", cfa_fixed_ra_offset);
printf (" Num FDEs: %d\n", sframe_decoder_get_num_fidx (sfd_ctx));
printf (" Num FREs: %d\n", header->sfh_num_fres);
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v4 04/15] readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (2 preceding siblings ...)
2024-06-24 14:23 ` [PATCH v4 03/15] readelf/objdump: Dump SFrame CFA fixed FP and RA offsets Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-24 14:23 ` [PATCH v4 05/15] gas: Print DWARF call frame insn name in SFrame warning message Jens Remus
` (12 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
For the SFrame FRE frame-pointer (FP) offset from CFA a 'u' is displayed
if it is unavailable.
For the SFrame FRE return-address (RA) offset from CFA a 'u' was
displayed if the ABI uses a fixed RA offset from CFA. By chance a
'u' was also displayed if the RA offset is unavailable, as the string
buffer was not initialized after formatting the FP offset. Note that it
could not occur that the FP offset was erroneously displayed as RA
offset, as the SFrame format cannot have a FRE with FP offset without
RA offset.
For the FRE RA offset display 'f' if the ABI uses a fixed RA offset
from CFA. Display a 'u' if it is unavailable.
libsframe/
* sframe-dump.c: Display SFrame fixed RA offset as 'f' in dump.
gas/testsuite/
* gas/cfi-sframe/cfi-sframe-common-4.d: Test for RA displayed
either as 'u' (if RA tracking) or as 'f' (fixed RA offset if no
RA tracking).
* gas/cfi-sframe/cfi-sframe-common-5.d: Likewise.
* gas/cfi-sframe/cfi-sframe-common-6.d: Likewise.
* gas/cfi-sframe/cfi-sframe-common-7.d: Likewise.
* gas/cfi-sframe/cfi-sframe-common-8.d: Likewise.
* gas/cfi-sframe/cfi-sframe-x86_64-1.d: Test for RA displayed
as 'f' (fixed RA offset), as x86-64 does not use RA tracking.
* gas/scfi/x86_64/scfi-cfi-sections-1.d: Likewise.
* gas/scfi/x86_64/scfi-dyn-stack-1.d: Likewise.
ld/testsuite/
* ld-x86-64/sframe-plt-1.d: Test for RA displayed as 'f' (fixed
RA offset), as x86-64 does not use RA tracking.
* ld-x86-64/sframe-simple-1.d: Likewise.
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v2 -> v3:
- New patch. If the change in display of fixed RA offset as 'f' is
undesired the patch can be reduced to fixing the logic to display the
FP offset.
.../gas/cfi-sframe/cfi-sframe-common-4.d | 4 ++--
.../gas/cfi-sframe/cfi-sframe-common-5.d | 4 ++--
.../gas/cfi-sframe/cfi-sframe-common-6.d | 4 ++--
.../gas/cfi-sframe/cfi-sframe-common-7.d | 4 ++--
.../gas/cfi-sframe/cfi-sframe-common-8.d | 2 +-
.../gas/cfi-sframe/cfi-sframe-x86_64-1.d | 8 ++++----
.../gas/scfi/x86_64/scfi-cfi-sections-1.d | 10 +++++-----
gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d | 10 +++++-----
ld/testsuite/ld-x86-64/sframe-plt-1.d | 8 ++++----
ld/testsuite/ld-x86-64/sframe-simple-1.d | 16 ++++++++--------
libsframe/sframe-dump.c | 8 +++++---
11 files changed, 40 insertions(+), 38 deletions(-)
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
index ca559bd0a029..8632613f532f 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
@@ -17,7 +17,7 @@ Contents of the SFrame section .sframe:
func idx \[0\]: pc = 0x0, size = 12 bytes
STARTPC + CFA + FP + RA +
#...
- 0+0004 +sp\+16 +u +u +
- 0+0008 +sp\+32 +u +u +
+ 0+0004 +sp\+16 +u +[uf] +
+ 0+0008 +sp\+32 +u +[uf] +
#pass
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
index ee82053e13db..dd2c32d3d9fc 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
@@ -17,7 +17,7 @@ Contents of the SFrame section .sframe:
func idx \[0\]: pc = 0x0, size = 12 bytes
STARTPC + CFA + FP + RA +
#...
- 0+0004 +sp\+16 +u +u +
- 0+0008 +sp\+24 +u +u +
+ 0+0004 +sp\+16 +u +[uf] +
+ 0+0008 +sp\+24 +u +[uf] +
#pass
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
index 9d54b98552bf..34390c46a074 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
@@ -17,7 +17,7 @@ Contents of the SFrame section .sframe:
func idx \[0\]: pc = 0x0, size = 12 bytes
STARTPC + CFA + FP + RA +
#...
- 0+0004 +sp\+8 +u +u +
- 0+0008 +sp\+8 +u +u +
+ 0+0004 +sp\+8 +u +[uf] +
+ 0+0008 +sp\+8 +u +[uf] +
#pass
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
index 2b7fe3aec8f4..61efb9c4ed12 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
@@ -17,7 +17,7 @@ Contents of the SFrame section .sframe:
func idx \[0\]: pc = 0x0, size = 12 bytes
STARTPC + CFA + FP + RA +
#...
- 0+0004 +sp\+8 +u +u +
- 0+0008 +sp\+8 +u +u +
+ 0+0004 +sp\+8 +u +[uf] +
+ 0+0008 +sp\+8 +u +[uf] +
#pass
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
index d654e1d0bcd4..d77645636b36 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
@@ -17,6 +17,6 @@ Contents of the SFrame section .sframe:
func idx \[0\]: pc = 0x0, size = 8 bytes
STARTPC + CFA + FP + RA +
#...
- 0+0004 +sp\+16 +u +u +
+ 0+0004 +sp\+16 +u +[uf] +
#pass
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d
index c8b5e6adfea0..88b4cc63dbaa 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d
@@ -16,8 +16,8 @@ Contents of the SFrame section .sframe:
func idx \[0\]: pc = 0x0, size = 25 bytes
STARTPC +CFA +FP +RA +
- 0+0000 +sp\+8 +u +u +
- 0+0001 +sp\+16 +c\-16 +u +
- 0+0004 +fp\+16 +c\-16 +u +
- 0+0018 +sp\+8 +c\-16 +u +
+ 0+0000 +sp\+8 +u +f +
+ 0+0001 +sp\+16 +c\-16 +f +
+ 0+0004 +fp\+16 +c\-16 +f +
+ 0+0018 +sp\+8 +c\-16 +f +
#pass
diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d b/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d
index c45933b72edc..7c247e33a6e8 100644
--- a/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d
+++ b/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d
@@ -16,10 +16,10 @@ Contents of the SFrame section .sframe:
func idx \[0\]: pc = 0x0, size = 12 bytes
STARTPC + CFA + FP + RA +
- 0+0000 +sp\+8 +u +u +
- 0+0001 +sp\+16 +c\-16 +u +
- 0+0004 +fp\+16 +c-16 +u +
- 0+000a +sp\+16 +c\-16 +u +
- 0+000b +sp\+8 +u +u +
+ 0+0000 +sp\+8 +u +f +
+ 0+0001 +sp\+16 +c\-16 +f +
+ 0+0004 +fp\+16 +c-16 +f +
+ 0+000a +sp\+16 +c\-16 +f +
+ 0+000b +sp\+8 +u +f +
#pass
diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d b/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d
index 6cd0484d5793..c6a9b53f4e09 100644
--- a/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d
+++ b/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d
@@ -16,10 +16,10 @@ Contents of the SFrame section .sframe:
Function Index :
func idx \[0\]: pc = 0x0, size = 87 bytes
- STARTPC + CFA + FP + RA
- 0+0000 + sp\+8 + u + u
- 0+0001 + sp\+16 + c-16 + u
- 0+0004 + fp\+16 + c-16 + u
- 0+0056 + sp\+8 + u + u
+ STARTPC + CFA + FP + RA +
+ 0+0000 + sp\+8 + u + f +
+ 0+0001 + sp\+16 + c-16 + f +
+ 0+0004 + fp\+16 + c-16 + f +
+ 0+0056 + sp\+8 + u + f +
#pass
diff --git a/ld/testsuite/ld-x86-64/sframe-plt-1.d b/ld/testsuite/ld-x86-64/sframe-plt-1.d
index 6aea5f424690..52bca18d4c08 100644
--- a/ld/testsuite/ld-x86-64/sframe-plt-1.d
+++ b/ld/testsuite/ld-x86-64/sframe-plt-1.d
@@ -19,12 +19,12 @@ Contents of the SFrame section .sframe:
func idx \[0\]: pc = 0x1000, size = 16 bytes
STARTPC +CFA +FP +RA +
- 0+1000 +sp\+16 +u +u +
- 0+1006 +sp\+24 +u +u +
+ 0+1000 +sp\+16 +u +f +
+ 0+1006 +sp\+24 +u +f +
func idx \[1\]: pc = 0x1010, size = 16 bytes
STARTPC\[m\] +CFA +FP +RA +
- 0+0000 +sp\+8 +u +u +
- 0+000b +sp\+16 +u +u +
+ 0+0000 +sp\+8 +u +f +
+ 0+000b +sp\+16 +u +f +
#...
diff --git a/ld/testsuite/ld-x86-64/sframe-simple-1.d b/ld/testsuite/ld-x86-64/sframe-simple-1.d
index 5ffd0ca78187..7d88419226f1 100644
--- a/ld/testsuite/ld-x86-64/sframe-simple-1.d
+++ b/ld/testsuite/ld-x86-64/sframe-simple-1.d
@@ -23,14 +23,14 @@ Contents of the SFrame section .sframe:
func idx \[2\]: pc = 0x1020, size = 53 bytes
STARTPC +CFA +FP +RA +
- 0+1020 +sp\+8 +u +u +
- 0+1021 +sp\+16 +c-16 +u +
- 0+1024 +fp\+16 +c-16 +u +
- 0+1054 +sp\+8 +c-16 +u +
+ 0+1020 +sp\+8 +u +f +
+ 0+1021 +sp\+16 +c-16 +f +
+ 0+1024 +fp\+16 +c-16 +f +
+ 0+1054 +sp\+8 +c-16 +f +
func idx \[3\]: pc = 0x1055, size = 37 bytes
STARTPC +CFA +FP +RA +
- 0+1055 +sp\+8 +u +u +
- 0+1056 +sp\+16 +c-16 +u +
- 0+1059 +fp\+16 +c-16 +u +
- 0+1079 +sp\+8 +c-16 +u +
+ 0+1055 +sp\+8 +u +f +
+ 0+1056 +sp\+16 +c-16 +f +
+ 0+1059 +fp\+16 +c-16 +f +
+ 0+1079 +sp\+8 +c-16 +f +
diff --git a/libsframe/sframe-dump.c b/libsframe/sframe-dump.c
index 493d052ce91f..69633d53a33a 100644
--- a/libsframe/sframe-dump.c
+++ b/libsframe/sframe-dump.c
@@ -181,13 +181,15 @@ dump_sframe_func_with_fres (sframe_decoder_ctx *sfd_ctx,
printf ("%-10s", temp);
/* Dump RA info.
- If an ABI does not track RA offset, e.g., AMD64, display a 'u',
+ If an ABI does not track RA offset, e.g., AMD64, display 'f',
else display the offset d as 'c+-d'. */
- if (sframe_decoder_get_fixed_ra_offset(sfd_ctx)
+ if (sframe_decoder_get_fixed_ra_offset (sfd_ctx)
!= SFRAME_CFA_FIXED_RA_INVALID)
- strcpy (temp, "u");
+ strcpy (temp, "f");
else if (err[2] == 0)
sprintf (temp, "c%+d", ra_offset);
+ else
+ strcpy (temp, "u");
/* Mark SFrame FRE's RA information with "[s]" if the RA is mangled
with signature bits. */
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v4 05/15] gas: Print DWARF call frame insn name in SFrame warning message
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (3 preceding siblings ...)
2024-06-24 14:23 ` [PATCH v4 04/15] readelf/objdump: Display SFrame fixed RA offset as 'f' in dump Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-24 14:23 ` [PATCH v4 06/15] gas: Skip SFrame FDE if CFI specifies non-FP/SP base register Jens Remus
` (11 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
SFrame generation prints the DWARF call frame instruction opcode in
hexadecimal. Leverage get_DW_CFA_name to additionally print the
DWARF call frame instruction name in human readable form, while also
respecting fake CFI types. Use "(unknown)", if the DWARF call frame
instruction name is not known.
While at it use the terminology "instruction" for these DW_CFA_*, as
suggested by Indu.
This changes the following assembler SFrame generation warning message
as follows:
Old:
Warning: skipping SFrame FDE due to DWARF CFI op 0x<hexval>
New:
Warning: skipping SFrame FDE; CFI insn <name> (0x<hexval>)
gas/
* gen-sframe.c (sframe_get_cfi_name): New function to get the
DWARF call frame instruction name for a DWARF call frame
instruction opcode.
(sframe_do_cfi_insn): Use sframe_get_cfi_name to print the
DWARF call frame instruction name for the DWARF call frame
instruction opcode in the warning message.
gas/testsuite/
* gas/cfi-sframe/common-empty-1.d: Update expected SFrame
warning message text for DWARF call frame insn name.
* gas/cfi-sframe/common-empty-2.d: Likewise.
Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v3 -> v4:
- Change warning message text to "due to CFI insn", as suggested by
Indu, to use proper DWARF terminology "CFI instruction" and shorten
message text.
Changes v2 -> v3:
- Removed stale ChangeLog entries from commit message.
- Corrected formatting of ChangeLog in commit message.
gas/gen-sframe.c | 49 ++++++++++++++++++-
gas/testsuite/gas/cfi-sframe/common-empty-1.d | 2 +-
gas/testsuite/gas/cfi-sframe/common-empty-2.d | 2 +-
3 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 238b76f88ed0..116d1f48f781 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1199,6 +1199,46 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
return SFRAME_XLATE_OK;
}
+/* Returns the DWARF call frame instruction name or fake CFI name for the
+ specified CFI opcode, or NULL if the value is not recognized. */
+
+static const char *
+sframe_get_cfi_name (int cfi_opc)
+{
+ const char *cfi_name;
+
+ switch (cfi_opc)
+ {
+ /* Fake CFI type; outside the byte range of any real CFI insn. */
+ /* See gas/dw2gencfi.h. */
+ case CFI_adjust_cfa_offset:
+ cfi_name = "CFI_adjust_cfa_offset";
+ break;
+ case CFI_return_column:
+ cfi_name = "CFI_return_column";
+ break;
+ case CFI_rel_offset:
+ cfi_name = "CFI_rel_offset";
+ break;
+ case CFI_escape:
+ cfi_name = "CFI_escape";
+ break;
+ case CFI_signal_frame:
+ cfi_name = "CFI_signal_frame";
+ break;
+ case CFI_val_encoded_addr:
+ cfi_name = "CFI_val_encoded_addr";
+ break;
+ case CFI_label:
+ cfi_name = "CFI_label";
+ break;
+ default:
+ cfi_name = get_DW_CFA_name (cfi_opc);
+ }
+
+ return cfi_name;
+}
+
/* Process CFI_INSN and update the translation context with the FRE
information.
@@ -1274,7 +1314,14 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
/* An error here will cause no SFrame FDE later. Warn the user because this
will affect the overall coverage and hence, asynchronicity. */
if (err)
- as_warn (_("skipping SFrame FDE due to DWARF CFI op %#x"), op);
+ {
+ const char *cfi_name = sframe_get_cfi_name (op);
+
+ if (!cfi_name)
+ cfi_name = _("(unknown)");
+ as_warn (_("skipping SFrame FDE; CFI insn %s (%#x)"),
+ cfi_name, op);
+ }
return err;
}
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
index 125612ff841f..dbb48949c713 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-1.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
@@ -1,5 +1,5 @@
#as: --gsframe
-#warning: skipping SFrame FDE due to DWARF CFI op 0xa
+#warning: skipping SFrame FDE; CFI insn DW_CFA_remember_state \(0xa\)
#objdump: --sframe=.sframe
#name: Uninteresting cfi directives generate an empty SFrame section
#...
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-2.d b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
index 59328fc1033f..7b2ce3e8c796 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-2.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
@@ -1,5 +1,5 @@
#as: --gsframe
-#warning: skipping SFrame FDE due to DWARF CFI op 0xe
+#warning: skipping SFrame FDE; CFI insn DW_CFA_def_cfa_offset \(0xe\)
#objdump: --sframe=.sframe
#name: SFrame supports only FP/SP based CFA
#...
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v4 06/15] gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (4 preceding siblings ...)
2024-06-24 14:23 ` [PATCH v4 05/15] gas: Print DWARF call frame insn name in SFrame warning message Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-24 14:23 ` [PATCH v4 07/15] gas: Warn if SFrame FDE is skipped due to non-default return column Jens Remus
` (10 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
Do not generate SFrame FDE if DWARF CFI directives .cfi_def_cfa or
.cfi_def_cfa_register specify a CFA base register number other than
the architecture-specific stack-pointer (SP) or frame-pointer (FP)
register numbers.
This also causes the assembler to print a warning message, so that
skipping of the SFrame FDE does not occur silently.
Update the generic ld SFrame test case to be architecture independent.
Do not use CFI directive .cfi_def_cfa, as the specified CFA base
register number is not a valid SP/FP register number on all
architectures. An invalid SP/FP register number will now cause the
assembler to print a warning message and skip SFrame FDE generation.
Remove the offending CFI directive, that cannot be coded architecture-
independent, as the test case requires SFrame information to be
generated. This was reported by the Linaro-TCWG-CI for AArch64.
gas/
* gen-sframe.c: Skip SFrame generation if CFI specifies
non-FP/SP base register.
ld/testsuite/
* ld-sframe/discard.s: Update generic SFrame test case to be
architecture independent.
Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v2 -> v3:
- Corrected formatting of ChangeLog in commit message.
Changes v1 -> v2:
- Update generic SFrame test case to be architecture independent to
resolve generic ld SFrame test case failure reported by
Linaro-TCWG-CI for AArch64. It would fail similar on s390x.
Without this patch the assembler would erroneously generate bad SFrame
information for the s390-specific SFrame error test cases 1 and 2, that
get introduced by patch "s390: Initial support to generate .sframe from
CFI directives in assembler".
gas/gen-sframe.c | 13 +++++++++++--
ld/testsuite/ld-sframe/discard.s | 1 -
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 116d1f48f781..52c2f3f4ff89 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -988,7 +988,11 @@ sframe_xlate_do_def_cfa (struct sframe_xlate_ctx *xlate_ctx,
get_dw_fde_start_addrS (xlate_ctx->dw_fde));
}
/* Define the current CFA rule to use the provided register and
- offset. */
+ offset. However, if the register is not FP/SP, skip creating
+ SFrame stack trace info for the function. */
+ if (cfi_insn->u.r != SFRAME_CFA_SP_REG
+ && cfi_insn->u.r != SFRAME_CFA_FP_REG)
+ return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
sframe_fre_set_cfa_offset (cur_fre, cfi_insn->u.ri.offset);
cur_fre->merge_candidate = false;
@@ -1006,9 +1010,14 @@ sframe_xlate_do_def_cfa_register (struct sframe_xlate_ctx *xlate_ctx,
struct sframe_row_entry *last_fre = xlate_ctx->last_fre;
/* Get the scratchpad FRE. This FRE will eventually get linked in. */
struct sframe_row_entry *cur_fre = xlate_ctx->cur_fre;
+
gas_assert (cur_fre);
/* Define the current CFA rule to use the provided register (but to
- keep the old offset). */
+ keep the old offset). However, if the register is not FP/SP,
+ skip creating SFrame stack trace info for the function. */
+ if (cfi_insn->u.r != SFRAME_CFA_SP_REG
+ && cfi_insn->u.r != SFRAME_CFA_FP_REG)
+ return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
sframe_fre_set_cfa_offset (cur_fre, last_fre->cfa_offset);
cur_fre->merge_candidate = false;
diff --git a/ld/testsuite/ld-sframe/discard.s b/ld/testsuite/ld-sframe/discard.s
index a438b42bffa1..5591a50d486a 100644
--- a/ld/testsuite/ld-sframe/discard.s
+++ b/ld/testsuite/ld-sframe/discard.s
@@ -5,7 +5,6 @@
foo:
.cfi_startproc
.cfi_def_cfa_offset 16
- .cfi_def_cfa 7, 8
.cfi_endproc
.globl _start
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v4 07/15] gas: Warn if SFrame FDE is skipped due to non-default return column
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (5 preceding siblings ...)
2024-06-24 14:23 ` [PATCH v4 06/15] gas: Skip SFrame FDE if CFI specifies non-FP/SP base register Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-24 14:23 ` [PATCH v4 08/15] gas: Refactor SFrame CFI opcode DW_CFA_register processing Jens Remus
` (9 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
Print a warning message if SFrame FDE is skipped due to a non-default
DWARF return column (i.e. return address (RA) register number). This
may be caused by the use of CFI directive .cfi_return_column with a
non-default return address (RA) register number in the processed
assembler source code.
Warning: skipping SFrame FDE due to non-default DWARF return column
gas/
* gen-sframe.c: Warn if SFrame FDE is skipped due to non-default
DWARF return column.
gas/testsuite/
* gas/cfi-sframe/common-empty-3.d: Update test case to expect
for new warning message when SFrame FDE is skipped due to
a non-default DWARF return column.
Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v2 -> v3:
- Enhance comment in code as suggested by Indu.
- Corrected formatting of ChangeLog in commit message.
Without this patch the assembler would generate incomplete SFrame
information without warning for the s390-specific SFrame error test
case 4, that gets introduced by patch "s390: Initial support to
generate .sframe from CFI directives in assembler".
gas/gen-sframe.c | 10 +++++++---
gas/testsuite/gas/cfi-sframe/common-empty-3.d | 1 +
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 52c2f3f4ff89..fac5b6f0fa62 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1345,9 +1345,12 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
xlate_ctx->dw_fde = dw_fde;
- /* If the return column is not RIP, SFrame format cannot represent it. */
+ /* SFrame format cannot represent a non-default DWARF return column reg. */
if (xlate_ctx->dw_fde->return_column != DWARF2_DEFAULT_RETURN_COLUMN)
- return SFRAME_XLATE_ERR_NOTREPRESENTED;
+ {
+ as_warn (_("skipping SFrame FDE due to non-default DWARF return column"));
+ return SFRAME_XLATE_ERR_NOTREPRESENTED;
+ }
/* Iterate over the CFIs and create SFrame FREs. */
for (cfi_insn = dw_fde->data; cfi_insn; cfi_insn = cfi_insn->next)
@@ -1357,7 +1360,8 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
if (err != SFRAME_XLATE_OK)
{
/* Skip generating SFrame stack trace info for the function if any
- offending CFI is encountered by sframe_do_cfi_insn (). */
+ offending CFI is encountered by sframe_do_cfi_insn (). Warning
+ message already printed by sframe_do_cfi_insn (). */
return err; /* Return the error code. */
}
}
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
index 5914c620760d..d17521dd88ea 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
@@ -1,4 +1,5 @@
#as: --gsframe
+#warning: skipping SFrame FDE due to non-default DWARF return column
#objdump: --sframe=.sframe
#name: SFrame supports only default return column
#...
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v4 08/15] gas: Refactor SFrame CFI opcode DW_CFA_register processing
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (6 preceding siblings ...)
2024-06-24 14:23 ` [PATCH v4 07/15] gas: Warn if SFrame FDE is skipped due to non-default return column Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-24 14:23 ` [PATCH v4 09/15] gas: User readable warnings if SFrame FDE is not generated Jens Remus
` (8 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
Refactor SFrame processing of CFI opcode DW_CFA_register into a separate
function. This harmonizes the CFI opcode processing.
While at it reword the comment on CFI opcodes that are not processed.
This is a purely mechanical change.
gas/
* gen-sframe.c (sframe_do_cfi_insn, sframe_xlate_do_register):
Refactor SFrame CFI opcode DW_CFA_register processing.
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v2 -> v3:
- New patch.
gas/gen-sframe.c | 37 ++++++++++++++++++++++++++-----------
1 file changed, 26 insertions(+), 11 deletions(-)
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index fac5b6f0fa62..4809619f4636 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1110,6 +1110,28 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
return SFRAME_XLATE_OK;
}
+/* Translate DW_CFA_register into SFrame context.
+ Return SFRAME_XLATE_OK if success. */
+
+static int
+sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
+ struct cfi_insn_data *cfi_insn)
+{
+ /* Previous value of register1 is register2. However, if the specified
+ register1 is not interesting (SP, FP, or RA reg), the current DW_CFA_register
+ instruction can be safely skipped without sacrificing the asynchronicity of
+ stack trace information. */
+ if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
+#ifdef SFRAME_FRE_RA_TRACKING
+ || (cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
+#endif
+ || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
+ return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
+
+ /* Safe to skip. */
+ return SFRAME_XLATE_OK;
+}
+
/* Translate DW_CFA_remember_state into SFrame context.
Return SFRAME_XLATE_OK if success. */
@@ -1298,19 +1320,12 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
case DW_CFA_GNU_window_save:
err = sframe_xlate_do_gnu_window_save (xlate_ctx, cfi_insn);
break;
- /* Other CFI opcodes are not processed at this time.
- These do not impact the coverage of the basic stack tracing
- information as conveyed in the SFrame format.
- - DW_CFA_register,
- - etc. */
case DW_CFA_register:
- if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
-#ifdef SFRAME_FRE_RA_TRACKING
- || cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG
-#endif
- || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
- err = SFRAME_XLATE_ERR_NOTREPRESENTED;
+ err = sframe_xlate_do_register (xlate_ctx, cfi_insn);
break;
+ /* Following CFI opcodes are not processed at this time.
+ These do not impact the coverage of the basic stack tracing
+ information as conveyed in the SFrame format. */
case DW_CFA_undefined:
case DW_CFA_same_value:
break;
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v4 09/15] gas: User readable warnings if SFrame FDE is not generated
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (7 preceding siblings ...)
2024-06-24 14:23 ` [PATCH v4 08/15] gas: Refactor SFrame CFI opcode DW_CFA_register processing Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-25 23:57 ` Indu Bhagat
2024-06-24 14:23 ` [PATCH v4 10/15] gas: Skip SFrame FDE if FP without RA on stack Jens Remus
` (7 subsequent siblings)
16 siblings, 1 reply; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
The following generic warning message, which is printed whenever the
assembler skips generation of SFrame FDE, is not very helpful for the
user:
skipping SFrame FDE; CFI insn <name> (0x<hexval>)
Whenever possible print meaningful warning messages, when the assembler
skips generation of SFrame FDE:
- skipping SFrame FDE; non-SP/FP register <regno> in .cfi_def_cfa
- skipping SFrame FDE; non-SP/FP register <regno> in
.cfi_def_cfa_register
- skipping SFrame FDE; .cfi_def_cfa_offset without CFA base register
in effect
- skipping SFrame FDE; {FP|RA} register <regno> in .cfi_val_offset
- skipping SFrame FDE; {SP|FP|RA} register <regno> in in .cfi_register
- skipping SFrame FDE; .cfi_remember_state without prior SFrame FRE
state
- skipping SFrame FDE; non-default RA register <regno>
gas/
* gen-sframe.h (SFRAME_FRE_BASE_REG_INVAL): New macro for
invalid SFrame FRE CFA base register value of -1.
* gen-sframe.c: User readable warnings if SFrame FDE is not
generated.
gas/testsuite/
* gas/cfi-sframe/common-empty-1.d: Update generic SFrame test
case to updated warning message texts.
* gas/cfi-sframe/common-empty-2.d: Likewise.
* gas/cfi-sframe/common-empty-3.d: Likewise.
Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v3 -> v4:
- Use terse warning message texts as suggested by Indu.
- Fix bad indentation reported by GCC's check_GNU_style.py.
Changes v2 -> v3:
- Updated SFrame warning message texts as suggested by Indu, except for
the .cfi_def_cfa[_register] ones. I think it would be helpful for the
user to know that the issue is caused by a non-SP/FP register. But
maybe that is because I am new to CFI and those that would actually
debug the cause for these warnings would not need this extra
information? Anyhow, Indu, if you still prefer your suggestions I
would go with yours.
The ".cfi_remember_state without SFrame FRE state" warning needs to
remain, as there would otherwise not be any warning at all. The reason
is that sframe_do_cfi_insn now expects any of its called CFI
processing functions to emit a warning, if they return with an error,
such as SFRAME_XLATE_ERR_NOTREPRESENTED. Silently skipping SFrame FDE
does not seem an acceptable approach to me.
- Do not test sframe_ra_tracking_p when determining the SFrame register
name in sframe_register_name. The RA register name does not depend on
whether RA tracking is used or not.
- Corrected formatting of ChangeLog in commit message.
gas/gen-sframe.c | 92 ++++++++++++++-----
gas/gen-sframe.h | 2 +
gas/testsuite/gas/cfi-sframe/common-empty-1.d | 2 +-
gas/testsuite/gas/cfi-sframe/common-empty-2.d | 2 +-
gas/testsuite/gas/cfi-sframe/common-empty-3.d | 2 +-
5 files changed, 72 insertions(+), 28 deletions(-)
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 4809619f4636..3d9824a7a080 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -869,7 +869,7 @@ sframe_row_entry_new (void)
struct sframe_row_entry *fre = XCNEW (struct sframe_row_entry);
/* Reset cfa_base_reg to -1. A value of 0 will imply some valid register
for the supported arches. */
- fre->cfa_base_reg = -1;
+ fre->cfa_base_reg = SFRAME_FRE_BASE_REG_INVAL;
fre->merge_candidate = true;
/* Reset the mangled RA status bit to zero by default. We will initialize it in
sframe_row_entry_initialize () with the sticky bit if set. */
@@ -924,6 +924,23 @@ sframe_row_entry_initialize (struct sframe_row_entry *cur_fre,
cur_fre->mangled_ra_p = prev_fre->mangled_ra_p;
}
+/* Return SFrame register name for SP, FP, and RA, or NULL if other. */
+
+static const char *
+sframe_register_name (unsigned int reg)
+{
+ if (reg == SFRAME_CFA_SP_REG)
+ return "SP";
+ else if (reg == SFRAME_CFA_FP_REG)
+ return "FP";
+#ifdef SFRAME_FRE_RA_TRACKING
+ else if (reg == SFRAME_CFA_RA_REG)
+ return "RA";
+#endif
+ else
+ return NULL;
+}
+
/* Translate DW_CFA_advance_loc into SFrame context.
Return SFRAME_XLATE_OK if success. */
@@ -992,7 +1009,11 @@ sframe_xlate_do_def_cfa (struct sframe_xlate_ctx *xlate_ctx,
SFrame stack trace info for the function. */
if (cfi_insn->u.r != SFRAME_CFA_SP_REG
&& cfi_insn->u.r != SFRAME_CFA_FP_REG)
- return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
+ {
+ as_warn (_("skipping SFrame FDE; non-SP/FP register %u in .cfi_def_cfa"),
+ cfi_insn->u.r);
+ return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
+ }
sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
sframe_fre_set_cfa_offset (cur_fre, cfi_insn->u.ri.offset);
cur_fre->merge_candidate = false;
@@ -1017,7 +1038,12 @@ sframe_xlate_do_def_cfa_register (struct sframe_xlate_ctx *xlate_ctx,
skip creating SFrame stack trace info for the function. */
if (cfi_insn->u.r != SFRAME_CFA_SP_REG
&& cfi_insn->u.r != SFRAME_CFA_FP_REG)
- return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
+ {
+ as_warn (_("skipping SFrame FDE; "
+ "non-SP/FP register %u in .cfi_def_cfa_register"),
+ cfi_insn->u.r);
+ return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
+ }
sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
sframe_fre_set_cfa_offset (cur_fre, last_fre->cfa_offset);
cur_fre->merge_candidate = false;
@@ -1048,7 +1074,13 @@ sframe_xlate_do_def_cfa_offset (struct sframe_xlate_ctx *xlate_ctx,
cur_fre->merge_candidate = false;
}
else
- return SFRAME_XLATE_ERR_NOTREPRESENTED;
+ {
+ /* No CFA base register in effect. Non-SP/FP CFA base register should
+ not occur, as sframe_xlate_do_def_cfa[_register] would detect this. */
+ as_warn (_("skipping SFrame FDE; "
+ ".cfi_def_cfa_offset without CFA base register in effect"));
+ return SFRAME_XLATE_ERR_NOTREPRESENTED;
+ }
return SFRAME_XLATE_OK;
}
@@ -1098,13 +1130,16 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
register is not interesting (FP or RA reg), the current DW_CFA_val_offset
instruction can be safely skipped without sacrificing the asynchronicity of
stack trace information. */
- if (cfi_insn->u.r == SFRAME_CFA_FP_REG)
- return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
+ if (cfi_insn->u.r == SFRAME_CFA_FP_REG
#ifdef SFRAME_FRE_RA_TRACKING
- else if (sframe_ra_tracking_p ()
- && cfi_insn->u.r == SFRAME_CFA_RA_REG)
- return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
+ || (sframe_ra_tracking_p () && cfi_insn->u.r == SFRAME_CFA_RA_REG)
#endif
+ )
+ {
+ as_warn (_("skipping SFrame FDE; %s register %u in .cfi_val_offset"),
+ sframe_register_name (cfi_insn->u.r), cfi_insn->u.r);
+ return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
+ }
/* Safe to skip. */
return SFRAME_XLATE_OK;
@@ -1126,7 +1161,11 @@ sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
|| (cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
#endif
|| cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
- return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
+ {
+ as_warn (_("skipping SFrame FDE; %s register %u in .cfi_register"),
+ sframe_register_name (cfi_insn->u.rr.reg1), cfi_insn->u.rr.reg1);
+ return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
+ }
/* Safe to skip. */
return SFRAME_XLATE_OK;
@@ -1144,7 +1183,11 @@ sframe_xlate_do_remember_state (struct sframe_xlate_ctx *xlate_ctx)
early with non-zero error code, this will cause no SFrame stack trace
info for the function involved. */
if (!last_fre)
- return SFRAME_XLATE_ERR_INVAL;
+ {
+ as_warn (_("skipping SFrame FDE; "
+ ".cfi_remember_state without prior SFrame FRE state"));
+ return SFRAME_XLATE_ERR_INVAL;
+ }
if (!xlate_ctx->remember_fre)
xlate_ctx->remember_fre = sframe_row_entry_new ();
@@ -1332,21 +1375,19 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
default:
/* Following skipped operations do, however, impact the asynchronicity:
- CFI_escape. */
- err = SFRAME_XLATE_ERR_NOTREPRESENTED;
- }
-
- /* An error here will cause no SFrame FDE later. Warn the user because this
- will affect the overall coverage and hence, asynchronicity. */
- if (err)
- {
- const char *cfi_name = sframe_get_cfi_name (op);
-
- if (!cfi_name)
- cfi_name = _("(unknown)");
- as_warn (_("skipping SFrame FDE; CFI insn %s (%#x)"),
- cfi_name, op);
+ {
+ const char *cfi_name = sframe_get_cfi_name (op);
+
+ if (!cfi_name)
+ cfi_name = _("(unknown)");
+ as_warn (_("skipping SFrame FDE; CFI insn %s (%#x)"),
+ cfi_name, op);
+ err = SFRAME_XLATE_ERR_NOTREPRESENTED;
+ }
}
+ /* Any error will cause no SFrame FDE later. The user has already been
+ warned. */
return err;
}
@@ -1363,7 +1404,8 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
/* SFrame format cannot represent a non-default DWARF return column reg. */
if (xlate_ctx->dw_fde->return_column != DWARF2_DEFAULT_RETURN_COLUMN)
{
- as_warn (_("skipping SFrame FDE due to non-default DWARF return column"));
+ as_warn (_("skipping SFrame FDE; non-default RA register %u"),
+ xlate_ctx->dw_fde->return_column);
return SFRAME_XLATE_ERR_NOTREPRESENTED;
}
diff --git a/gas/gen-sframe.h b/gas/gen-sframe.h
index fbe2fd5d9368..8ed46dbb087b 100644
--- a/gas/gen-sframe.h
+++ b/gas/gen-sframe.h
@@ -24,6 +24,8 @@
#define SFRAME_FRE_ELEM_LOC_REG 0
#define SFRAME_FRE_ELEM_LOC_STACK 1
+#define SFRAME_FRE_BASE_REG_INVAL ((unsigned int)-1)
+
/* SFrame Frame Row Entry (FRE).
A frame row entry is a slice of the frame and can be valid for a set of
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
index dbb48949c713..6e99dd57af94 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-1.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
@@ -1,5 +1,5 @@
#as: --gsframe
-#warning: skipping SFrame FDE; CFI insn DW_CFA_remember_state \(0xa\)
+#warning: skipping SFrame FDE; \.cfi_remember_state without prior SFrame FRE state
#objdump: --sframe=.sframe
#name: Uninteresting cfi directives generate an empty SFrame section
#...
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-2.d b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
index 7b2ce3e8c796..77c303eec8fb 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-2.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
@@ -1,5 +1,5 @@
#as: --gsframe
-#warning: skipping SFrame FDE; CFI insn DW_CFA_def_cfa_offset \(0xe\)
+#warning: skipping SFrame FDE; \.cfi_def_cfa_offset without CFA base register in effect
#objdump: --sframe=.sframe
#name: SFrame supports only FP/SP based CFA
#...
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
index d17521dd88ea..4ec5b44dd51f 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
@@ -1,5 +1,5 @@
#as: --gsframe
-#warning: skipping SFrame FDE due to non-default DWARF return column
+#warning: skipping SFrame FDE; non-default RA register 0
#objdump: --sframe=.sframe
#name: SFrame supports only default return column
#...
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v4 09/15] gas: User readable warnings if SFrame FDE is not generated
2024-06-24 14:23 ` [PATCH v4 09/15] gas: User readable warnings if SFrame FDE is not generated Jens Remus
@ 2024-06-25 23:57 ` Indu Bhagat
0 siblings, 0 replies; 32+ messages in thread
From: Indu Bhagat @ 2024-06-25 23:57 UTC (permalink / raw)
To: Jens Remus, binutils; +Cc: Andreas Krebbel
On 6/24/24 07:23, Jens Remus wrote:
> The following generic warning message, which is printed whenever the
> assembler skips generation of SFrame FDE, is not very helpful for the
> user:
>
> skipping SFrame FDE; CFI insn <name> (0x<hexval>)
>
> Whenever possible print meaningful warning messages, when the assembler
> skips generation of SFrame FDE:
>
> - skipping SFrame FDE; non-SP/FP register <regno> in .cfi_def_cfa
> - skipping SFrame FDE; non-SP/FP register <regno> in
> .cfi_def_cfa_register
> - skipping SFrame FDE; .cfi_def_cfa_offset without CFA base register
> in effect
> - skipping SFrame FDE; {FP|RA} register <regno> in .cfi_val_offset
> - skipping SFrame FDE; {SP|FP|RA} register <regno> in in .cfi_register
> - skipping SFrame FDE; .cfi_remember_state without prior SFrame FRE
> state
> - skipping SFrame FDE; non-default RA register <regno>
>
> gas/
> * gen-sframe.h (SFRAME_FRE_BASE_REG_INVAL): New macro for
> invalid SFrame FRE CFA base register value of -1.
> * gen-sframe.c: User readable warnings if SFrame FDE is not
> generated.
>
> gas/testsuite/
> * gas/cfi-sframe/common-empty-1.d: Update generic SFrame test
> case to updated warning message texts.
> * gas/cfi-sframe/common-empty-2.d: Likewise.
> * gas/cfi-sframe/common-empty-3.d: Likewise.
>
> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
LGTM.
Thanks
> ---
>
> Notes (jremus):
> Changes v3 -> v4:
> - Use terse warning message texts as suggested by Indu.
> - Fix bad indentation reported by GCC's check_GNU_style.py.
>
> Changes v2 -> v3:
> - Updated SFrame warning message texts as suggested by Indu, except for
> the .cfi_def_cfa[_register] ones. I think it would be helpful for the
> user to know that the issue is caused by a non-SP/FP register. But
> maybe that is because I am new to CFI and those that would actually
> debug the cause for these warnings would not need this extra
> information? Anyhow, Indu, if you still prefer your suggestions I
> would go with yours.
> The ".cfi_remember_state without SFrame FRE state" warning needs to
> remain, as there would otherwise not be any warning at all. The reason
> is that sframe_do_cfi_insn now expects any of its called CFI
> processing functions to emit a warning, if they return with an error,
> such as SFRAME_XLATE_ERR_NOTREPRESENTED. Silently skipping SFrame FDE
> does not seem an acceptable approach to me.
> - Do not test sframe_ra_tracking_p when determining the SFrame register
> name in sframe_register_name. The RA register name does not depend on
> whether RA tracking is used or not.
> - Corrected formatting of ChangeLog in commit message.
>
> gas/gen-sframe.c | 92 ++++++++++++++-----
> gas/gen-sframe.h | 2 +
> gas/testsuite/gas/cfi-sframe/common-empty-1.d | 2 +-
> gas/testsuite/gas/cfi-sframe/common-empty-2.d | 2 +-
> gas/testsuite/gas/cfi-sframe/common-empty-3.d | 2 +-
> 5 files changed, 72 insertions(+), 28 deletions(-)
>
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 4809619f4636..3d9824a7a080 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -869,7 +869,7 @@ sframe_row_entry_new (void)
> struct sframe_row_entry *fre = XCNEW (struct sframe_row_entry);
> /* Reset cfa_base_reg to -1. A value of 0 will imply some valid register
> for the supported arches. */
> - fre->cfa_base_reg = -1;
> + fre->cfa_base_reg = SFRAME_FRE_BASE_REG_INVAL;
> fre->merge_candidate = true;
> /* Reset the mangled RA status bit to zero by default. We will initialize it in
> sframe_row_entry_initialize () with the sticky bit if set. */
> @@ -924,6 +924,23 @@ sframe_row_entry_initialize (struct sframe_row_entry *cur_fre,
> cur_fre->mangled_ra_p = prev_fre->mangled_ra_p;
> }
>
> +/* Return SFrame register name for SP, FP, and RA, or NULL if other. */
> +
> +static const char *
> +sframe_register_name (unsigned int reg)
> +{
> + if (reg == SFRAME_CFA_SP_REG)
> + return "SP";
> + else if (reg == SFRAME_CFA_FP_REG)
> + return "FP";
> +#ifdef SFRAME_FRE_RA_TRACKING
> + else if (reg == SFRAME_CFA_RA_REG)
> + return "RA";
> +#endif
> + else
> + return NULL;
> +}
> +
> /* Translate DW_CFA_advance_loc into SFrame context.
> Return SFRAME_XLATE_OK if success. */
>
> @@ -992,7 +1009,11 @@ sframe_xlate_do_def_cfa (struct sframe_xlate_ctx *xlate_ctx,
> SFrame stack trace info for the function. */
> if (cfi_insn->u.r != SFRAME_CFA_SP_REG
> && cfi_insn->u.r != SFRAME_CFA_FP_REG)
> - return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
> + {
> + as_warn (_("skipping SFrame FDE; non-SP/FP register %u in .cfi_def_cfa"),
> + cfi_insn->u.r);
> + return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
> + }
> sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
> sframe_fre_set_cfa_offset (cur_fre, cfi_insn->u.ri.offset);
> cur_fre->merge_candidate = false;
> @@ -1017,7 +1038,12 @@ sframe_xlate_do_def_cfa_register (struct sframe_xlate_ctx *xlate_ctx,
> skip creating SFrame stack trace info for the function. */
> if (cfi_insn->u.r != SFRAME_CFA_SP_REG
> && cfi_insn->u.r != SFRAME_CFA_FP_REG)
> - return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
> + {
> + as_warn (_("skipping SFrame FDE; "
> + "non-SP/FP register %u in .cfi_def_cfa_register"),
> + cfi_insn->u.r);
> + return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
> + }
> sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
> sframe_fre_set_cfa_offset (cur_fre, last_fre->cfa_offset);
> cur_fre->merge_candidate = false;
> @@ -1048,7 +1074,13 @@ sframe_xlate_do_def_cfa_offset (struct sframe_xlate_ctx *xlate_ctx,
> cur_fre->merge_candidate = false;
> }
> else
> - return SFRAME_XLATE_ERR_NOTREPRESENTED;
> + {
> + /* No CFA base register in effect. Non-SP/FP CFA base register should
> + not occur, as sframe_xlate_do_def_cfa[_register] would detect this. */
> + as_warn (_("skipping SFrame FDE; "
> + ".cfi_def_cfa_offset without CFA base register in effect"));
> + return SFRAME_XLATE_ERR_NOTREPRESENTED;
> + }
>
> return SFRAME_XLATE_OK;
> }
> @@ -1098,13 +1130,16 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
> register is not interesting (FP or RA reg), the current DW_CFA_val_offset
> instruction can be safely skipped without sacrificing the asynchronicity of
> stack trace information. */
> - if (cfi_insn->u.r == SFRAME_CFA_FP_REG)
> - return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
> + if (cfi_insn->u.r == SFRAME_CFA_FP_REG
> #ifdef SFRAME_FRE_RA_TRACKING
> - else if (sframe_ra_tracking_p ()
> - && cfi_insn->u.r == SFRAME_CFA_RA_REG)
> - return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
> + || (sframe_ra_tracking_p () && cfi_insn->u.r == SFRAME_CFA_RA_REG)
> #endif
> + )
> + {
> + as_warn (_("skipping SFrame FDE; %s register %u in .cfi_val_offset"),
> + sframe_register_name (cfi_insn->u.r), cfi_insn->u.r);
> + return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
> + }
>
> /* Safe to skip. */
> return SFRAME_XLATE_OK;
> @@ -1126,7 +1161,11 @@ sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
> || (cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
> #endif
> || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
> - return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
> + {
> + as_warn (_("skipping SFrame FDE; %s register %u in .cfi_register"),
> + sframe_register_name (cfi_insn->u.rr.reg1), cfi_insn->u.rr.reg1);
> + return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
> + }
>
> /* Safe to skip. */
> return SFRAME_XLATE_OK;
> @@ -1144,7 +1183,11 @@ sframe_xlate_do_remember_state (struct sframe_xlate_ctx *xlate_ctx)
> early with non-zero error code, this will cause no SFrame stack trace
> info for the function involved. */
> if (!last_fre)
> - return SFRAME_XLATE_ERR_INVAL;
> + {
> + as_warn (_("skipping SFrame FDE; "
> + ".cfi_remember_state without prior SFrame FRE state"));
> + return SFRAME_XLATE_ERR_INVAL;
> + }
>
> if (!xlate_ctx->remember_fre)
> xlate_ctx->remember_fre = sframe_row_entry_new ();
> @@ -1332,21 +1375,19 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
> default:
> /* Following skipped operations do, however, impact the asynchronicity:
> - CFI_escape. */
> - err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> - }
> -
> - /* An error here will cause no SFrame FDE later. Warn the user because this
> - will affect the overall coverage and hence, asynchronicity. */
> - if (err)
> - {
> - const char *cfi_name = sframe_get_cfi_name (op);
> -
> - if (!cfi_name)
> - cfi_name = _("(unknown)");
> - as_warn (_("skipping SFrame FDE; CFI insn %s (%#x)"),
> - cfi_name, op);
> + {
> + const char *cfi_name = sframe_get_cfi_name (op);
> +
> + if (!cfi_name)
> + cfi_name = _("(unknown)");
> + as_warn (_("skipping SFrame FDE; CFI insn %s (%#x)"),
> + cfi_name, op);
> + err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> + }
> }
>
> + /* Any error will cause no SFrame FDE later. The user has already been
> + warned. */
> return err;
> }
>
> @@ -1363,7 +1404,8 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
> /* SFrame format cannot represent a non-default DWARF return column reg. */
> if (xlate_ctx->dw_fde->return_column != DWARF2_DEFAULT_RETURN_COLUMN)
> {
> - as_warn (_("skipping SFrame FDE due to non-default DWARF return column"));
> + as_warn (_("skipping SFrame FDE; non-default RA register %u"),
> + xlate_ctx->dw_fde->return_column);
> return SFRAME_XLATE_ERR_NOTREPRESENTED;
> }
>
> diff --git a/gas/gen-sframe.h b/gas/gen-sframe.h
> index fbe2fd5d9368..8ed46dbb087b 100644
> --- a/gas/gen-sframe.h
> +++ b/gas/gen-sframe.h
> @@ -24,6 +24,8 @@
> #define SFRAME_FRE_ELEM_LOC_REG 0
> #define SFRAME_FRE_ELEM_LOC_STACK 1
>
> +#define SFRAME_FRE_BASE_REG_INVAL ((unsigned int)-1)
> +
> /* SFrame Frame Row Entry (FRE).
>
> A frame row entry is a slice of the frame and can be valid for a set of
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> index dbb48949c713..6e99dd57af94 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> @@ -1,5 +1,5 @@
> #as: --gsframe
> -#warning: skipping SFrame FDE; CFI insn DW_CFA_remember_state \(0xa\)
> +#warning: skipping SFrame FDE; \.cfi_remember_state without prior SFrame FRE state
> #objdump: --sframe=.sframe
> #name: Uninteresting cfi directives generate an empty SFrame section
> #...
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-2.d b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> index 7b2ce3e8c796..77c303eec8fb 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> @@ -1,5 +1,5 @@
> #as: --gsframe
> -#warning: skipping SFrame FDE; CFI insn DW_CFA_def_cfa_offset \(0xe\)
> +#warning: skipping SFrame FDE; \.cfi_def_cfa_offset without CFA base register in effect
> #objdump: --sframe=.sframe
> #name: SFrame supports only FP/SP based CFA
> #...
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> index d17521dd88ea..4ec5b44dd51f 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> @@ -1,5 +1,5 @@
> #as: --gsframe
> -#warning: skipping SFrame FDE due to non-default DWARF return column
> +#warning: skipping SFrame FDE; non-default RA register 0
> #objdump: --sframe=.sframe
> #name: SFrame supports only default return column
> #...
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v4 10/15] gas: Skip SFrame FDE if FP without RA on stack
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (8 preceding siblings ...)
2024-06-24 14:23 ` [PATCH v4 09/15] gas: User readable warnings if SFrame FDE is not generated Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-24 14:23 ` [PATCH v4 11/15] gas: Skip SFrame FDE if .cfi_window_save Jens Remus
` (6 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
The SFrame format cannot represent the frame pointer (FP) being saved
on the stack without the return address (RA) also being saved on the
stack, if RA tracking is used.
A SFrame FDE is followed by 1-3 offsets with the following information:
Without RA tracking:
1. Offset from base pointer (SP or FP) to locate the CFA
2. Optional: Offset to CFA to restore the frame pointer (FP)
With RA tracking:
1. Offset from base pointer (SP or FP) to locate the CFA
2. Optional: Offset to CFA to restore the return address (RA)
3. Optional: Offset to CFA to restore the frame pointer (FP)
When RA tracking is used and a FDE is followed by two offsets the
SFrame format does not provide any information to distinguish whether
the second offset is the RA or FP offset. SFrame assumes the offset to
be the RA offset, which may be wrong.
Therefore skip generation of SFrame FDE information and print the
following warning, if RA tracking is used and the FP is saved on the
stack without the RA being saved as well:
skipping SFrame FDE; FP without RA on stack
gas/
* gen-sframe.c (sframe_do_fde): Skip SFrame FDE if FP without RA
on stack, as the SFrame format cannot represent this case.
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v3 -> v4:
- Dropped misleading comment "No errors encountered". as suggtested by
Indu.
- Changed warning message text as follows for consistency:
skipping SFrame FDE; FP without RA on stack
Changes v2 -> v3:
- New patch.
Without this patch the assembler would generate incorrect SFrame
information without warning for the s390-specific SFrame error test
case 5, that gets introduced by patch "s390: Initial support to
generate .sframe from CFI directives in assembler". The FRE would
be followed by two offsets for the CFA and FP. SFrame would
erroneously interpret them as CFA and RA offsets, as it cannot
represent FP without RA on stack.
gas/gen-sframe.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 3d9824a7a080..c29c407cc86f 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1423,8 +1423,6 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
}
}
- /* No errors encountered. */
-
/* Link in the scratchpad FRE that the last few CFI insns helped create. */
if (xlate_ctx->cur_fre)
{
@@ -1438,6 +1436,25 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
= get_dw_fde_end_addrS (xlate_ctx->dw_fde);
}
+#ifdef SFRAME_FRE_RA_TRACKING
+ if (sframe_ra_tracking_p ())
+ {
+ struct sframe_row_entry *fre;
+
+ /* Iterate over the scratchpad FREs and validate them. */
+ for (fre = xlate_ctx->first_fre; fre; fre = fre->next)
+ {
+ /* SFrame format cannot represent FP on stack without RA on stack. */
+ if (fre->ra_loc != SFRAME_FRE_ELEM_LOC_STACK
+ && fre->bp_loc == SFRAME_FRE_ELEM_LOC_STACK)
+ {
+ as_warn (_("skipping SFrame FDE; FP without RA on stack"));
+ return SFRAME_XLATE_ERR_NOTREPRESENTED;
+ }
+ }
+ }
+#endif /* SFRAME_FRE_RA_TRACKING */
+
return SFRAME_XLATE_OK;
}
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v4 11/15] gas: Skip SFrame FDE if .cfi_window_save
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (9 preceding siblings ...)
2024-06-24 14:23 ` [PATCH v4 10/15] gas: Skip SFrame FDE if FP without RA on stack Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-24 14:23 ` [PATCH v4 12/15] gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking Jens Remus
` (5 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
CFI opcode DW_CFA_AARCH64_negate_ra_state is multiplexed with
DW_CFA_GNU_window_save. Process DW_CFA_AARCH64_negate_ra_state on
AArch64. Skip generation of SFrame FDE otherwise with the following
warning message:
skipping SFrame FDE; .cfi_window_save
gas/
* gen-sframe.c: Skip SFrame FDE if .cfi_window_save.
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v3 -> v4:
- Change warning message text to the following for consistency:
skipping SFrame FDE; .cfi_window_save
Changes v2 -> v3:
- New patch. The intention is to skip all "unknown" CFI opcodes,
which SFrame does not explicitly handle. DW_CFA_GNU_window_save
seems to be handled only for the AArch64-specific multiplexed
DW_CFA_AARCH64_negate_ra_state. The logic could be changed to be
dependent on TC_AARCH64 at build-time instead of
sframe_get_abi_arch() at run-time.
gas/gen-sframe.c | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index c29c407cc86f..2222d768799f 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1256,12 +1256,12 @@ sframe_xlate_do_restore (struct sframe_xlate_ctx *xlate_ctx,
return SFRAME_XLATE_OK;
}
-/* Translate DW_CFA_GNU_window_save into SFrame context.
+/* Translate DW_CFA_AARCH64_negate_ra_state into SFrame context.
Return SFRAME_XLATE_OK if success. */
static int
-sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
- struct cfi_insn_data *cfi_insn ATTRIBUTE_UNUSED)
+sframe_xlate_do_aarch64_negate_ra_state (struct sframe_xlate_ctx *xlate_ctx,
+ struct cfi_insn_data *cfi_insn ATTRIBUTE_UNUSED)
{
struct sframe_row_entry *cur_fre = xlate_ctx->cur_fre;
@@ -1273,6 +1273,25 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
return SFRAME_XLATE_OK;
}
+/* Translate DW_CFA_GNU_window_save into SFrame context.
+ DW_CFA_AARCH64_negate_ra_state is multiplexed with DW_CFA_GNU_window_save.
+ Return SFRAME_XLATE_OK if success. */
+
+static int
+sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
+ struct cfi_insn_data *cfi_insn)
+{
+ unsigned char abi_arch = sframe_get_abi_arch ();
+
+ /* Translate DW_CFA_AARCH64_negate_ra_state into SFrame context. */
+ if (abi_arch == SFRAME_ABI_AARCH64_ENDIAN_BIG
+ || abi_arch == SFRAME_ABI_AARCH64_ENDIAN_LITTLE)
+ return sframe_xlate_do_aarch64_negate_ra_state (xlate_ctx, cfi_insn);
+
+ as_warn (_("skipping SFrame FDE; .cfi_window_save"));
+ return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented. */
+}
+
/* Returns the DWARF call frame instruction name or fake CFI name for the
specified CFI opcode, or NULL if the value is not recognized. */
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v4 12/15] gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (10 preceding siblings ...)
2024-06-24 14:23 ` [PATCH v4 11/15] gas: Skip SFrame FDE if .cfi_window_save Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-24 14:23 ` [PATCH v4 13/15] gas: Don't skip SFrame FDE if .cfi_register specifies SP register Jens Remus
` (4 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
Do not skip SFrame FDE if .cfi_register specifies RA register without
RA tracking being actually used. Without RA tracking the register
contents can always be restored from the stack using the fixed
RA offset from CFA.
gas/
* gen-sframe.c (sframe_xlate_do_register): Do not skip SFrame
FDE if .cfi_register specifies RA register without RA tracking
being used.
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v2 -> v3:
- New patch.
gas/gen-sframe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 2222d768799f..31f2e5118280 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1158,7 +1158,7 @@ sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
stack trace information. */
if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
#ifdef SFRAME_FRE_RA_TRACKING
- || (cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
+ || (sframe_ra_tracking_p () && cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
#endif
|| cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
{
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v4 13/15] gas: Don't skip SFrame FDE if .cfi_register specifies SP register
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (11 preceding siblings ...)
2024-06-24 14:23 ` [PATCH v4 12/15] gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-25 23:59 ` Indu Bhagat
2024-06-24 14:23 ` [PATCH v4 14/15] gas: Test predicate whether SFrame RA tracking is used Jens Remus
` (3 subsequent siblings)
16 siblings, 1 reply; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
Neither ".cfi_offset SP, <offset>", ".cfi_register SP, <regno>", nor
".cfi_val_offset SP, <offset>" alter the tracking information to recover
the stack pointer (SP). Doing so would need an explicit .cfi_def_cfa,
which SFrame tracks.
The stack-pointer (SP) register contents on entry can be reconstructed
from the SFrame CFA tracking information using information from the
current and initial SFrame FREs of the SFrame FDE:
1. Compute CFA from the current CFA base register (SP or FP) and CFA
offset from the SFrame CFA tracking information from the SFrame FRE
for the current instruction address:
CFA = <current_base_reg> + <current_cfa_offset>
2. Compute SP from the current CFA and the CFA offset from the SFrame
CFA tracking information from the initial SFrame FRE of the FDE:
SP = CFA - <initial_cfa_offset>
While at it add comments to the processing of .cfi_offset and
.cfi_val_offset that the SP can be reconstructed from the CFA tracking
information.
gas/
* gen-sframe.c (sframe_xlate_do_register): Do not skip SFrame
FDE if .cfi_register specifies SP register.
(sframe_xlate_do_offset,sframe_xlate_do_val_offset): Add comment
that this is likewise.
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v3 -> v4:
- Add comment to sframe_xlate_do_offset why SP register is ignored.
- Reword commit message accordingly.
Changes v2 -> v3:
- New patch.
gas/gen-sframe.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 31f2e5118280..f4bdbb5944d9 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1100,6 +1100,7 @@ sframe_xlate_do_offset (struct sframe_xlate_ctx *xlate_ctx,
gas_assert (cur_fre);
/* Change the rule for the register indicated by the register number to
be the specified offset. */
+ /* Ignore SP reg, as it can be recovered from the CFA tracking info. */
if (cfi_insn->u.r == SFRAME_CFA_FP_REG)
{
gas_assert (!cur_fre->base_reg);
@@ -1134,6 +1135,7 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
#ifdef SFRAME_FRE_RA_TRACKING
|| (sframe_ra_tracking_p () && cfi_insn->u.r == SFRAME_CFA_RA_REG)
#endif
+ /* Ignore SP reg, as it can be recovered from the CFA tracking info. */
)
{
as_warn (_("skipping SFrame FDE; %s register %u in .cfi_val_offset"),
@@ -1153,14 +1155,15 @@ sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
struct cfi_insn_data *cfi_insn)
{
/* Previous value of register1 is register2. However, if the specified
- register1 is not interesting (SP, FP, or RA reg), the current DW_CFA_register
+ register1 is not interesting (FP or RA reg), the current DW_CFA_register
instruction can be safely skipped without sacrificing the asynchronicity of
stack trace information. */
- if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
+ if (cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG
#ifdef SFRAME_FRE_RA_TRACKING
|| (sframe_ra_tracking_p () && cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
#endif
- || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
+ /* Ignore SP reg, as it can be recovered from the CFA tracking info. */
+ )
{
as_warn (_("skipping SFrame FDE; %s register %u in .cfi_register"),
sframe_register_name (cfi_insn->u.rr.reg1), cfi_insn->u.rr.reg1);
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v4 13/15] gas: Don't skip SFrame FDE if .cfi_register specifies SP register
2024-06-24 14:23 ` [PATCH v4 13/15] gas: Don't skip SFrame FDE if .cfi_register specifies SP register Jens Remus
@ 2024-06-25 23:59 ` Indu Bhagat
0 siblings, 0 replies; 32+ messages in thread
From: Indu Bhagat @ 2024-06-25 23:59 UTC (permalink / raw)
To: Jens Remus, binutils; +Cc: Andreas Krebbel
On 6/24/24 07:23, Jens Remus wrote:
> Neither ".cfi_offset SP, <offset>", ".cfi_register SP, <regno>", nor
> ".cfi_val_offset SP, <offset>" alter the tracking information to recover
> the stack pointer (SP). Doing so would need an explicit .cfi_def_cfa,
> which SFrame tracks.
>
> The stack-pointer (SP) register contents on entry can be reconstructed
Nit: stack pointer instead of stack-pointer.
> from the SFrame CFA tracking information using information from the
> current and initial SFrame FREs of the SFrame FDE:
>
> 1. Compute CFA from the current CFA base register (SP or FP) and CFA
> offset from the SFrame CFA tracking information from the SFrame FRE
> for the current instruction address:
>
> CFA = <current_base_reg> + <current_cfa_offset>
>
> 2. Compute SP from the current CFA and the CFA offset from the SFrame
> CFA tracking information from the initial SFrame FRE of the FDE:
>
> SP = CFA - <initial_cfa_offset>
>
> While at it add comments to the processing of .cfi_offset and
> .cfi_val_offset that the SP can be reconstructed from the CFA tracking
> information.
>
> gas/
> * gen-sframe.c (sframe_xlate_do_register): Do not skip SFrame
> FDE if .cfi_register specifies SP register.
> (sframe_xlate_do_offset,sframe_xlate_do_val_offset): Add comment
> that this is likewise.
>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
There is no need to post a V5 for the nit above.
LGTM
Thanks
> ---
>
> Notes (jremus):
> Changes v3 -> v4:
> - Add comment to sframe_xlate_do_offset why SP register is ignored.
> - Reword commit message accordingly.
>
> Changes v2 -> v3:
> - New patch.
>
> gas/gen-sframe.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 31f2e5118280..f4bdbb5944d9 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1100,6 +1100,7 @@ sframe_xlate_do_offset (struct sframe_xlate_ctx *xlate_ctx,
> gas_assert (cur_fre);
> /* Change the rule for the register indicated by the register number to
> be the specified offset. */
> + /* Ignore SP reg, as it can be recovered from the CFA tracking info. */
> if (cfi_insn->u.r == SFRAME_CFA_FP_REG)
> {
> gas_assert (!cur_fre->base_reg);
> @@ -1134,6 +1135,7 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
> #ifdef SFRAME_FRE_RA_TRACKING
> || (sframe_ra_tracking_p () && cfi_insn->u.r == SFRAME_CFA_RA_REG)
> #endif
> + /* Ignore SP reg, as it can be recovered from the CFA tracking info. */
> )
> {
> as_warn (_("skipping SFrame FDE; %s register %u in .cfi_val_offset"),
> @@ -1153,14 +1155,15 @@ sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
> struct cfi_insn_data *cfi_insn)
> {
> /* Previous value of register1 is register2. However, if the specified
> - register1 is not interesting (SP, FP, or RA reg), the current DW_CFA_register
> + register1 is not interesting (FP or RA reg), the current DW_CFA_register
> instruction can be safely skipped without sacrificing the asynchronicity of
> stack trace information. */
> - if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
> + if (cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG
> #ifdef SFRAME_FRE_RA_TRACKING
> || (sframe_ra_tracking_p () && cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
> #endif
> - || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
> + /* Ignore SP reg, as it can be recovered from the CFA tracking info. */
> + )
> {
> as_warn (_("skipping SFrame FDE; %s register %u in .cfi_register"),
> sframe_register_name (cfi_insn->u.rr.reg1), cfi_insn->u.rr.reg1);
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v4 14/15] gas: Test predicate whether SFrame RA tracking is used
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (12 preceding siblings ...)
2024-06-24 14:23 ` [PATCH v4 13/15] gas: Don't skip SFrame FDE if .cfi_register specifies SP register Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-24 14:23 ` [PATCH v4 15/15] gas: Validate SFrame RA tracking and fixed RA offset Jens Remus
` (2 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
The existence of the macro SFRAME_FRE_RA_TRACKING only ensures the
existence of the macro SFRAME_CFA_RA_REG and the predicate function
sframe_ra_tracking_p. It does not indicate whether SFrame RA tracking
is actually used.
Test the return value of the SFrame RA tracking predicate function
sframe_ra_tracking_p to determine whether RA tracking is used.
This aligns the logic in functions get_fre_num_offsets and
output_sframe_row_entry to the one used in all other places.
gas/
* gen-sframe.c (get_fre_num_offsets, output_sframe_row_entry):
Test predicate to determine whether SFrame RA tracking is used.
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v2 -> v3:
- New patch.
gas/gen-sframe.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index f4bdbb5944d9..f83a64518c28 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -346,7 +346,8 @@ get_fre_num_offsets (struct sframe_row_entry *sframe_fre)
if (sframe_fre->bp_loc == SFRAME_FRE_ELEM_LOC_STACK)
fre_num_offsets++;
#ifdef SFRAME_FRE_RA_TRACKING
- if (sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
+ if (sframe_ra_tracking_p ()
+ && sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
fre_num_offsets++;
#endif
return fre_num_offsets;
@@ -536,7 +537,8 @@ output_sframe_row_entry (symbolS *fde_start_addr,
fre_write_offsets++;
#ifdef SFRAME_FRE_RA_TRACKING
- if (sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
+ if (sframe_ra_tracking_p ()
+ && sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
{
fre_offset_func_map[idx].out_func (sframe_fre->ra_offset);
fre_write_offsets++;
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v4 15/15] gas: Validate SFrame RA tracking and fixed RA offset
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (13 preceding siblings ...)
2024-06-24 14:23 ` [PATCH v4 14/15] gas: Test predicate whether SFrame RA tracking is used Jens Remus
@ 2024-06-24 14:23 ` Jens Remus
2024-06-25 23:57 ` Indu Bhagat
2024-06-25 8:22 ` [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
2024-06-27 8:28 ` Jens Remus
16 siblings, 1 reply; 32+ messages in thread
From: Jens Remus @ 2024-06-24 14:23 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Jens Remus, Andreas Krebbel
Verify all architectures participating in SFrame generation do define
the mandatory SFrame return address (RA) tracking predicate function
sframe_ra_tracking_p. Do so by explicitly not testing for the macro
SFRAME_FRE_RA_TRACKING as otherwise required.
Verify that architectures not using SFrame RA tracking specify a valid
fixed RA offset.
gas/
* gen-sframe.c (output_sframe_internal): Validate SFrame
RA tracking and fixed RA offset.
Suggested-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
Changes v3 -> v4:
- Implement review feedback and suggestions from Indu.
Changes v2 -> v3:
- New patch.
This could be made dependent on ENABLE_CHECKING (configure option
--enable-checking).
gas/gen-sframe.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index f83a64518c28..626dc33b71dc 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -667,12 +667,16 @@ output_sframe_internal (void)
-fno-omit-frame-pointer is used. */
out_one (fixed_fp_offset);
- /* Offset for the return address from CFA is fixed for some ABIs
- (e.g., AMD64), output a SFRAME_CFA_FIXED_RA_INVALID otherwise. */
-#ifdef sframe_ra_tracking_p
+ /* All ABIs participating in SFrame generation must define
+ sframe_ra_tracking_p.
+ When RA tracking (in FREs) is not needed (e.g., AMD64), SFrame assumes
+ the RA is going to be at a fixed offset from CFA. Check that the fixed RA
+ offset is appropriately defined in all cases. */
if (!sframe_ra_tracking_p ())
- fixed_ra_offset = sframe_cfa_ra_offset ();
-#endif
+ {
+ fixed_ra_offset = sframe_cfa_ra_offset ();
+ gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
+ }
out_one (fixed_ra_offset);
/* None of the AMD64, or AARCH64 ABIs need the auxiliary header.
--
2.40.1
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v4 15/15] gas: Validate SFrame RA tracking and fixed RA offset
2024-06-24 14:23 ` [PATCH v4 15/15] gas: Validate SFrame RA tracking and fixed RA offset Jens Remus
@ 2024-06-25 23:57 ` Indu Bhagat
0 siblings, 0 replies; 32+ messages in thread
From: Indu Bhagat @ 2024-06-25 23:57 UTC (permalink / raw)
To: Jens Remus, binutils; +Cc: Andreas Krebbel
On 6/24/24 07:23, Jens Remus wrote:
> Verify all architectures participating in SFrame generation do define
> the mandatory SFrame return address (RA) tracking predicate function
> sframe_ra_tracking_p. Do so by explicitly not testing for the macro
> SFRAME_FRE_RA_TRACKING as otherwise required.
>
> Verify that architectures not using SFrame RA tracking specify a valid
> fixed RA offset.
>
> gas/
> * gen-sframe.c (output_sframe_internal): Validate SFrame
> RA tracking and fixed RA offset.
>
> Suggested-by: Indu Bhagat <indu.bhagat@oracle.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
LGTM.
Thanks
> ---
>
> Notes (jremus):
> Changes v3 -> v4:
> - Implement review feedback and suggestions from Indu.
>
> Changes v2 -> v3:
> - New patch.
>
> This could be made dependent on ENABLE_CHECKING (configure option
> --enable-checking).
>
> gas/gen-sframe.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index f83a64518c28..626dc33b71dc 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -667,12 +667,16 @@ output_sframe_internal (void)
> -fno-omit-frame-pointer is used. */
> out_one (fixed_fp_offset);
>
> - /* Offset for the return address from CFA is fixed for some ABIs
> - (e.g., AMD64), output a SFRAME_CFA_FIXED_RA_INVALID otherwise. */
> -#ifdef sframe_ra_tracking_p
> + /* All ABIs participating in SFrame generation must define
> + sframe_ra_tracking_p.
> + When RA tracking (in FREs) is not needed (e.g., AMD64), SFrame assumes
> + the RA is going to be at a fixed offset from CFA. Check that the fixed RA
> + offset is appropriately defined in all cases. */
> if (!sframe_ra_tracking_p ())
> - fixed_ra_offset = sframe_cfa_ra_offset ();
> -#endif
> + {
> + fixed_ra_offset = sframe_cfa_ra_offset ();
> + gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
> + }
> out_one (fixed_ra_offset);
>
> /* None of the AMD64, or AARCH64 ABIs need the auxiliary header.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 00/15] sframe: Enhancements to SFrame info generation
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (14 preceding siblings ...)
2024-06-24 14:23 ` [PATCH v4 15/15] gas: Validate SFrame RA tracking and fixed RA offset Jens Remus
@ 2024-06-25 8:22 ` Jens Remus
2024-06-25 14:04 ` Jens Remus
2024-06-27 8:28 ` Jens Remus
16 siblings, 1 reply; 32+ messages in thread
From: Jens Remus @ 2024-06-25 8:22 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Andreas Krebbel, Jan Beulich
Hello Indu!
Am 24.06.2024 um 16:23 schrieb Jens Remus:
...
> Jens Remus (15):
> x86: Remove unused SFrame CFI RA register variable
> gas: Enhance arch-specific SFrame configuration descriptions
> readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
> readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
> gas: Print DWARF call frame insn name in SFrame warning message
> gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
> gas: Warn if SFrame FDE is skipped due to non-default return column
> gas: Refactor SFrame CFI opcode DW_CFA_register processing
> gas: User readable warnings if SFrame FDE is not generated
> gas: Skip SFrame FDE if FP without RA on stack
> gas: Skip SFrame FDE if .cfi_window_save
> gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
> gas: Don't skip SFrame FDE if .cfi_register specifies SP register
> gas: Test predicate whether SFrame RA tracking is used
> gas: Validate SFrame RA tracking and fixed RA offset...
As Jan pointed out there are issues with my patches appearing severely
delayed on the list. Furthermore it seems patches 1, 6, and 9 did not
make it to the list at all. I have reached out to mailman@sourceware.org
to hopefully get this resolved.
Nevertheless you should have received the whole series since you were
explicitly listed as recipient. I am just not sure whether it makes
sense for you to review them, as your replies to patches 1, 6, and 9
would appear out of nowhere on the list.
I would like to wait for a response from mailman@sourceware.org before
attempting a resend, as this is not the first time my patches get
delayed (although the first time some are dropped).
Hopefully my missing patches are only awaiting moderation and can be
reinstated.
Regards,
Jens
--
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+49-7031-16-1128 Office
jremus@de.ibm.com
IBM
IBM Deutschland Research & Development GmbH; Vorsitzender des
Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der
Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v4 00/15] sframe: Enhancements to SFrame info generation
2024-06-25 8:22 ` [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
@ 2024-06-25 14:04 ` Jens Remus
0 siblings, 0 replies; 32+ messages in thread
From: Jens Remus @ 2024-06-25 14:04 UTC (permalink / raw)
To: binutils, Indu Bhagat; +Cc: Andreas Krebbel, Jan Beulich
Hello Indu!
Am 25.06.2024 um 10:22 schrieb Jens Remus:
> Am 24.06.2024 um 16:23 schrieb Jens Remus:
> ...
>> Jens Remus (15):
>> x86: Remove unused SFrame CFI RA register variable
>> gas: Enhance arch-specific SFrame configuration descriptions
>> readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>> readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>> gas: Print DWARF call frame insn name in SFrame warning message
>> gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>> gas: Warn if SFrame FDE is skipped due to non-default return column
>> gas: Refactor SFrame CFI opcode DW_CFA_register processing
>> gas: User readable warnings if SFrame FDE is not generated
>> gas: Skip SFrame FDE if FP without RA on stack
>> gas: Skip SFrame FDE if .cfi_window_save
>> gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>> gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>> gas: Test predicate whether SFrame RA tracking is used
>> gas: Validate SFrame RA tracking and fixed RA offset...
>
> As Jan pointed out there are issues with my patches appearing severely
> delayed on the list. Furthermore it seems patches 1, 6, and 9 did not
> make it to the list at all. I have reached out to mailman@sourceware.org
> to hopefully get this resolved.
>
> Nevertheless you should have received the whole series since you were
> explicitly listed as recipient. I am just not sure whether it makes
> sense for you to review them, as your replies to patches 1, 6, and 9
> would appear out of nowhere on the list.
>
> I would like to wait for a response from mailman@sourceware.org before
> attempting a resend, as this is not the first time my patches get
> delayed (although the first time some are dropped).
>
> Hopefully my missing patches are only awaiting moderation and can be
> reinstated.
Nick kindly assisted to get this resolved. The previously missing
patches have meanwhile shown up in my inbox and the binutils list
archive. From my perspective there is therefore no need for me to resent
and you should be safe to review my patch series from yesterday.
Regards,
Jens
--
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+49-7031-16-1128 Office
jremus@de.ibm.com
IBM
IBM Deutschland Research & Development GmbH; Vorsitzender des
Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der
Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 00/15] sframe: Enhancements to SFrame info generation
2024-06-24 14:23 [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
` (15 preceding siblings ...)
2024-06-25 8:22 ` [PATCH v4 00/15] sframe: Enhancements to SFrame info generation Jens Remus
@ 2024-06-27 8:28 ` Jens Remus
2024-06-27 8:32 ` Indu Bhagat
2024-06-27 8:39 ` Jan Beulich
16 siblings, 2 replies; 32+ messages in thread
From: Jens Remus @ 2024-06-27 8:28 UTC (permalink / raw)
To: binutils, Indu Bhagat
Cc: Andreas Krebbel, Nick Clifton, Alan Modra, Jan Beulich
Am 24.06.2024 um 16:23 schrieb Jens Remus:
> Patches 1 and 2 (updated) are minor cleanups/enhancements to the
> existing SFrame support on AArch64 and x86 AMD64.
>
> Patch 3 enables readelf/objdump to dump the SFrame fixed offsets from
> CFA to the frame pointer (FP) and return address (RA).
>
> Patch 4 changes readelf/objdump to display 'f' in the SFrame
> RA tracking column, if the architecture is using a fixed RA offset.
> Additionally it corrects the logic to display 'u' in the SFrame
> FP tracking column.
>
> Patch 5 (updated) enhances an SFrame warning message to print the human
> readable DWARF call frame instruction name.
>
> Patches 6 and 7 resolve issues that cause the assembler to either
> generate bad SFrame FDE or to silently skip it. Both issues would be
> triggered by s390-specific SFrame error test cases introduced by the
> separate patch series.
>
> Patch 8 refactors SFrame CFI opcode DW_CFA_register processing into a
> separate function. This harmonizes the CFI opcode processing.
>
> Patch 9 (updated) adds verbose assembler warning messages when
> generation of SFrame FDE is skipped.
>
> Patch 10 (updated) resolves an issue that causes the assembler to
> generate bad SFrame FDE in case the FP without RA was saved on the
> stack, which the SFrame format cannot represent. I will send two
> alternative solution proposals as RFC.
>
> Patch 11 (updated) skips SFrame FDE for .cfi_window_save on all
> architectures except AArch64, which multiplexed it with
> .cfi_negate_ra_state.
>
> Patches 12 and 13 (updated) resolve issues where generation of SFrame
> FDE was unnecessarily skipped.
>
> Patch 14 adds tests for the SFrame RA tracking predicate to places where
> it was missing to align the logic.
>
> Patch 15 (updated) is a minor enhancement to add checks that the
> architecture-dependent RA tracking is correctly configured.
...
> Jens Remus (15):
> x86: Remove unused SFrame CFI RA register variable
> gas: Enhance arch-specific SFrame configuration descriptions
> readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
> readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
> gas: Print DWARF call frame insn name in SFrame warning message
> gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
> gas: Warn if SFrame FDE is skipped due to non-default return column
> gas: Refactor SFrame CFI opcode DW_CFA_register processing
> gas: User readable warnings if SFrame FDE is not generated
> gas: Skip SFrame FDE if FP without RA on stack
> gas: Skip SFrame FDE if .cfi_window_save
> gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
> gas: Don't skip SFrame FDE if .cfi_register specifies SP register
> gas: Test predicate whether SFrame RA tracking is used
> gas: Validate SFrame RA tracking and fixed RA offset
>
> gas/config/tc-aarch64.c | 6 +-
> gas/config/tc-aarch64.h | 12 +-
> gas/config/tc-i386.c | 6 +-
> gas/config/tc-i386.h | 10 +-
> gas/gen-sframe.c | 246 +++++++++++++++---
> gas/gen-sframe.h | 2 +
> .../gas/cfi-sframe/cfi-sframe-common-1.d | 2 +
> .../gas/cfi-sframe/cfi-sframe-common-2.d | 2 +
> .../gas/cfi-sframe/cfi-sframe-common-3.d | 2 +
> .../gas/cfi-sframe/cfi-sframe-common-4.d | 6 +-
> .../gas/cfi-sframe/cfi-sframe-common-5.d | 6 +-
> .../gas/cfi-sframe/cfi-sframe-common-6.d | 6 +-
> .../gas/cfi-sframe/cfi-sframe-common-7.d | 6 +-
> .../gas/cfi-sframe/cfi-sframe-common-8.d | 4 +-
> .../gas/cfi-sframe/cfi-sframe-x86_64-1.d | 9 +-
> gas/testsuite/gas/cfi-sframe/common-empty-1.d | 4 +-
> gas/testsuite/gas/cfi-sframe/common-empty-2.d | 4 +-
> gas/testsuite/gas/cfi-sframe/common-empty-3.d | 3 +
> .../gas/scfi/x86_64/scfi-cfi-sections-1.d | 11 +-
> .../gas/scfi/x86_64/scfi-dyn-stack-1.d | 11 +-
> ld/testsuite/ld-sframe/discard.s | 1 -
> ld/testsuite/ld-x86-64/sframe-plt-1.d | 9 +-
> ld/testsuite/ld-x86-64/sframe-simple-1.d | 17 +-
> libsframe/sframe-dump.c | 18 +-
> 24 files changed, 305 insertions(+), 98 deletions(-)
Thank you for the review, Indu!
Can I go ahead and commit the series to mainline, with the review
feedback implemented and the offending trailers removed?
Thanks and regards,
Jens
--
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+49-7031-16-1128 Office
jremus@de.ibm.com
IBM
IBM Deutschland Research & Development GmbH; Vorsitzender des
Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der
Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v4 00/15] sframe: Enhancements to SFrame info generation
2024-06-27 8:28 ` Jens Remus
@ 2024-06-27 8:32 ` Indu Bhagat
2024-07-04 8:42 ` Jens Remus
2024-06-27 8:39 ` Jan Beulich
1 sibling, 1 reply; 32+ messages in thread
From: Indu Bhagat @ 2024-06-27 8:32 UTC (permalink / raw)
To: Jens Remus, binutils
Cc: Andreas Krebbel, Nick Clifton, Alan Modra, Jan Beulich
On 6/27/24 01:28, Jens Remus wrote:
> Am 24.06.2024 um 16:23 schrieb Jens Remus:
>> Patches 1 and 2 (updated) are minor cleanups/enhancements to the
>> existing SFrame support on AArch64 and x86 AMD64.
>>
>> Patch 3 enables readelf/objdump to dump the SFrame fixed offsets from
>> CFA to the frame pointer (FP) and return address (RA).
>>
>> Patch 4 changes readelf/objdump to display 'f' in the SFrame
>> RA tracking column, if the architecture is using a fixed RA offset.
>> Additionally it corrects the logic to display 'u' in the SFrame
>> FP tracking column.
>>
>> Patch 5 (updated) enhances an SFrame warning message to print the human
>> readable DWARF call frame instruction name.
>>
>> Patches 6 and 7 resolve issues that cause the assembler to either
>> generate bad SFrame FDE or to silently skip it. Both issues would be
>> triggered by s390-specific SFrame error test cases introduced by the
>> separate patch series.
>>
>> Patch 8 refactors SFrame CFI opcode DW_CFA_register processing into a
>> separate function. This harmonizes the CFI opcode processing.
>>
>> Patch 9 (updated) adds verbose assembler warning messages when
>> generation of SFrame FDE is skipped.
>>
>> Patch 10 (updated) resolves an issue that causes the assembler to
>> generate bad SFrame FDE in case the FP without RA was saved on the
>> stack, which the SFrame format cannot represent. I will send two
>> alternative solution proposals as RFC.
>>
>> Patch 11 (updated) skips SFrame FDE for .cfi_window_save on all
>> architectures except AArch64, which multiplexed it with
>> .cfi_negate_ra_state.
>>
>> Patches 12 and 13 (updated) resolve issues where generation of SFrame
>> FDE was unnecessarily skipped.
>>
>> Patch 14 adds tests for the SFrame RA tracking predicate to places where
>> it was missing to align the logic.
>>
>> Patch 15 (updated) is a minor enhancement to add checks that the
>> architecture-dependent RA tracking is correctly configured.
>
> ...
>
>> Jens Remus (15):
>> x86: Remove unused SFrame CFI RA register variable
>> gas: Enhance arch-specific SFrame configuration descriptions
>> readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>> readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>> gas: Print DWARF call frame insn name in SFrame warning message
>> gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>> gas: Warn if SFrame FDE is skipped due to non-default return column
>> gas: Refactor SFrame CFI opcode DW_CFA_register processing
>> gas: User readable warnings if SFrame FDE is not generated
>> gas: Skip SFrame FDE if FP without RA on stack
>> gas: Skip SFrame FDE if .cfi_window_save
>> gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>> gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>> gas: Test predicate whether SFrame RA tracking is used
>> gas: Validate SFrame RA tracking and fixed RA offset
>>
>> gas/config/tc-aarch64.c | 6 +-
>> gas/config/tc-aarch64.h | 12 +-
>> gas/config/tc-i386.c | 6 +-
>> gas/config/tc-i386.h | 10 +-
>> gas/gen-sframe.c | 246 +++++++++++++++---
>> gas/gen-sframe.h | 2 +
>> .../gas/cfi-sframe/cfi-sframe-common-1.d | 2 +
>> .../gas/cfi-sframe/cfi-sframe-common-2.d | 2 +
>> .../gas/cfi-sframe/cfi-sframe-common-3.d | 2 +
>> .../gas/cfi-sframe/cfi-sframe-common-4.d | 6 +-
>> .../gas/cfi-sframe/cfi-sframe-common-5.d | 6 +-
>> .../gas/cfi-sframe/cfi-sframe-common-6.d | 6 +-
>> .../gas/cfi-sframe/cfi-sframe-common-7.d | 6 +-
>> .../gas/cfi-sframe/cfi-sframe-common-8.d | 4 +-
>> .../gas/cfi-sframe/cfi-sframe-x86_64-1.d | 9 +-
>> gas/testsuite/gas/cfi-sframe/common-empty-1.d | 4 +-
>> gas/testsuite/gas/cfi-sframe/common-empty-2.d | 4 +-
>> gas/testsuite/gas/cfi-sframe/common-empty-3.d | 3 +
>> .../gas/scfi/x86_64/scfi-cfi-sections-1.d | 11 +-
>> .../gas/scfi/x86_64/scfi-dyn-stack-1.d | 11 +-
>> ld/testsuite/ld-sframe/discard.s | 1 -
>> ld/testsuite/ld-x86-64/sframe-plt-1.d | 9 +-
>> ld/testsuite/ld-x86-64/sframe-simple-1.d | 17 +-
>> libsframe/sframe-dump.c | 18 +-
>> 24 files changed, 305 insertions(+), 98 deletions(-)
>
> Thank you for the review, Indu!
>
> Can I go ahead and commit the series to mainline, with the review
> feedback implemented and the offending trailers removed?
Yes.
Thanks for improving the SFrame functionality,
Indu
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 00/15] sframe: Enhancements to SFrame info generation
2024-06-27 8:32 ` Indu Bhagat
@ 2024-07-04 8:42 ` Jens Remus
0 siblings, 0 replies; 32+ messages in thread
From: Jens Remus @ 2024-07-04 8:42 UTC (permalink / raw)
To: Indu Bhagat, binutils
Cc: Andreas Krebbel, Nick Clifton, Jan Beulich, Richard Earnshaw
Am 27.06.2024 um 10:32 schrieb Indu Bhagat:
> On 6/27/24 01:28, Jens Remus wrote:
>> Am 24.06.2024 um 16:23 schrieb Jens Remus:
...
>>> Jens Remus (15):
>>> x86: Remove unused SFrame CFI RA register variable
>>> gas: Enhance arch-specific SFrame configuration descriptions
>>> readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>>> readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>>> gas: Print DWARF call frame insn name in SFrame warning message
>>> gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>>> gas: Warn if SFrame FDE is skipped due to non-default return column
>>> gas: Refactor SFrame CFI opcode DW_CFA_register processing
>>> gas: User readable warnings if SFrame FDE is not generated
>>> gas: Skip SFrame FDE if FP without RA on stack
>>> gas: Skip SFrame FDE if .cfi_window_save
>>> gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>>> gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>>> gas: Test predicate whether SFrame RA tracking is used
>>> gas: Validate SFrame RA tracking and fixed RA offset
...
>> Can I go ahead and commit the series to mainline, with the review
>> feedback implemented and the offending trailers removed?
>
> Yes.
Committed to mainline with approval from Indu, Jan, and Richard. Thank
you for the review!
Regards,
Jens
--
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+49-7031-16-1128 Office
jremus@de.ibm.com
IBM
IBM Deutschland Research & Development GmbH; Vorsitzender des
Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der
Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 00/15] sframe: Enhancements to SFrame info generation
2024-06-27 8:28 ` Jens Remus
2024-06-27 8:32 ` Indu Bhagat
@ 2024-06-27 8:39 ` Jan Beulich
2024-06-27 11:02 ` Jens Remus
1 sibling, 1 reply; 32+ messages in thread
From: Jan Beulich @ 2024-06-27 8:39 UTC (permalink / raw)
To: Jens Remus
Cc: Andreas Krebbel, Nick Clifton, Alan Modra, binutils, Indu Bhagat
On 27.06.2024 10:28, Jens Remus wrote:
> Am 24.06.2024 um 16:23 schrieb Jens Remus:
>> Patches 1 and 2 (updated) are minor cleanups/enhancements to the
>> existing SFrame support on AArch64 and x86 AMD64.
>>
>> Patch 3 enables readelf/objdump to dump the SFrame fixed offsets from
>> CFA to the frame pointer (FP) and return address (RA).
>>
>> Patch 4 changes readelf/objdump to display 'f' in the SFrame
>> RA tracking column, if the architecture is using a fixed RA offset.
>> Additionally it corrects the logic to display 'u' in the SFrame
>> FP tracking column.
>>
>> Patch 5 (updated) enhances an SFrame warning message to print the human
>> readable DWARF call frame instruction name.
>>
>> Patches 6 and 7 resolve issues that cause the assembler to either
>> generate bad SFrame FDE or to silently skip it. Both issues would be
>> triggered by s390-specific SFrame error test cases introduced by the
>> separate patch series.
>>
>> Patch 8 refactors SFrame CFI opcode DW_CFA_register processing into a
>> separate function. This harmonizes the CFI opcode processing.
>>
>> Patch 9 (updated) adds verbose assembler warning messages when
>> generation of SFrame FDE is skipped.
>>
>> Patch 10 (updated) resolves an issue that causes the assembler to
>> generate bad SFrame FDE in case the FP without RA was saved on the
>> stack, which the SFrame format cannot represent. I will send two
>> alternative solution proposals as RFC.
>>
>> Patch 11 (updated) skips SFrame FDE for .cfi_window_save on all
>> architectures except AArch64, which multiplexed it with
>> .cfi_negate_ra_state.
>>
>> Patches 12 and 13 (updated) resolve issues where generation of SFrame
>> FDE was unnecessarily skipped.
>>
>> Patch 14 adds tests for the SFrame RA tracking predicate to places where
>> it was missing to align the logic.
>>
>> Patch 15 (updated) is a minor enhancement to add checks that the
>> architecture-dependent RA tracking is correctly configured.
>
> ...
>
>> Jens Remus (15):
>> x86: Remove unused SFrame CFI RA register variable
>> gas: Enhance arch-specific SFrame configuration descriptions
>> readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>> readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>> gas: Print DWARF call frame insn name in SFrame warning message
>> gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>> gas: Warn if SFrame FDE is skipped due to non-default return column
>> gas: Refactor SFrame CFI opcode DW_CFA_register processing
>> gas: User readable warnings if SFrame FDE is not generated
>> gas: Skip SFrame FDE if FP without RA on stack
>> gas: Skip SFrame FDE if .cfi_window_save
>> gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>> gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>> gas: Test predicate whether SFrame RA tracking is used
>> gas: Validate SFrame RA tracking and fixed RA offset
>>
>> gas/config/tc-aarch64.c | 6 +-
>> gas/config/tc-aarch64.h | 12 +-
>> gas/config/tc-i386.c | 6 +-
>> gas/config/tc-i386.h | 10 +-
>> gas/gen-sframe.c | 246 +++++++++++++++---
>> gas/gen-sframe.h | 2 +
>> .../gas/cfi-sframe/cfi-sframe-common-1.d | 2 +
>> .../gas/cfi-sframe/cfi-sframe-common-2.d | 2 +
>> .../gas/cfi-sframe/cfi-sframe-common-3.d | 2 +
>> .../gas/cfi-sframe/cfi-sframe-common-4.d | 6 +-
>> .../gas/cfi-sframe/cfi-sframe-common-5.d | 6 +-
>> .../gas/cfi-sframe/cfi-sframe-common-6.d | 6 +-
>> .../gas/cfi-sframe/cfi-sframe-common-7.d | 6 +-
>> .../gas/cfi-sframe/cfi-sframe-common-8.d | 4 +-
>> .../gas/cfi-sframe/cfi-sframe-x86_64-1.d | 9 +-
>> gas/testsuite/gas/cfi-sframe/common-empty-1.d | 4 +-
>> gas/testsuite/gas/cfi-sframe/common-empty-2.d | 4 +-
>> gas/testsuite/gas/cfi-sframe/common-empty-3.d | 3 +
>> .../gas/scfi/x86_64/scfi-cfi-sections-1.d | 11 +-
>> .../gas/scfi/x86_64/scfi-dyn-stack-1.d | 11 +-
>> ld/testsuite/ld-sframe/discard.s | 1 -
>> ld/testsuite/ld-x86-64/sframe-plt-1.d | 9 +-
>> ld/testsuite/ld-x86-64/sframe-simple-1.d | 17 +-
>> libsframe/sframe-dump.c | 18 +-
>> 24 files changed, 305 insertions(+), 98 deletions(-)
>
> Thank you for the review, Indu!
>
> Can I go ahead and commit the series to mainline, with the review
> feedback implemented and the offending trailers removed?
I haven't been following closely what specifically Indu asked for. In
general, if he's happy with the sframe-specific changes, respective
changes can have my (implicit) okay. Target-specific changes will want
a target-specific okay, though. From the titles I can only identify
patch 1 as having x86-specific aspects. Looks like it's only patch 2
which has further arch-specific changes - I'm okay with the x86 parts,
but I can't speak for Arm64 (well, technically I could, but I'd like
to avoid doing so unless strictly necessary).
Jan
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 00/15] sframe: Enhancements to SFrame info generation
2024-06-27 8:39 ` Jan Beulich
@ 2024-06-27 11:02 ` Jens Remus
2024-07-03 13:09 ` Richard Earnshaw (lists)
0 siblings, 1 reply; 32+ messages in thread
From: Jens Remus @ 2024-06-27 11:02 UTC (permalink / raw)
To: Richard Earnshaw, Marcus Shawcroft
Cc: Andreas Krebbel, Nick Clifton, Alan Modra, binutils, Indu Bhagat,
Jan Beulich
Am 27.06.2024 um 10:39 schrieb Jan Beulich:
> On 27.06.2024 10:28, Jens Remus wrote:
>> Am 24.06.2024 um 16:23 schrieb Jens Remus:
>>> Patches 1 and 2 (updated) are minor cleanups/enhancements to the
>>> existing SFrame support on AArch64 and x86 AMD64.
...
>>> Jens Remus (15):
>>> x86: Remove unused SFrame CFI RA register variable
>>> gas: Enhance arch-specific SFrame configuration descriptions
>>> readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>>> readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>>> gas: Print DWARF call frame insn name in SFrame warning message
>>> gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>>> gas: Warn if SFrame FDE is skipped due to non-default return column
>>> gas: Refactor SFrame CFI opcode DW_CFA_register processing
>>> gas: User readable warnings if SFrame FDE is not generated
>>> gas: Skip SFrame FDE if FP without RA on stack
>>> gas: Skip SFrame FDE if .cfi_window_save
>>> gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>>> gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>>> gas: Test predicate whether SFrame RA tracking is used
>>> gas: Validate SFrame RA tracking and fixed RA offset
>>>
>>> gas/config/tc-aarch64.c | 6 +-
>>> gas/config/tc-aarch64.h | 12 +-
>>> gas/config/tc-i386.c | 6 +-
>>> gas/config/tc-i386.h | 10 +-
>>> gas/gen-sframe.c | 246 +++++++++++++++---
>>> gas/gen-sframe.h | 2 +
>>> .../gas/cfi-sframe/cfi-sframe-common-1.d | 2 +
>>> .../gas/cfi-sframe/cfi-sframe-common-2.d | 2 +
>>> .../gas/cfi-sframe/cfi-sframe-common-3.d | 2 +
>>> .../gas/cfi-sframe/cfi-sframe-common-4.d | 6 +-
>>> .../gas/cfi-sframe/cfi-sframe-common-5.d | 6 +-
>>> .../gas/cfi-sframe/cfi-sframe-common-6.d | 6 +-
>>> .../gas/cfi-sframe/cfi-sframe-common-7.d | 6 +-
>>> .../gas/cfi-sframe/cfi-sframe-common-8.d | 4 +-
>>> .../gas/cfi-sframe/cfi-sframe-x86_64-1.d | 9 +-
>>> gas/testsuite/gas/cfi-sframe/common-empty-1.d | 4 +-
>>> gas/testsuite/gas/cfi-sframe/common-empty-2.d | 4 +-
>>> gas/testsuite/gas/cfi-sframe/common-empty-3.d | 3 +
>>> .../gas/scfi/x86_64/scfi-cfi-sections-1.d | 11 +-
>>> .../gas/scfi/x86_64/scfi-dyn-stack-1.d | 11 +-
>>> ld/testsuite/ld-sframe/discard.s | 1 -
>>> ld/testsuite/ld-x86-64/sframe-plt-1.d | 9 +-
>>> ld/testsuite/ld-x86-64/sframe-simple-1.d | 17 +-
>>> libsframe/sframe-dump.c | 18 +-
>>> 24 files changed, 305 insertions(+), 98 deletions(-)
>>
>> Thank you for the review, Indu!
>>
>> Can I go ahead and commit the series to mainline, with the review
>> feedback implemented and the offending trailers removed?
>
> I haven't been following closely what specifically Indu asked for. In
> general, if he's happy with the sframe-specific changes, respective
> changes can have my (implicit) okay. Target-specific changes will want
> a target-specific okay, though. From the titles I can only identify
> patch 1 as having x86-specific aspects. Looks like it's only patch 2
> which has further arch-specific changes - I'm okay with the x86 parts,
> but I can't speak for Arm64 (well, technically I could, but I'd like
> to avoid doing so unless strictly necessary).
Richard and Marcus, could one of you please have a short look at patch 2
of this series, as it contains changes to the AArch64-specific assembler
code? Note that it only rewords and harmonizes generic comments that are
SFrame specific. Therefore I had not considered to Cc you as AArch64
maintainers nor the x86-64 maintainers and assumed Indu's ok as SFrame
maintainer would be sufficient.
Thanks and regards,
Jens
--
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+49-7031-16-1128 Office
jremus@de.ibm.com
IBM
IBM Deutschland Research & Development GmbH; Vorsitzender des
Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der
Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 00/15] sframe: Enhancements to SFrame info generation
2024-06-27 11:02 ` Jens Remus
@ 2024-07-03 13:09 ` Richard Earnshaw (lists)
0 siblings, 0 replies; 32+ messages in thread
From: Richard Earnshaw (lists) @ 2024-07-03 13:09 UTC (permalink / raw)
To: Jens Remus, Richard Earnshaw, Marcus Shawcroft
Cc: Andreas Krebbel, Nick Clifton, Alan Modra, binutils, Indu Bhagat,
Jan Beulich
On 27/06/2024 12:02, Jens Remus wrote:
> Am 27.06.2024 um 10:39 schrieb Jan Beulich:
>> On 27.06.2024 10:28, Jens Remus wrote:
>>> Am 24.06.2024 um 16:23 schrieb Jens Remus:
>>>> Patches 1 and 2 (updated) are minor cleanups/enhancements to the
>>>> existing SFrame support on AArch64 and x86 AMD64.
> ...
>>>> Jens Remus (15):
>>>> x86: Remove unused SFrame CFI RA register variable
>>>> gas: Enhance arch-specific SFrame configuration descriptions
>>>> readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>>>> readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>>>> gas: Print DWARF call frame insn name in SFrame warning message
>>>> gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>>>> gas: Warn if SFrame FDE is skipped due to non-default return column
>>>> gas: Refactor SFrame CFI opcode DW_CFA_register processing
>>>> gas: User readable warnings if SFrame FDE is not generated
>>>> gas: Skip SFrame FDE if FP without RA on stack
>>>> gas: Skip SFrame FDE if .cfi_window_save
>>>> gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>>>> gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>>>> gas: Test predicate whether SFrame RA tracking is used
>>>> gas: Validate SFrame RA tracking and fixed RA offset
>>>>
>>>> gas/config/tc-aarch64.c | 6 +-
>>>> gas/config/tc-aarch64.h | 12 +-
>>>> gas/config/tc-i386.c | 6 +-
>>>> gas/config/tc-i386.h | 10 +-
>>>> gas/gen-sframe.c | 246 +++++++++++++++---
>>>> gas/gen-sframe.h | 2 +
>>>> .../gas/cfi-sframe/cfi-sframe-common-1.d | 2 +
>>>> .../gas/cfi-sframe/cfi-sframe-common-2.d | 2 +
>>>> .../gas/cfi-sframe/cfi-sframe-common-3.d | 2 +
>>>> .../gas/cfi-sframe/cfi-sframe-common-4.d | 6 +-
>>>> .../gas/cfi-sframe/cfi-sframe-common-5.d | 6 +-
>>>> .../gas/cfi-sframe/cfi-sframe-common-6.d | 6 +-
>>>> .../gas/cfi-sframe/cfi-sframe-common-7.d | 6 +-
>>>> .../gas/cfi-sframe/cfi-sframe-common-8.d | 4 +-
>>>> .../gas/cfi-sframe/cfi-sframe-x86_64-1.d | 9 +-
>>>> gas/testsuite/gas/cfi-sframe/common-empty-1.d | 4 +-
>>>> gas/testsuite/gas/cfi-sframe/common-empty-2.d | 4 +-
>>>> gas/testsuite/gas/cfi-sframe/common-empty-3.d | 3 +
>>>> .../gas/scfi/x86_64/scfi-cfi-sections-1.d | 11 +-
>>>> .../gas/scfi/x86_64/scfi-dyn-stack-1.d | 11 +-
>>>> ld/testsuite/ld-sframe/discard.s | 1 -
>>>> ld/testsuite/ld-x86-64/sframe-plt-1.d | 9 +-
>>>> ld/testsuite/ld-x86-64/sframe-simple-1.d | 17 +-
>>>> libsframe/sframe-dump.c | 18 +-
>>>> 24 files changed, 305 insertions(+), 98 deletions(-)
>>>
>>> Thank you for the review, Indu!
>>>
>>> Can I go ahead and commit the series to mainline, with the review
>>> feedback implemented and the offending trailers removed?
>>
>> I haven't been following closely what specifically Indu asked for. In
>> general, if he's happy with the sframe-specific changes, respective
>> changes can have my (implicit) okay. Target-specific changes will want
>> a target-specific okay, though. From the titles I can only identify
>> patch 1 as having x86-specific aspects. Looks like it's only patch 2
>> which has further arch-specific changes - I'm okay with the x86 parts,
>> but I can't speak for Arm64 (well, technically I could, but I'd like
>> to avoid doing so unless strictly necessary).
>
> Richard and Marcus, could one of you please have a short look at patch 2 of this series, as it contains changes to the AArch64-specific assembler code? Note that it only rewords and harmonizes generic comments that are SFrame specific. Therefore I had not considered to Cc you as AArch64 maintainers nor the x86-64 maintainers and assumed Indu's ok as SFrame maintainer would be sufficient.
The aarch64 changes look to me to all be comment clarifications related to the SFrame stuff. If Indu is happy, then so am I.
R.
>
> Thanks and regards,
> Jens
^ permalink raw reply [flat|nested] 32+ messages in thread