| Message ID | 20200409142011.GG987@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64: Fix .cfi_window_save with pac-ret [PR94515] | expand |
Hi Szabolcs, > -----Original Message----- > From: Szabolcs Nagy <Szabolcs.Nagy@arm.com> > Sent: 09 April 2020 15:20 > To: gcc-patches@gcc.gnu.org > Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>; Richard Sandiford > <Richard.Sandiford@arm.com>; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > Subject: [PATCH] aarch64: Fix .cfi_window_save with pac-ret [PR94515] > > On aarch64 -mbranch-protection=pac-ret reuses the dwarf > opcode for window_save to mean "toggle the return address > mangle state", but in the dwarf2cfi internal logic the > state was not properly recorded so the remember/restore > logic was broken. > > This can cause the unwinder not to authenticate return > addresses that were signed (or vice versa) which means > a runtime crash on a pauth enabled system. > > Currently only aarch64 pac-ret uses REG_CFA_TOGGLE_RA_MANGLE. I think this is ok, but this code is in the midend so I've CC'ed Jason who, from what I gather from MAINTAINERS, is maintainer for this file. Thanks, Kyrill > > gcc/ChangeLog: > > 2020-04-XX Szabolcs Nagy <szabolcs.nagy@arm.com> > > PR target/94515 > * dwarf2cfi.c (dwarf2out_frame_debug): Record RA > mangle state in cur_row->window_save. > > gcc/testsuite/ChangeLog: > > 2020-04-XX Szabolcs Nagy <szabolcs.nagy@arm.com> > > PR target/94515 > * g++.target/aarch64/pr94515.C: New test. > --- > gcc/dwarf2cfi.c | 3 ++ > gcc/testsuite/g++.target/aarch64/pr94515.C | 43 ++++++++++++++++++++++ > 2 files changed, 46 insertions(+) > create mode 100644 gcc/testsuite/g++.target/aarch64/pr94515.C > > diff --git a/gcc/dwarf2cfi.c b/gcc/dwarf2cfi.c > index 229fbfacc30..22989a6c2fb 100644 > --- a/gcc/dwarf2cfi.c > +++ b/gcc/dwarf2cfi.c > @@ -2145,6 +2145,9 @@ dwarf2out_frame_debug (rtx_insn *insn) > case REG_CFA_TOGGLE_RA_MANGLE: > /* This uses the same DWARF opcode as the next operation. */ > dwarf2out_frame_debug_cfa_window_save (true); > + /* Keep track of RA mangle state by toggling the window_save bit. > + This is needed so .cfi_window_save is emitted correctly. */ > + cur_row->window_save = !cur_row->window_save; > handled_one = true; > break; > > diff --git a/gcc/testsuite/g++.target/aarch64/pr94515.C > b/gcc/testsuite/g++.target/aarch64/pr94515.C > new file mode 100644 > index 00000000000..7707c773a72 > --- /dev/null > +++ b/gcc/testsuite/g++.target/aarch64/pr94515.C > @@ -0,0 +1,43 @@ > +/* PR target/94515. Check .cfi_window_save with multiple return paths. */ > +/* { dg-do run } */ > +/* { dg-additional-options "-O2 -mbranch-protection=pac-ret --save-temps" } > */ > + > +volatile int zero = 0; > + > +__attribute__((noinline, target("branch-protection=none"))) > +void unwind (void) > +{ > + if (zero == 0) > + throw 42; > +} > + > +__attribute__((noinline, noipa, target("branch-protection=pac-ret"))) > +int test (int z) > +{ > + if (z) { > + asm volatile ("":::"x20","x21"); > + unwind (); > + return 1; > + } else { > + unwind (); > + return 2; > + } > +} > + > +__attribute__((target("branch-protection=none"))) > +int main () > +{ > + try { > + test (zero); > + __builtin_abort (); > + } catch (...) { > + return 0; > + } > + __builtin_abort (); > +} > + > +/* This check only works if there are two return paths in test and > + cfi_window_save is used for both instead of cfi_remember_state > + plus cfi_restore_state. This is currently the case with -O2. */ > + > +/* { dg-final { scan-assembler-times {\t\.cfi_window_save\n} 4 } } */ > -- > 2.17.1
On 4/17/20 6:08 AM, Kyrylo Tkachov wrote: > Hi Szabolcs, > >> -----Original Message----- >> From: Szabolcs Nagy <Szabolcs.Nagy@arm.com> >> Sent: 09 April 2020 15:20 >> To: gcc-patches@gcc.gnu.org >> Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>; Richard Sandiford >> <Richard.Sandiford@arm.com>; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> >> Subject: [PATCH] aarch64: Fix .cfi_window_save with pac-ret [PR94515] >> >> On aarch64 -mbranch-protection=pac-ret reuses the dwarf >> opcode for window_save to mean "toggle the return address >> mangle state", but in the dwarf2cfi internal logic the >> state was not properly recorded so the remember/restore >> logic was broken. >> >> This can cause the unwinder not to authenticate return >> addresses that were signed (or vice versa) which means >> a runtime crash on a pauth enabled system. >> >> Currently only aarch64 pac-ret uses REG_CFA_TOGGLE_RA_MANGLE. > > I think this is ok, but this code is in the midend so I've CC'ed Jason who, from what I gather from MAINTAINERS, is maintainer for this file. > Thanks, > Kyrill > >> >> gcc/ChangeLog: >> >> 2020-04-XX Szabolcs Nagy <szabolcs.nagy@arm.com> >> >> PR target/94515 >> * dwarf2cfi.c (dwarf2out_frame_debug): Record RA >> mangle state in cur_row->window_save. >> >> gcc/testsuite/ChangeLog: >> >> 2020-04-XX Szabolcs Nagy <szabolcs.nagy@arm.com> >> >> PR target/94515 >> * g++.target/aarch64/pr94515.C: New test. >> --- >> gcc/dwarf2cfi.c | 3 ++ >> gcc/testsuite/g++.target/aarch64/pr94515.C | 43 ++++++++++++++++++++++ >> 2 files changed, 46 insertions(+) >> create mode 100644 gcc/testsuite/g++.target/aarch64/pr94515.C >> >> diff --git a/gcc/dwarf2cfi.c b/gcc/dwarf2cfi.c >> index 229fbfacc30..22989a6c2fb 100644 >> --- a/gcc/dwarf2cfi.c >> +++ b/gcc/dwarf2cfi.c >> @@ -2145,6 +2145,9 @@ dwarf2out_frame_debug (rtx_insn *insn) >> case REG_CFA_TOGGLE_RA_MANGLE: >> /* This uses the same DWARF opcode as the next operation. */ >> dwarf2out_frame_debug_cfa_window_save (true); >> + /* Keep track of RA mangle state by toggling the window_save bit. >> + This is needed so .cfi_window_save is emitted correctly. */ >> + cur_row->window_save = !cur_row->window_save; It looks like passing 'true' to dwarf2out_frame_debug_cfa_window_save prevents that function from messing with the window_safe flag. Does changing the argument to 'false' get the behavior you want? >> handled_one = true; >> break; >> >> diff --git a/gcc/testsuite/g++.target/aarch64/pr94515.C >> b/gcc/testsuite/g++.target/aarch64/pr94515.C >> new file mode 100644 >> index 00000000000..7707c773a72 >> --- /dev/null >> +++ b/gcc/testsuite/g++.target/aarch64/pr94515.C >> @@ -0,0 +1,43 @@ >> +/* PR target/94515. Check .cfi_window_save with multiple return paths. */ >> +/* { dg-do run } */ >> +/* { dg-additional-options "-O2 -mbranch-protection=pac-ret --save-temps" } >> */ >> + >> +volatile int zero = 0; >> + >> +__attribute__((noinline, target("branch-protection=none"))) >> +void unwind (void) >> +{ >> + if (zero == 0) >> + throw 42; >> +} >> + >> +__attribute__((noinline, noipa, target("branch-protection=pac-ret"))) >> +int test (int z) >> +{ >> + if (z) { >> + asm volatile ("":::"x20","x21"); >> + unwind (); >> + return 1; >> + } else { >> + unwind (); >> + return 2; >> + } >> +} >> + >> +__attribute__((target("branch-protection=none"))) >> +int main () >> +{ >> + try { >> + test (zero); >> + __builtin_abort (); >> + } catch (...) { >> + return 0; >> + } >> + __builtin_abort (); >> +} >> + >> +/* This check only works if there are two return paths in test and >> + cfi_window_save is used for both instead of cfi_remember_state >> + plus cfi_restore_state. This is currently the case with -O2. */ >> + >> +/* { dg-final { scan-assembler-times {\t\.cfi_window_save\n} 4 } } */ >> -- >> 2.17.1 >
The 04/17/2020 12:50, Jason Merrill wrote: > On 4/17/20 6:08 AM, Kyrylo Tkachov wrote: > > Hi Szabolcs, > > > > > -----Original Message----- > > > From: Szabolcs Nagy <Szabolcs.Nagy@arm.com> > > > Sent: 09 April 2020 15:20 > > > To: gcc-patches@gcc.gnu.org > > > Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>; Richard Sandiford > > > <Richard.Sandiford@arm.com>; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > > > Subject: [PATCH] aarch64: Fix .cfi_window_save with pac-ret [PR94515] > > > > > > On aarch64 -mbranch-protection=pac-ret reuses the dwarf > > > opcode for window_save to mean "toggle the return address > > > mangle state", but in the dwarf2cfi internal logic the > > > state was not properly recorded so the remember/restore > > > logic was broken. > > > > > > This can cause the unwinder not to authenticate return > > > addresses that were signed (or vice versa) which means > > > a runtime crash on a pauth enabled system. > > > > > > Currently only aarch64 pac-ret uses REG_CFA_TOGGLE_RA_MANGLE. > > > > I think this is ok, but this code is in the midend so I've CC'ed Jason who, from what I gather from MAINTAINERS, is maintainer for this file. > > Thanks, > > Kyrill > > > > > > > > gcc/ChangeLog: > > > > > > 2020-04-XX Szabolcs Nagy <szabolcs.nagy@arm.com> > > > > > > PR target/94515 > > > * dwarf2cfi.c (dwarf2out_frame_debug): Record RA > > > mangle state in cur_row->window_save. > > > > > > gcc/testsuite/ChangeLog: > > > > > > 2020-04-XX Szabolcs Nagy <szabolcs.nagy@arm.com> > > > > > > PR target/94515 > > > * g++.target/aarch64/pr94515.C: New test. > > > --- > > > gcc/dwarf2cfi.c | 3 ++ > > > gcc/testsuite/g++.target/aarch64/pr94515.C | 43 ++++++++++++++++++++++ > > > 2 files changed, 46 insertions(+) > > > create mode 100644 gcc/testsuite/g++.target/aarch64/pr94515.C > > > > > > diff --git a/gcc/dwarf2cfi.c b/gcc/dwarf2cfi.c > > > index 229fbfacc30..22989a6c2fb 100644 > > > --- a/gcc/dwarf2cfi.c > > > +++ b/gcc/dwarf2cfi.c > > > @@ -2145,6 +2145,9 @@ dwarf2out_frame_debug (rtx_insn *insn) > > > case REG_CFA_TOGGLE_RA_MANGLE: > > > /* This uses the same DWARF opcode as the next operation. */ > > > dwarf2out_frame_debug_cfa_window_save (true); > > > + /* Keep track of RA mangle state by toggling the window_save bit. > > > + This is needed so .cfi_window_save is emitted correctly. */ > > > + cur_row->window_save = !cur_row->window_save; > > It looks like passing 'true' to dwarf2out_frame_debug_cfa_window_save > prevents that function from messing with the window_safe flag. Does > changing the argument to 'false' get the behavior you want? we want the state = !state toggling. it might make more sense to do that in dwarf2out_frame_debug_cfa_window_save(true) or to inline that entire logic into the two places where it is used (instead of dispatching with a bool argument). for the bug fix i'd like a minimal change (so it can be backported), doing the fix in dwarf2out_frame_debug_cfa_window_save is fine with me, would you prefer that? > > > > handled_one = true; > > > break; > > > > > > diff --git a/gcc/testsuite/g++.target/aarch64/pr94515.C > > > b/gcc/testsuite/g++.target/aarch64/pr94515.C > > > new file mode 100644 > > > index 00000000000..7707c773a72 > > > --- /dev/null > > > +++ b/gcc/testsuite/g++.target/aarch64/pr94515.C > > > @@ -0,0 +1,43 @@ > > > +/* PR target/94515. Check .cfi_window_save with multiple return paths. */ > > > +/* { dg-do run } */ > > > +/* { dg-additional-options "-O2 -mbranch-protection=pac-ret --save-temps" } > > > */ > > > + > > > +volatile int zero = 0; > > > + > > > +__attribute__((noinline, target("branch-protection=none"))) > > > +void unwind (void) > > > +{ > > > + if (zero == 0) > > > + throw 42; > > > +} > > > + > > > +__attribute__((noinline, noipa, target("branch-protection=pac-ret"))) > > > +int test (int z) > > > +{ > > > + if (z) { > > > + asm volatile ("":::"x20","x21"); > > > + unwind (); > > > + return 1; > > > + } else { > > > + unwind (); > > > + return 2; > > > + } > > > +} > > > + > > > +__attribute__((target("branch-protection=none"))) > > > +int main () > > > +{ > > > + try { > > > + test (zero); > > > + __builtin_abort (); > > > + } catch (...) { > > > + return 0; > > > + } > > > + __builtin_abort (); > > > +} > > > + > > > +/* This check only works if there are two return paths in test and > > > + cfi_window_save is used for both instead of cfi_remember_state > > > + plus cfi_restore_state. This is currently the case with -O2. */ > > > + > > > +/* { dg-final { scan-assembler-times {\t\.cfi_window_save\n} 4 } } */ > > > -- > > > 2.17.1 > > > --
On 4/17/20 1:55 PM, Szabolcs Nagy wrote: > The 04/17/2020 12:50, Jason Merrill wrote: >> On 4/17/20 6:08 AM, Kyrylo Tkachov wrote: >>> Hi Szabolcs, >>> >>>> -----Original Message----- >>>> From: Szabolcs Nagy <Szabolcs.Nagy@arm.com> >>>> Sent: 09 April 2020 15:20 >>>> To: gcc-patches@gcc.gnu.org >>>> Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>; Richard Sandiford >>>> <Richard.Sandiford@arm.com>; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> >>>> Subject: [PATCH] aarch64: Fix .cfi_window_save with pac-ret [PR94515] >>>> >>>> On aarch64 -mbranch-protection=pac-ret reuses the dwarf >>>> opcode for window_save to mean "toggle the return address >>>> mangle state", but in the dwarf2cfi internal logic the >>>> state was not properly recorded so the remember/restore >>>> logic was broken. >>>> >>>> This can cause the unwinder not to authenticate return >>>> addresses that were signed (or vice versa) which means >>>> a runtime crash on a pauth enabled system. >>>> >>>> Currently only aarch64 pac-ret uses REG_CFA_TOGGLE_RA_MANGLE. >>> >>> I think this is ok, but this code is in the midend so I've CC'ed Jason who, from what I gather from MAINTAINERS, is maintainer for this file. >>> Thanks, >>> Kyrill >>> >>>> >>>> gcc/ChangeLog: >>>> >>>> 2020-04-XX Szabolcs Nagy <szabolcs.nagy@arm.com> >>>> >>>> PR target/94515 >>>> * dwarf2cfi.c (dwarf2out_frame_debug): Record RA >>>> mangle state in cur_row->window_save. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> 2020-04-XX Szabolcs Nagy <szabolcs.nagy@arm.com> >>>> >>>> PR target/94515 >>>> * g++.target/aarch64/pr94515.C: New test. >>>> --- >>>> gcc/dwarf2cfi.c | 3 ++ >>>> gcc/testsuite/g++.target/aarch64/pr94515.C | 43 ++++++++++++++++++++++ >>>> 2 files changed, 46 insertions(+) >>>> create mode 100644 gcc/testsuite/g++.target/aarch64/pr94515.C >>>> >>>> diff --git a/gcc/dwarf2cfi.c b/gcc/dwarf2cfi.c >>>> index 229fbfacc30..22989a6c2fb 100644 >>>> --- a/gcc/dwarf2cfi.c >>>> +++ b/gcc/dwarf2cfi.c >>>> @@ -2145,6 +2145,9 @@ dwarf2out_frame_debug (rtx_insn *insn) >>>> case REG_CFA_TOGGLE_RA_MANGLE: >>>> /* This uses the same DWARF opcode as the next operation. */ >>>> dwarf2out_frame_debug_cfa_window_save (true); >>>> + /* Keep track of RA mangle state by toggling the window_save bit. >>>> + This is needed so .cfi_window_save is emitted correctly. */ >>>> + cur_row->window_save = !cur_row->window_save; >> >> It looks like passing 'true' to dwarf2out_frame_debug_cfa_window_save >> prevents that function from messing with the window_safe flag. Does >> changing the argument to 'false' get the behavior you want? > > we want the state = !state toggling. > it might make more sense to do that in > dwarf2out_frame_debug_cfa_window_save(true) > or to inline that entire logic into the two > places where it is used (instead of > dispatching with a bool argument). I think that inlining and dropping the parameter would be cleaner. > for the bug fix i'd like a minimal change > (so it can be backported), doing the fix in > dwarf2out_frame_debug_cfa_window_save > is fine with me, would you prefer that? No, thanks. If you want to commit your patch as is for backporting and then do the inlining in a separate commit, that works for me. >>>> handled_one = true; >>>> break; >>>> >>>> diff --git a/gcc/testsuite/g++.target/aarch64/pr94515.C >>>> b/gcc/testsuite/g++.target/aarch64/pr94515.C >>>> new file mode 100644 >>>> index 00000000000..7707c773a72 >>>> --- /dev/null >>>> +++ b/gcc/testsuite/g++.target/aarch64/pr94515.C >>>> @@ -0,0 +1,43 @@ >>>> +/* PR target/94515. Check .cfi_window_save with multiple return paths. */ >>>> +/* { dg-do run } */ >>>> +/* { dg-additional-options "-O2 -mbranch-protection=pac-ret --save-temps" } >>>> */ >>>> + >>>> +volatile int zero = 0; >>>> + >>>> +__attribute__((noinline, target("branch-protection=none"))) >>>> +void unwind (void) >>>> +{ >>>> + if (zero == 0) >>>> + throw 42; >>>> +} >>>> + >>>> +__attribute__((noinline, noipa, target("branch-protection=pac-ret"))) >>>> +int test (int z) >>>> +{ >>>> + if (z) { >>>> + asm volatile ("":::"x20","x21"); >>>> + unwind (); >>>> + return 1; >>>> + } else { >>>> + unwind (); >>>> + return 2; >>>> + } >>>> +} >>>> + >>>> +__attribute__((target("branch-protection=none"))) >>>> +int main () >>>> +{ >>>> + try { >>>> + test (zero); >>>> + __builtin_abort (); >>>> + } catch (...) { >>>> + return 0; >>>> + } >>>> + __builtin_abort (); >>>> +} >>>> + >>>> +/* This check only works if there are two return paths in test and >>>> + cfi_window_save is used for both instead of cfi_remember_state >>>> + plus cfi_restore_state. This is currently the case with -O2. */ >>>> + >>>> +/* { dg-final { scan-assembler-times {\t\.cfi_window_save\n} 4 } } */ >>>> -- >>>> 2.17.1 >>> >> >
The 04/17/2020 15:26, Jason Merrill wrote: > On 4/17/20 1:55 PM, Szabolcs Nagy wrote: > > The 04/17/2020 12:50, Jason Merrill wrote: > > > On 4/17/20 6:08 AM, Kyrylo Tkachov wrote: > > > > Hi Szabolcs, > > > > > > > > > -----Original Message----- > > > > > From: Szabolcs Nagy <Szabolcs.Nagy@arm.com> > > > > > Sent: 09 April 2020 15:20 > > > > > To: gcc-patches@gcc.gnu.org > > > > > Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>; Richard Sandiford > > > > > <Richard.Sandiford@arm.com>; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > > > > > Subject: [PATCH] aarch64: Fix .cfi_window_save with pac-ret [PR94515] > > > > > > > > > > On aarch64 -mbranch-protection=pac-ret reuses the dwarf > > > > > opcode for window_save to mean "toggle the return address > > > > > mangle state", but in the dwarf2cfi internal logic the > > > > > state was not properly recorded so the remember/restore > > > > > logic was broken. > > > > > > > > > > This can cause the unwinder not to authenticate return > > > > > addresses that were signed (or vice versa) which means > > > > > a runtime crash on a pauth enabled system. > > > > > > > > > > Currently only aarch64 pac-ret uses REG_CFA_TOGGLE_RA_MANGLE. > > > > > > > > I think this is ok, but this code is in the midend so I've CC'ed Jason who, from what I gather from MAINTAINERS, is maintainer for this file. > > > > Thanks, > > > > Kyrill > > > > > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > 2020-04-XX Szabolcs Nagy <szabolcs.nagy@arm.com> > > > > > > > > > > PR target/94515 > > > > > * dwarf2cfi.c (dwarf2out_frame_debug): Record RA > > > > > mangle state in cur_row->window_save. > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > 2020-04-XX Szabolcs Nagy <szabolcs.nagy@arm.com> > > > > > > > > > > PR target/94515 > > > > > * g++.target/aarch64/pr94515.C: New test. > > > > > --- > > > > > gcc/dwarf2cfi.c | 3 ++ > > > > > gcc/testsuite/g++.target/aarch64/pr94515.C | 43 ++++++++++++++++++++++ > > > > > 2 files changed, 46 insertions(+) > > > > > create mode 100644 gcc/testsuite/g++.target/aarch64/pr94515.C > > > > > > > > > > diff --git a/gcc/dwarf2cfi.c b/gcc/dwarf2cfi.c > > > > > index 229fbfacc30..22989a6c2fb 100644 > > > > > --- a/gcc/dwarf2cfi.c > > > > > +++ b/gcc/dwarf2cfi.c > > > > > @@ -2145,6 +2145,9 @@ dwarf2out_frame_debug (rtx_insn *insn) > > > > > case REG_CFA_TOGGLE_RA_MANGLE: > > > > > /* This uses the same DWARF opcode as the next operation. */ > > > > > dwarf2out_frame_debug_cfa_window_save (true); > > > > > + /* Keep track of RA mangle state by toggling the window_save bit. > > > > > + This is needed so .cfi_window_save is emitted correctly. */ > > > > > + cur_row->window_save = !cur_row->window_save; > > > > > > It looks like passing 'true' to dwarf2out_frame_debug_cfa_window_save > > > prevents that function from messing with the window_safe flag. Does > > > changing the argument to 'false' get the behavior you want? > > > > we want the state = !state toggling. > > it might make more sense to do that in > > dwarf2out_frame_debug_cfa_window_save(true) > > or to inline that entire logic into the two > > places where it is used (instead of > > dispatching with a bool argument). > > I think that inlining and dropping the parameter would be cleaner. > > > for the bug fix i'd like a minimal change > > (so it can be backported), doing the fix in > > dwarf2out_frame_debug_cfa_window_save > > is fine with me, would you prefer that? > > No, thanks. If you want to commit your patch as is for backporting and then > do the inlining in a separate commit, that works for me. i spoted failing execution tests that expected to pass, it turns out change_cfi_row() needs something like - if (!old_row->window_save && new_row->window_save) + if (old_row->window_save != new_row->window_save) to generate .cfi_window_save that correctly tracks the toggled state on aarch64, but i assume sparc wants something else, i added Eric to CC he might know what's right for the old&&!new case on sparc, any help is welcome, the logic was added in commit dfe1fe91dbc7f068bb3efcee40237caacc0c53ae i can imagine adding a new bool flag in dw_cfi_row for aarch64 or making the logic target specific (depending on if the target uses REG_CFA_WINDOW_SAVE or REG_CFA_TOGGLE_RA_MANGLE for cfi_window_save) i added a test to the bug report that fails with my original patch, but works if change_cfi_row is updated.
> i spoted failing execution tests that expected to pass, > it turns out change_cfi_row() needs something like > > - if (!old_row->window_save && new_row->window_save) > + if (old_row->window_save != new_row->window_save) > > to generate .cfi_window_save that correctly tracks the > toggled state on aarch64, but i assume sparc wants > something else, i added Eric to CC he might know what's > right for the old&&!new case on sparc, any help is > welcome, the logic was added in > > commit dfe1fe91dbc7f068bb3efcee40237caacc0c53ae Yes, you cannot really "toggle" a register window on SPARC. > i can imagine adding a new bool flag in dw_cfi_row > for aarch64 or making the logic target specific > (depending on if the target uses REG_CFA_WINDOW_SAVE > or REG_CFA_TOGGLE_RA_MANGLE for cfi_window_save) Yes, please do not hijack again the SPARC logic here.
diff --git a/gcc/dwarf2cfi.c b/gcc/dwarf2cfi.c index 229fbfacc30..22989a6c2fb 100644 --- a/gcc/dwarf2cfi.c +++ b/gcc/dwarf2cfi.c @@ -2145,6 +2145,9 @@ dwarf2out_frame_debug (rtx_insn *insn) case REG_CFA_TOGGLE_RA_MANGLE: /* This uses the same DWARF opcode as the next operation. */ dwarf2out_frame_debug_cfa_window_save (true); + /* Keep track of RA mangle state by toggling the window_save bit. + This is needed so .cfi_window_save is emitted correctly. */ + cur_row->window_save = !cur_row->window_save; handled_one = true; break; diff --git a/gcc/testsuite/g++.target/aarch64/pr94515.C b/gcc/testsuite/g++.target/aarch64/pr94515.C new file mode 100644 index 00000000000..7707c773a72 --- /dev/null +++ b/gcc/testsuite/g++.target/aarch64/pr94515.C @@ -0,0 +1,43 @@ +/* PR target/94515. Check .cfi_window_save with multiple return paths. */ +/* { dg-do run } */ +/* { dg-additional-options "-O2 -mbranch-protection=pac-ret --save-temps" } */ + +volatile int zero = 0; + +__attribute__((noinline, target("branch-protection=none"))) +void unwind (void) +{ + if (zero == 0) + throw 42; +} + +__attribute__((noinline, noipa, target("branch-protection=pac-ret"))) +int test (int z) +{ + if (z) { + asm volatile ("":::"x20","x21"); + unwind (); + return 1; + } else { + unwind (); + return 2; + } +} + +__attribute__((target("branch-protection=none"))) +int main () +{ + try { + test (zero); + __builtin_abort (); + } catch (...) { + return 0; + } + __builtin_abort (); +} + +/* This check only works if there are two return paths in test and + cfi_window_save is used for both instead of cfi_remember_state + plus cfi_restore_state. This is currently the case with -O2. */ + +/* { dg-final { scan-assembler-times {\t\.cfi_window_save\n} 4 } } */