[Bug target/114843] aarch64: epilogue in _Unwind_RaiseException corrupts return value due to __builtin_eh_return
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114843 --- Comment #20 from Andrew Pinski --- New patch posted: https://gcc.gnu.org/pipermail/gcc-patches/2024-May/650757.html It was simplier than I had expected too.
[Bug target/114843] aarch64: epilogue in _Unwind_RaiseException corrupts return value due to __builtin_eh_return
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114843 Andrew Pinski changed: What|Removed |Added Attachment #58043|0 |1 is obsolete|| --- Comment #19 from Andrew Pinski --- Created attachment 58052 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=58052=edit v2 of the patch I am attaching this here just so I don't lose it; it includes the fix for the case mentioned. I am not going to post this version to the list. I will be doing a v3 which I will post which contains the rewrite part.
[Bug target/114843] aarch64: epilogue in _Unwind_RaiseException corrupts return value due to __builtin_eh_return
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114843 --- Comment #18 from Andrew Pinski --- (In reply to Wilco from comment #17) > So I don't believe you should change aarch64_pop_regs at all - it's too late > to change things and just adds unnecessary complexity and more bugs. The > best option would be to handle eh_return explicitly and insert the extra > push/pops rather than treating them like a generic callee-save (because > clearly they are not anymore). Thinking about this I think you are right, I will work on an incremental patch to change them to be in seperate array. Note I do have a fix for the issue in comment #17, I had a premature optimization thinking only if regno1 was an eh_handler data register, then regno2 could be one; rather they should be checked independently. I will attach that patch to this bug once my testing is finished. And then will work on splitting out the eh_handler data register load/stores.
[Bug target/114843] aarch64: epilogue in _Unwind_RaiseException corrupts return value due to __builtin_eh_return
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114843
--- Comment #17 from Wilco ---
(In reply to Andrew Pinski from comment #16)
> Patch posted with all of the testcases included:
> https://gcc.gnu.org/pipermail/gcc-patches/2024-April/650080.html
Not nearly enough testcases... What about:
void g(void);
int f(long offset, void *handler)
{
g();
if (offset > 5)
return arr[offset];
__builtin_eh_return (offset, handler);
}
With -O2 -fomit-frame-pointer:
f:
.LFB0:
.cfi_startproc
stp x30, x0, [sp, -64]!
.cfi_def_cfa_offset 64
.cfi_offset 30, -64
.cfi_offset 0, -56
stp x1, x2, [sp, 16]
stp x3, x19, [sp, 32]
.cfi_offset 1, -48
.cfi_offset 2, -40
.cfi_offset 3, -32
.cfi_offset 19, -24
mov x19, x0
str x20, [sp, 48]
.cfi_offset 20, -16
mov x20, x1
bl g
cmp x19, 5
ble .L8
mov w0, w19
ldp x19, x20, [sp, 40]
ldp x30, x0, [sp], 64** oops
.cfi_remember_state
.cfi_restore 0
.cfi_restore 30
.cfi_restore 19
.cfi_restore 20
.cfi_def_cfa_offset 0
ret
.L8:
.cfi_restore_state
mov x5, x19
ldp x1, x2, [sp, 16]
mov x6, x20
ldp x3, x19, [sp, 32]
ldr x20, [sp, 48]
ldp x30, x0, [sp], 64
.cfi_restore 0
.cfi_restore 30
.cfi_restore 20
.cfi_restore 3
.cfi_restore 19
.cfi_restore 1
.cfi_restore 2
.cfi_def_cfa_offset 0
add sp, sp, x5
br x6
.cfi_endproc
So I don't believe you should change aarch64_pop_regs at all - it's too late to
change things and just adds unnecessary complexity and more bugs. The best
option would be to handle eh_return explicitly and insert the extra push/pops
rather than treating them like a generic callee-save (because clearly they are
not anymore).
[Bug target/114843] aarch64: epilogue in _Unwind_RaiseException corrupts return value due to __builtin_eh_return
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114843 --- Comment #16 from Andrew Pinski --- Patch posted with all of the testcases included: https://gcc.gnu.org/pipermail/gcc-patches/2024-April/650080.html
[Bug target/114843] aarch64: epilogue in _Unwind_RaiseException corrupts return value due to __builtin_eh_return
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114843 --- Comment #15 from Andrew Pinski --- Created attachment 58043 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=58043=edit Patch but no testcases yet Will add testcases in a little bit. Also I have not tested this fully yet. Will do tonight.
[Bug target/114843] aarch64: epilogue in _Unwind_RaiseException corrupts return value due to __builtin_eh_return
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114843 Andrew Pinski changed: What|Removed |Added See Also||https://github.com/gnustep/ ||libobjc2/issues/247 --- Comment #14 from Andrew Pinski --- I tested alloca and large stack sizes, they both look good to me. I will add runtime testcase for both. not omitting FP also looks correct. The patch itself is most contained in aarch64_pop_regs/aarch64_restore_callee_saves which just skips the restore if the regno was eh_return_data_regno and returning normally. > no tests tried to return a value on the non-exception path well libobjc2 does and that is how this was originally root caused even: https://github.com/gnustep/libobjc2/issues/247
[Bug target/114843] aarch64: epilogue in _Unwind_RaiseException corrupts return value due to __builtin_eh_return
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114843 --- Comment #13 from Wilco --- (In reply to Andrew Pinski from comment #11) > I have a fix for aarch64, able to produce now: > ``` > f: > .LFB0: > .cfi_startproc > stp x0, x1, [sp, -32]! > .cfi_def_cfa_offset 32 > .cfi_offset 0, -32 > .cfi_offset 1, -24 > stp x2, x3, [sp, 16] > .cfi_offset 2, -16 > .cfi_offset 3, -8 > ldr w0, [x0] > cmp w0, 5 > bne .L8 > add sp, sp, 32 > .cfi_remember_state > .cfi_def_cfa_offset 0 > ret > .L8: > .cfi_restore_state > mov x5, x1 > ldp x2, x3, [sp, 16] > ldp x0, x1, [sp], 32 > .cfi_restore 1 > .cfi_restore 0 > .cfi_restore 2 > .cfi_restore 3 > .cfi_def_cfa_offset 0 > add sp, sp, x5 > ret > .cfi_endproc > ``` > > Which is exactly what we should produce I think. > The patch is a bit more complex than I expected but that is due to how > aarch64 has some of the most complex epilogues. I'm not convinced that is an easy solution. Try various cases with large stack sizes, alloca and other scalar and FP callee-saves. Getting all cases right and writing good tests for them is a lot of work.
[Bug target/114843] aarch64: epilogue in _Unwind_RaiseException corrupts return value due to __builtin_eh_return
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114843 --- Comment #12 from Andrew Pinski --- (In reply to Wilco from comment #10) > Since the whole eh_return is an internal ABI in libgcc, a fix would be to > change EH_RETURN_DATA_REGNO(N) to avoid x0 and x1. Since eh_return already > reserves 7 registers(!) and now need to avoid using x0/x1 too, using x2-x5 > and x6,x7 and x9 for the other special registers should work. You can't change EH_RETURN_DATA_REGNO now because it is not just internal ABI to libgcc, it is part of libstdc++v3 too (libsupc++/eh_personality.cc): /* For targets with pointers smaller than the word size, we must extend the pointer, and this extension is target dependent. */ _Unwind_SetGR (context, __builtin_eh_return_data_regno (0), __builtin_extend_pointer (ue_header)); _Unwind_SetGR (context, __builtin_eh_return_data_regno (1), handler_switch_value); And libobjc: exception.c:#define __builtin_eh_return_data_regno(x) x exception.c: _Unwind_SetGR (context, __builtin_eh_return_data_regno (0), exception.c: _Unwind_SetGR (context, __builtin_eh_return_data_regno (1), And really any exception personality.
[Bug target/114843] aarch64: epilogue in _Unwind_RaiseException corrupts return value due to __builtin_eh_return
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114843 Andrew Pinski changed: What|Removed |Added Status|NEW |ASSIGNED Assignee|unassigned at gcc dot gnu.org |pinskia at gcc dot gnu.org --- Comment #11 from Andrew Pinski --- I have a fix for aarch64, able to produce now: ``` f: .LFB0: .cfi_startproc stp x0, x1, [sp, -32]! .cfi_def_cfa_offset 32 .cfi_offset 0, -32 .cfi_offset 1, -24 stp x2, x3, [sp, 16] .cfi_offset 2, -16 .cfi_offset 3, -8 ldr w0, [x0] cmp w0, 5 bne .L8 add sp, sp, 32 .cfi_remember_state .cfi_def_cfa_offset 0 ret .L8: .cfi_restore_state mov x5, x1 ldp x2, x3, [sp, 16] ldp x0, x1, [sp], 32 .cfi_restore 1 .cfi_restore 0 .cfi_restore 2 .cfi_restore 3 .cfi_def_cfa_offset 0 add sp, sp, x5 ret .cfi_endproc ``` Which is exactly what we should produce I think. The patch is a bit more complex than I expected but that is due to how aarch64 has some of the most complex epilogues.
[Bug target/114843] aarch64: epilogue in _Unwind_RaiseException corrupts return value due to __builtin_eh_return
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114843 Wilco changed: What|Removed |Added CC||wilco at gcc dot gnu.org --- Comment #10 from Wilco --- (In reply to Andrew Pinski from comment #9) > Just a quick note here. Even though eh_return pattern was removed with > r7-6051-g8144a493ddc008, it was broken before that patch. Yeah I only fixed the broken behaviours that I encountered at the time - no tests tried to return a value on the non-exception path. There is no clear specification (eg. making it clear that EH_RETURN_DATA_REGNO must not overlap with registers used to return or if they do, you need to conditionally restore them), so no wonder that many targets get this wrong. Who knew that introducing lots of complex builtins that affect prolog and epilog generation in a major way to avoid a few lines of assembly code was such a bad idea... Since the whole eh_return is an internal ABI in libgcc, a fix would be to change EH_RETURN_DATA_REGNO(N) to avoid x0 and x1. Since eh_return already reserves 7 registers(!) and now need to avoid using x0/x1 too, using x2-x5 and x6,x7 and x9 for the other special registers should work.
[Bug target/114843] aarch64: epilogue in _Unwind_RaiseException corrupts return value due to __builtin_eh_return
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114843 Andrew Pinski changed: What|Removed |Added See Also||https://gcc.gnu.org/bugzill ||a/show_bug.cgi?id=77455 --- Comment #9 from Andrew Pinski --- Just a quick note here. Even though eh_return pattern was removed with r7-6051-g8144a493ddc008, it was broken before that patch.
[Bug target/114843] aarch64: epilogue in _Unwind_RaiseException corrupts return value due to __builtin_eh_return
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114843 --- Comment #8 from Andrew Pinski --- (In reply to Andrew Pinski from comment #7) > Just an FYI on other targets on my reduced testcase (I just quickly looked > at the generated assembly to see if it worked or not): > > backends that seems to work: > mips > riscv > x86 > s390 > m68k > sh > sparc > > backends that don't work: > powerpc: PR 114846 > arm: PR 114847 > longarch: PR 114848 Looks like longarch was fixed in GCC 14: r14-6440 . Most likely other targets should do something similar ...
