[v3,10/15] gas: Skip SFrame FDE if FP without RA on stack
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Testing passed
|
Commit Message
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 due to 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.
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
Notes (jremus):
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 | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
Comments
Hello Indu,
Am 12.04.2024 um 16:47 schrieb Jens Remus:
> 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.
[...]
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index a3b6f75cfe85..87be3eb05ad2 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1439,6 +1439,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 due to FP without RA on stack"));
> + return SFRAME_XLATE_ERR_NOTREPRESENTED;
> + }
> + }
> + }
> +#endif /* SFRAME_FRE_RA_TRACKING */
> +
> return SFRAME_XLATE_OK;
> }
I noticed that above new warning is erroneously emitted when assembling
the following CFI directive sequence with option "-alh" (to output a
listing of the assembly; probably any "-a[...]") on a SFrame enabled
target, that uses FP and RA tracking.
.cfi_offset <fp-regno>, <fp-offset>
.cfi_offset <ra-regno>, <ra-offset>
The reason is that with listings enabled there is an additional DWARF
DW_CFA_advance_loc CFI instruction (with a zero advance) between both
DW_CFA_offset instructions, that the DWARF .eh_frame generator is able
to process correctly, but causes the .sframe generator to choke.
Additionally with this patch reverted "bad" SFrame information is
generated (see example below), where there are multiple SFrame FREs for
the same PC start address.
Note that the FP-tracking information erroneously being displayed in the
RA-tracking column, is why I introduced this new warning message. I will
send two alternative patches how to potentially resolve that soon.
$ cat test_fpra_min.s
.cfi_sections .sframe, .eh_frame
.cfi_startproc
stmg %r11,%r15,88(%r15)
.cfi_rel_offset 11, 88
.cfi_rel_offset 14, 112
la %r11,0
la %r14,0
.Lreturn:
lmg %r11,%r15,88(%r15)
.cfi_restore 14
.cfi_restore 11
br %r14
.cfi_endproc
$ ojbdump --sframe test_fpra_without-alh.o
...
Function Index :
func idx [0]: pc = 0x0, size = 22 bytes
STARTPC CFA FP RA
0000000000000000 sp+160 u u
0000000000000006 sp+160 c-72 c-48
0000000000000014 sp+160 u u
$ objdump --sframe test_fpra_with_alh.o
...
Function Index :
func idx [0]: pc = 0x0, size = 22 bytes
STARTPC CFA FP RA
0000000000000000 sp+160 u u
0000000000000006 sp+160 u c-72
0000000000000006 sp+160 c-72 c-48
0000000000000014 sp+160 u c-72
0000000000000014 sp+160 u u
Note that the outputs of "objdump -Wf" and "objdump -WF" are identical
in both cases (with and without option "-alh").
Debugging of the SFrame processing of the DWARF CFI instructions shows
that with option "-a" there are additional DW_CFA_advance_loc:
DW_CFA_def_cfa: reg=15 offset=160
DW_CFA_advance_loc: lab1=L0, lab2=L0
DW_CFA_offset: reg=11 offset=-72
DW_CFA_advance_loc: lab1=L0, lab2=L0 <-- only with -a
DW_CFA_offset: reg=14 offset=-48
DW_CFA_advance_loc: lab1=L0, lab2=L0
DW_CFA_restore: reg=14
DW_CFA_advance_loc: lab1=L0, lab2=L0 <-- only with -a
DW_CFA_restore: reg=11
Debugging of the CFI directive processing in gas/dw2gencfi.c shows the
following:
- With option "-a" cfi_add_advance_loc() is invoked more often in
dot_cfi() due to the condition (symbol_get_frag
(frchain_now->frch_cfi_data->last_address) != frag_now) evaluating to true.
- output_cfi_insn() of case DW_CFA_advance_loc enters the condition
(symbol_get_frag (to) == symbol_get_frag (from)) without option "-a" and
enters the else condition with option "-a". The else path has an
interesting comment that suggests that there is logic to relax an
advance by zero at a later stage:
"... Call frag_grow with the sum of room needed by frag_more and
frag_var to preallocate space ensuring that the DW_CFA_advance_loc4 is
in the fixed part of the rs_cfa frag, so that the relax machinery can
remove the advance_loc should it advance by zero."
I don't have a clue how to resolve this potential issue in the SFrame
generation. I could not figure out how to detect the advance of zero in
the SFrame processing of DW_CFA_advance_loc, so that it could be treated
special.
I can open a ticket in the Sourceware Bugzilla, if you agree that this
is an issue.
Thanks and regards,
Jens
On 4/16/24 06:14, Jens Remus wrote:
> Hello Indu,
>
> Am 12.04.2024 um 16:47 schrieb Jens Remus:
>> 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.
>
> [...]
>
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>> index a3b6f75cfe85..87be3eb05ad2 100644
>> --- a/gas/gen-sframe.c
>> +++ b/gas/gen-sframe.c
>> @@ -1439,6 +1439,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 due to FP without RA on
>> stack"));
>> + return SFRAME_XLATE_ERR_NOTREPRESENTED;
>> + }
>> + }
>> + }
>> +#endif /* SFRAME_FRE_RA_TRACKING */
>> +
>> return SFRAME_XLATE_OK;
>> }
>
> I noticed that above new warning is erroneously emitted when assembling
> the following CFI directive sequence with option "-alh" (to output a
> listing of the assembly; probably any "-a[...]") on a SFrame enabled
> target, that uses FP and RA tracking.
>
> .cfi_offset <fp-regno>, <fp-offset>
> .cfi_offset <ra-regno>, <ra-offset>
>
> The reason is that with listings enabled there is an additional DWARF
> DW_CFA_advance_loc CFI instruction (with a zero advance) between both
> DW_CFA_offset instructions, that the DWARF .eh_frame generator is able
> to process correctly, but causes the .sframe generator to choke.
>
> Additionally with this patch reverted "bad" SFrame information is
> generated (see example below), where there are multiple SFrame FREs for
> the same PC start address.
> Note that the FP-tracking information erroneously being displayed in the
> RA-tracking column, is why I introduced this new warning message. I will
> send two alternative patches how to potentially resolve that soon.
>
> $ cat test_fpra_min.s
> .cfi_sections .sframe, .eh_frame
> .cfi_startproc
> stmg %r11,%r15,88(%r15)
> .cfi_rel_offset 11, 88
> .cfi_rel_offset 14, 112
> la %r11,0
> la %r14,0
> .Lreturn:
> lmg %r11,%r15,88(%r15)
> .cfi_restore 14
> .cfi_restore 11
> br %r14
> .cfi_endproc
>
> $ ojbdump --sframe test_fpra_without-alh.o
> ...
> Function Index :
>
> func idx [0]: pc = 0x0, size = 22 bytes
> STARTPC CFA FP RA
> 0000000000000000 sp+160 u u
> 0000000000000006 sp+160 c-72 c-48
> 0000000000000014 sp+160 u u
>
> $ objdump --sframe test_fpra_with_alh.o
> ...
> Function Index :
>
> func idx [0]: pc = 0x0, size = 22 bytes
> STARTPC CFA FP RA
> 0000000000000000 sp+160 u u
> 0000000000000006 sp+160 u c-72
> 0000000000000006 sp+160 c-72 c-48
> 0000000000000014 sp+160 u c-72
> 0000000000000014 sp+160 u u
>
> Note that the outputs of "objdump -Wf" and "objdump -WF" are identical
> in both cases (with and without option "-alh").
>
> Debugging of the SFrame processing of the DWARF CFI instructions shows
> that with option "-a" there are additional DW_CFA_advance_loc:
>
> DW_CFA_def_cfa: reg=15 offset=160
> DW_CFA_advance_loc: lab1=L0, lab2=L0
> DW_CFA_offset: reg=11 offset=-72
> DW_CFA_advance_loc: lab1=L0, lab2=L0 <-- only with -a
> DW_CFA_offset: reg=14 offset=-48
> DW_CFA_advance_loc: lab1=L0, lab2=L0
> DW_CFA_restore: reg=14
> DW_CFA_advance_loc: lab1=L0, lab2=L0 <-- only with -a
> DW_CFA_restore: reg=11
>
> Debugging of the CFI directive processing in gas/dw2gencfi.c shows the
> following:
>
> - With option "-a" cfi_add_advance_loc() is invoked more often in
> dot_cfi() due to the condition (symbol_get_frag
> (frchain_now->frch_cfi_data->last_address) != frag_now) evaluating to true.
>
> - output_cfi_insn() of case DW_CFA_advance_loc enters the condition
> (symbol_get_frag (to) == symbol_get_frag (from)) without option "-a" and
> enters the else condition with option "-a". The else path has an
> interesting comment that suggests that there is logic to relax an
> advance by zero at a later stage:
>
> "... Call frag_grow with the sum of room needed by frag_more and
> frag_var to preallocate space ensuring that the DW_CFA_advance_loc4 is
> in the fixed part of the rs_cfa frag, so that the relax machinery can
> remove the advance_loc should it advance by zero."
>
> I don't have a clue how to resolve this potential issue in the SFrame
> generation. I could not figure out how to detect the advance of zero in
> the SFrame processing of DW_CFA_advance_loc, so that it could be treated
> special.
> I can open a ticket in the Sourceware Bugzilla, if you agree that this
> is an issue.
>
Hi Jens,
Confirming that its reproducible and that this is an issue in the
existing implementation. Please go ahead and create a bugzilla.
Thanks
Am 18.04.2024 um 01:56 schrieb Indu Bhagat:
> On 4/16/24 06:14, Jens Remus wrote:
>> Hello Indu,
>>
>> Am 12.04.2024 um 16:47 schrieb Jens Remus:
>>> 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.
>>
>> [...]
>>
>>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>>> index a3b6f75cfe85..87be3eb05ad2 100644
>>> --- a/gas/gen-sframe.c
>>> +++ b/gas/gen-sframe.c
>>> @@ -1439,6 +1439,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 due to FP without RA on
>>> stack"));
>>> + return SFRAME_XLATE_ERR_NOTREPRESENTED;
>>> + }
>>> + }
>>> + }
>>> +#endif /* SFRAME_FRE_RA_TRACKING */
>>> +
>>> return SFRAME_XLATE_OK;
>>> }
>>
>> I noticed that above new warning is erroneously emitted when
>> assembling the following CFI directive sequence with option "-alh" (to
>> output a listing of the assembly; probably any "-a[...]") on a SFrame
>> enabled target, that uses FP and RA tracking.
>>
>> .cfi_offset <fp-regno>, <fp-offset>
>> .cfi_offset <ra-regno>, <ra-offset>
>>
>> The reason is that with listings enabled there is an additional DWARF
>> DW_CFA_advance_loc CFI instruction (with a zero advance) between both
>> DW_CFA_offset instructions, that the DWARF .eh_frame generator is able
>> to process correctly, but causes the .sframe generator to choke.
>>
>> Additionally with this patch reverted "bad" SFrame information is
>> generated (see example below), where there are multiple SFrame FREs
>> for the same PC start address.
>> Note that the FP-tracking information erroneously being displayed in
>> the RA-tracking column, is why I introduced this new warning message.
>> I will send two alternative patches how to potentially resolve that soon.
>>
>> $ cat test_fpra_min.s
>> .cfi_sections .sframe, .eh_frame
>> .cfi_startproc
>> stmg %r11,%r15,88(%r15)
>> .cfi_rel_offset 11, 88
>> .cfi_rel_offset 14, 112
>> la %r11,0
>> la %r14,0
>> .Lreturn:
>> lmg %r11,%r15,88(%r15)
>> .cfi_restore 14
>> .cfi_restore 11
>> br %r14
>> .cfi_endproc
>>
>> $ ojbdump --sframe test_fpra_without-alh.o
>> ...
>> Function Index :
>>
>> func idx [0]: pc = 0x0, size = 22 bytes
>> STARTPC CFA FP RA
>> 0000000000000000 sp+160 u u
>> 0000000000000006 sp+160 c-72 c-48
>> 0000000000000014 sp+160 u u
>>
>> $ objdump --sframe test_fpra_with_alh.o
>> ...
>> Function Index :
>>
>> func idx [0]: pc = 0x0, size = 22 bytes
>> STARTPC CFA FP RA
>> 0000000000000000 sp+160 u u
>> 0000000000000006 sp+160 u c-72
>> 0000000000000006 sp+160 c-72 c-48
>> 0000000000000014 sp+160 u c-72
>> 0000000000000014 sp+160 u u
>>
>> Note that the outputs of "objdump -Wf" and "objdump -WF" are identical
>> in both cases (with and without option "-alh").
>>
>> Debugging of the SFrame processing of the DWARF CFI instructions shows
>> that with option "-a" there are additional DW_CFA_advance_loc:
>>
>> DW_CFA_def_cfa: reg=15 offset=160
>> DW_CFA_advance_loc: lab1=L0, lab2=L0
>> DW_CFA_offset: reg=11 offset=-72
>> DW_CFA_advance_loc: lab1=L0, lab2=L0 <-- only with -a
>> DW_CFA_offset: reg=14 offset=-48
>> DW_CFA_advance_loc: lab1=L0, lab2=L0
>> DW_CFA_restore: reg=14
>> DW_CFA_advance_loc: lab1=L0, lab2=L0 <-- only with -a
>> DW_CFA_restore: reg=11
>>
>> Debugging of the CFI directive processing in gas/dw2gencfi.c shows the
>> following:
>>
>> - With option "-a" cfi_add_advance_loc() is invoked more often in
>> dot_cfi() due to the condition (symbol_get_frag
>> (frchain_now->frch_cfi_data->last_address) != frag_now) evaluating to
>> true.
>>
>> - output_cfi_insn() of case DW_CFA_advance_loc enters the condition
>> (symbol_get_frag (to) == symbol_get_frag (from)) without option "-a"
>> and enters the else condition with option "-a". The else path has an
>> interesting comment that suggests that there is logic to relax an
>> advance by zero at a later stage:
>>
>> "... Call frag_grow with the sum of room needed by frag_more and
>> frag_var to preallocate space ensuring that the DW_CFA_advance_loc4 is
>> in the fixed part of the rs_cfa frag, so that the relax machinery can
>> remove the advance_loc should it advance by zero."
>>
>> I don't have a clue how to resolve this potential issue in the SFrame
>> generation. I could not figure out how to detect the advance of zero
>> in the SFrame processing of DW_CFA_advance_loc, so that it could be
>> treated special.
>> I can open a ticket in the Sourceware Bugzilla, if you agree that this
>> is an issue.
>>
>
> Hi Jens,
>
> Confirming that its reproducible and that this is an issue in the
> existing implementation. Please go ahead and create a bugzilla.
>
> Thanks
Hello Indu,
thanks for confirming! Opened:
https://sourceware.org/bugzilla/show_bug.cgi?id=31654
Regards,
Jens
On 4/12/24 07:47, Jens Remus wrote:
> 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 due to FP without RA on stack
>
OK.
One comment below, otherwise LGTM.
Thanks
> gas/
> * gen-sframe.c (sframe_do_fde): Skip SFrame FDE if FP without RA
> on stack, as the SFrame format cannot represent this case.
>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
>
> Notes (jremus):
> 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 | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index a3b6f75cfe85..87be3eb05ad2 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1439,6 +1439,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 due to FP without RA on stack"));
> + return SFRAME_XLATE_ERR_NOTREPRESENTED;
> + }
> + }
> + }
> +#endif /* SFRAME_FRE_RA_TRACKING */
> +
There is a comment "/* No errors encountered. */" at line 1452 (few
lines above the proposed diff after applying). I suggest we also remove
/ move that comment too.
> return SFRAME_XLATE_OK;
> }
>
@@ -1439,6 +1439,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 due to FP without RA on stack"));
+ return SFRAME_XLATE_ERR_NOTREPRESENTED;
+ }
+ }
+ }
+#endif /* SFRAME_FRE_RA_TRACKING */
+
return SFRAME_XLATE_OK;
}