On 5/28/19 11:37 AM, Wilco Dijkstra wrote: > This fixes and simplifies the setjmp and non-local goto implementation. > Currently the virtual frame pointer is saved when using __builtin_setjmp or > a non-local goto. Depending on whether a frame pointer is used, this may > either save SP or FP with an immediate offset. However the goto or longjmp > always updates the hard frame pointer. > > A receiver veneer in the original function then assigns the hard frame pointer > to the virtual frame pointer, which should, if it works correctly, again > assign > SP or FP. However the special elimination code in eliminate_regs_in_insn > doesn't do this correctly unless the frame pointer is used, and even if it > worked by writing SP, the frame pointer would still be corrupted. > > A much simpler implementation is to always save and restore the hard frame > pointer. This avoids 2 redundant instructions which add/subtract the virtual > frame offset. A large amount of code can be removed as a result, including > all > implementations of TARGET_BUILTIN_SETJMP_FRAME_VALUE (all of which already use > the hard frame pointer). The expansion of nonlocal_goto on PA can be simplied > to just restore the hard frame pointer. > > This fixes the most obvious issues, however there are still issues on targets > which define HARD_FRAME_POINTER_IS_FRAME_POINTER (arm, mips, xtensa). > Each function could have a different hard frame pointer, so a non-local goto > may restore the wrong frame pointer (TARGET_BUILTIN_SETJMP_FRAME_VALUE could > be useful for this). > > The i386 TARGET_BUILTIN_SETJMP_FRAME_VALUE was incorrect: if stack_realign_fp > is true, it would save the hard frame pointer value but restore the virtual > frame pointer which according to ix86_initial_elimination_offset can have a > non-zero offset from the hard frame pointer. > > The ia64 implementation of nonlocal_goto seems incorrect since the helper > function moves the the frame pointer value into the static chain register > (so this patch does nothing to make it better or worse). > > AArch64 bootstrap OK, new test passes on AArch64, x86-64 and Arm. > > ChangeLog: > 2018-12-13 Wilco Dijkstra <wdijk...@arm.com> > > gcc/ > PR middle-end/84521 > * builtins.c (expand_builtin_setjmp_setup): Save hard_frame_pointer_rtx. > (expand_builtin_setjmp_receiver): Do not emit sfp = fp move since we > restore fp. > * function.c (expand_function_start): Save hard_frame_pointer_rtx for > non-local goto. > * lra-eliminations.c (eliminate_regs_in_insn): Remove sfp = fp > elimination code. > (remove_reg_equal_offset_note): Remove unused function. > * reload1.c (eliminate_regs_in_insn): Remove sfp = hfp elimination code. > * config/arc/arc.c (TARGET_BUILTIN_SETJMP_FRAME_VALUE): Remove. > (arc_builtin_setjmp_frame_value): Remove function. > * config/avr/avr.c (TARGET_BUILTIN_SETJMP_FRAME_VALUE): Remove. > (avr_builtin_setjmp_frame_value): Remove function. > * config/i386/i386.c (TARGET_BUILTIN_SETJMP_FRAME_VALUE): Remove. > (ix86_builtin_setjmp_frame_value): Remove function. > * config/pa/pa.md (nonlocal_goto): Remove FP adjustment. > * config/sparc/sparc.c (TARGET_BUILTIN_SETJMP_FRAME_VALUE): Remove. > (sparc_builtin_setjmp_frame_value): Remove function. > * config/vax/vax.c (TARGET_BUILTIN_SETJMP_FRAME_VALUE): Remove. > (vax_builtin_setjmp_frame_value): Remove function. > > testsuite/ > PR middle-end/84521 > * gcc.c-torture/execute/pr84521.c: New test. > So I like the significant simplification here. My worry is whether or not this is, in effect, an ABI change. ie, would we be able to mix and match .o files from before/after this change which used the builtin setjmp/longjmp bits?
You mention that arm, mips and xtensa are still broken. Are they worse after this patch? Presumably for arm/mips the problem is the frame pointer varies based on the thumb/mips and mips/mips16 state? Is it even valid to longjmp from one mode to the other? I think xtensa has two abis and the frame pointer is different across them. Presumably a longjmp from one abi to the other isn't valid. Or am I missing something? jeff
