* [PATCH] Fix PR84521
@ 2018-12-14 13:17 Wilco Dijkstra
2019-01-10 13:03 ` Wilco Dijkstra
0 siblings, 1 reply; 15+ messages in thread
From: Wilco Dijkstra @ 2018-12-14 13:17 UTC (permalink / raw)
To: GCC Patches; +Cc: nd
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 <wdijkstr@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 = fp 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.
---
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 2ef9c9afcc69fcb775dc6a6fff550025bdc76337..55b78adbc3df8c970083e6d9b548a8ca7dc52600 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -982,7 +982,7 @@ expand_builtin_setjmp_setup (rtx buf_addr, rtx receiver_label)
mem = gen_rtx_MEM (Pmode, buf_addr);
set_mem_alias_set (mem, setjmp_alias_set);
- emit_move_insn (mem, targetm.builtin_setjmp_frame_value ());
+ emit_move_insn (mem, hard_frame_pointer_rtx);
mem = gen_rtx_MEM (Pmode, plus_constant (Pmode, buf_addr,
GET_MODE_SIZE (Pmode))),
@@ -1024,31 +1024,6 @@ expand_builtin_setjmp_receiver (rtx receiver_label)
if (chain && REG_P (chain))
emit_clobber (chain);
- /* Now put in the code to restore the frame pointer, and argument
- pointer, if needed. */
- if (! targetm.have_nonlocal_goto ())
- {
- /* First adjust our frame pointer to its actual value. It was
- previously set to the start of the virtual area corresponding to
- the stacked variables when we branched here and now needs to be
- adjusted to the actual hardware fp value.
-
- Assignments to virtual registers are converted by
- instantiate_virtual_regs into the corresponding assignment
- to the underlying register (fp in this case) that makes
- the original assignment true.
- So the following insn will actually be decrementing fp by
- TARGET_STARTING_FRAME_OFFSET. */
- emit_move_insn (virtual_stack_vars_rtx, hard_frame_pointer_rtx);
-
- /* Restoring the frame pointer also modifies the hard frame pointer.
- Mark it used (so that the previous assignment remains live once
- the frame pointer is eliminated) and clobbered (to represent the
- implicit update from the assignment). */
- emit_use (hard_frame_pointer_rtx);
- emit_clobber (hard_frame_pointer_rtx);
- }
-
if (!HARD_FRAME_POINTER_IS_ARG_POINTER && fixed_regs[ARG_POINTER_REGNUM])
{
/* If the argument pointer can be eliminated in favor of the
diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c
index 55175215bfed6e166a92ae046b36487662c36dd0..9058729a76e7f85f188ed0323190549ddd886fa9 100644
--- a/gcc/config/arc/arc.c
+++ b/gcc/config/arc/arc.c
@@ -689,8 +689,6 @@ static rtx arc_legitimize_address_0 (rtx, rtx, machine_mode mode);
#undef TARGET_MODES_TIEABLE_P
#define TARGET_MODES_TIEABLE_P arc_modes_tieable_p
-#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
-#define TARGET_BUILTIN_SETJMP_FRAME_VALUE arc_builtin_setjmp_frame_value
/* Try to keep the (mov:DF _, reg) as early as possible so
that the d<add/sub/mul>h-lr insns appear together and can
@@ -10964,28 +10962,6 @@ compact_memory_operand_p (rtx op, machine_mode mode,
return false;
}
-/* Return the frame pointer value to be backed up in the setjmp buffer. */
-
-static rtx
-arc_builtin_setjmp_frame_value (void)
-{
- /* We always want to preserve whatever value is currently in the frame
- pointer register. For frames that are using the frame pointer the new
- value of the frame pointer register will have already been computed
- (as part of the prologue). For frames that are not using the frame
- pointer it is important that we backup whatever value is in the frame
- pointer register, as earlier (more outer) frames may have placed a
- value into the frame pointer register. It might be tempting to try
- and use `frame_pointer_rtx` here, however, this is not what we want.
- For frames that are using the frame pointer this will give the
- correct value. However, for frames that are not using the frame
- pointer this will still give the value that _would_ have been the
- frame pointer value for this frame (if the use of the frame pointer
- had not been removed). We really do want the raw frame pointer
- register value. */
- return gen_raw_REG (Pmode, FRAME_POINTER_REGNUM);
-}
-
/* Return nonzero if a jli call should be generated for a call from
the current function to DECL. */
diff --git a/gcc/config/avr/avr.c b/gcc/config/avr/avr.c
index 81c35e7fbc2b06bc06cc7fb7406694bbe48860e5..4fdc38649eb9dc88d33b17f64fe09ffe22f57d1c 100644
--- a/gcc/config/avr/avr.c
+++ b/gcc/config/avr/avr.c
@@ -1302,22 +1302,6 @@ avr_build_builtin_va_list (void)
}
-/* Implement `TARGET_BUILTIN_SETJMP_FRAME_VALUE'. */
-/* Actual start of frame is virtual_stack_vars_rtx this is offset from
- frame pointer by +TARGET_STARTING_FRAME_OFFSET.
- Using saved frame = virtual_stack_vars_rtx - TARGET_STARTING_FRAME_OFFSET
- avoids creating add/sub of offset in nonlocal goto and setjmp. */
-
-static rtx
-avr_builtin_setjmp_frame_value (void)
-{
- rtx xval = gen_reg_rtx (Pmode);
- emit_insn (gen_subhi3 (xval, virtual_stack_vars_rtx,
- gen_int_mode (avr_starting_frame_offset (), Pmode)));
- return xval;
-}
-
-
/* Return contents of MEM at frame pointer + stack size + 1 (+2 if 3-byte PC).
This is return address of function. */
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 1d7b1f0d1b28a7a9368391cef42fae7b6e7fd160..80f92d684ba705de48f3fafa5f4ccb7b937546c2 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -11108,17 +11108,6 @@ ix86_initial_elimination_offset (int from, int to)
}
}
-/* In a dynamically-aligned function, we can't know the offset from
- stack pointer to frame pointer, so we must ensure that setjmp
- eliminates fp against the hard fp (%ebp) rather than trying to
- index from %esp up to the top of the frame across a gap that is
- of unknown (at compile-time) size. */
-static rtx
-ix86_builtin_setjmp_frame_value (void)
-{
- return stack_realign_fp ? hard_frame_pointer_rtx : virtual_stack_vars_rtx;
-}
-
/* Emits a warning for unsupported msabi to sysv pro/epilogues. */
static void warn_once_call_ms2sysv_xlogues (const char *feature)
{
@@ -50934,9 +50923,6 @@ ix86_run_selftests (void)
#undef TARGET_MACHINE_DEPENDENT_REORG
#define TARGET_MACHINE_DEPENDENT_REORG ix86_reorg
-#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
-#define TARGET_BUILTIN_SETJMP_FRAME_VALUE ix86_builtin_setjmp_frame_value
-
#undef TARGET_BUILD_BUILTIN_VA_LIST
#define TARGET_BUILD_BUILTIN_VA_LIST ix86_build_builtin_va_list
diff --git a/gcc/config/pa/pa.md b/gcc/config/pa/pa.md
index 77611503c67732fb71114e7f1d8e7c1a8883ef41..2fcd597d572117f928c618082d9a61a1463ea6c2 100644
--- a/gcc/config/pa/pa.md
+++ b/gcc/config/pa/pa.md
@@ -6909,13 +6909,7 @@ (define_expand "nonlocal_goto"
emit_clobber (gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (VOIDmode)));
emit_clobber (gen_rtx_MEM (BLKmode, hard_frame_pointer_rtx));
- /* Restore the frame pointer. The virtual_stack_vars_rtx is saved
- instead of the hard_frame_pointer_rtx in the save area. As a
- result, an extra instruction is needed to adjust for the offset
- of the virtual stack variables and the hard frame pointer. */
- if (GET_CODE (fp) != REG)
- fp = force_reg (Pmode, fp);
- emit_move_insn (hard_frame_pointer_rtx, plus_constant (Pmode, fp, -8));
+ emit_move_insn (hard_frame_pointer_rtx, fp);
emit_stack_restore (SAVE_NONLOCAL, stack);
diff --git a/gcc/config/sparc/sparc.c b/gcc/config/sparc/sparc.c
index adbef1ab18f532739cf3c0ba1ac80ae5ee288076..093807a8c851024d60d4b96fbb942a9a02e19e8e 100644
--- a/gcc/config/sparc/sparc.c
+++ b/gcc/config/sparc/sparc.c
@@ -679,7 +679,6 @@ static void sparc_output_dwarf_dtprel (FILE *, int, rtx) ATTRIBUTE_UNUSED;
static void sparc_file_end (void);
static bool sparc_frame_pointer_required (void);
static bool sparc_can_eliminate (const int, const int);
-static rtx sparc_builtin_setjmp_frame_value (void);
static void sparc_conditional_register_usage (void);
static bool sparc_use_pseudo_pic_reg (void);
static void sparc_init_pic_reg (void);
@@ -883,9 +882,6 @@ char sparc_hard_reg_printed[8];
#undef TARGET_FRAME_POINTER_REQUIRED
#define TARGET_FRAME_POINTER_REQUIRED sparc_frame_pointer_required
-#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
-#define TARGET_BUILTIN_SETJMP_FRAME_VALUE sparc_builtin_setjmp_frame_value
-
#undef TARGET_CAN_ELIMINATE
#define TARGET_CAN_ELIMINATE sparc_can_eliminate
@@ -12985,14 +12981,6 @@ sparc_can_eliminate (const int from ATTRIBUTE_UNUSED, const int to)
return to == HARD_FRAME_POINTER_REGNUM || !sparc_frame_pointer_required ();
}
-/* Return the hard frame pointer directly to bypass the stack bias. */
-
-static rtx
-sparc_builtin_setjmp_frame_value (void)
-{
- return hard_frame_pointer_rtx;
-}
-
/* If !TARGET_FPU, then make the fp registers and fp cc regs fixed so that
they won't be allocated. */
diff --git a/gcc/config/vax/vax.c b/gcc/config/vax/vax.c
index 631c598d3b2f26f250d83b370e3e5ddc7ade0919..d0594f023002a2cd7a01e99690eda1d0f7a5b160 100644
--- a/gcc/config/vax/vax.c
+++ b/gcc/config/vax/vax.c
@@ -59,7 +59,6 @@ static rtx vax_function_arg (cumulative_args_t, machine_mode,
static void vax_function_arg_advance (cumulative_args_t, machine_mode,
const_tree, bool);
static rtx vax_struct_value_rtx (tree, int);
-static rtx vax_builtin_setjmp_frame_value (void);
static void vax_asm_trampoline_template (FILE *);
static void vax_trampoline_init (rtx, tree, rtx);
static poly_int64 vax_return_pops_args (tree, tree, poly_int64);
@@ -99,9 +98,6 @@ static HOST_WIDE_INT vax_starting_frame_offset (void);
#undef TARGET_STRUCT_VALUE_RTX
#define TARGET_STRUCT_VALUE_RTX vax_struct_value_rtx
-#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
-#define TARGET_BUILTIN_SETJMP_FRAME_VALUE vax_builtin_setjmp_frame_value
-
#undef TARGET_LRA_P
#define TARGET_LRA_P hook_bool_void_false
@@ -1063,12 +1059,6 @@ vax_struct_value_rtx (tree fntype ATTRIBUTE_UNUSED,
return gen_rtx_REG (Pmode, VAX_STRUCT_VALUE_REGNUM);
}
-static rtx
-vax_builtin_setjmp_frame_value (void)
-{
- return hard_frame_pointer_rtx;
-}
-
/* Worker function for NOTICE_UPDATE_CC. */
void
diff --git a/gcc/function.c b/gcc/function.c
index 69523c1d72309b7ba0e7a0e6ccfadcd3974dac75..1013d727cc877cdb39ecda73e2b72b5c29dd4e78 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -5170,7 +5170,7 @@ expand_function_start (tree subr)
r_save = expand_expr (t_save, NULL_RTX, VOIDmode, EXPAND_WRITE);
gcc_assert (GET_MODE (r_save) == Pmode);
- emit_move_insn (r_save, targetm.builtin_setjmp_frame_value ());
+ emit_move_insn (r_save, hard_frame_pointer_rtx);
update_nonlocal_goto_save_area ();
}
diff --git a/gcc/lra-eliminations.c b/gcc/lra-eliminations.c
index d0cfaa8714a8400a6db5da1ffef58b6225f743bd..df8fa9017fa8f9fffbd2e7c4af969609cf8a5266 100644
--- a/gcc/lra-eliminations.c
+++ b/gcc/lra-eliminations.c
@@ -876,32 +876,6 @@ mark_not_eliminable (rtx x, machine_mode mem_mode)
\f
-#ifdef HARD_FRAME_POINTER_REGNUM
-
-/* Search INSN's reg notes to see whether the destination is equal to
- WHAT + C for some constant C. Return true if so, storing C in
- *OFFSET_OUT and removing the reg note. */
-static bool
-remove_reg_equal_offset_note (rtx_insn *insn, rtx what, poly_int64 *offset_out)
-{
- rtx link, *link_loc;
-
- for (link_loc = ®_NOTES (insn);
- (link = *link_loc) != NULL_RTX;
- link_loc = &XEXP (link, 1))
- if (REG_NOTE_KIND (link) == REG_EQUAL
- && GET_CODE (XEXP (link, 0)) == PLUS
- && XEXP (XEXP (link, 0), 0) == what
- && poly_int_rtx_p (XEXP (XEXP (link, 0), 1), offset_out))
- {
- *link_loc = XEXP (link, 1);
- return true;
- }
- return false;
-}
-
-#endif
-
/* Scan INSN and eliminate all eliminable hard registers in it.
If REPLACE_P is true, do the replacement destructively. Also
@@ -939,72 +913,6 @@ eliminate_regs_in_insn (rtx_insn *insn, bool replace_p, bool first_p,
return;
}
- /* Check for setting an eliminable register. */
- if (old_set != 0 && REG_P (SET_DEST (old_set))
- && (ep = get_elimination (SET_DEST (old_set))) != NULL)
- {
- for (ep = reg_eliminate; ep < ®_eliminate[NUM_ELIMINABLE_REGS]; ep++)
- if (ep->from_rtx == SET_DEST (old_set) && ep->can_eliminate)
- {
- bool delete_p = replace_p;
-
-#ifdef HARD_FRAME_POINTER_REGNUM
- if (ep->from == FRAME_POINTER_REGNUM
- && ep->to == HARD_FRAME_POINTER_REGNUM)
- /* If this is setting the frame pointer register to the
- hardware frame pointer register and this is an
- elimination that will be done (tested above), this
- insn is really adjusting the frame pointer downward
- to compensate for the adjustment done before a
- nonlocal goto. */
- {
- rtx src = SET_SRC (old_set);
- poly_int64 offset = 0;
-
- /* We should never process such insn with non-zero
- UPDATE_SP_OFFSET. */
- lra_assert (known_eq (update_sp_offset, 0));
-
- if (remove_reg_equal_offset_note (insn, ep->to_rtx, &offset)
- || strip_offset (src, &offset) == ep->to_rtx)
- {
- if (replace_p)
- {
- SET_DEST (old_set) = ep->to_rtx;
- lra_update_insn_recog_data (insn);
- return;
- }
- offset -= (ep->offset - ep->previous_offset);
- src = plus_constant (Pmode, ep->to_rtx, offset);
-
- /* First see if this insn remains valid when we
- make the change. If not, keep the INSN_CODE
- the same and let the constraint pass fit it
- up. */
- validate_change (insn, &SET_SRC (old_set), src, 1);
- validate_change (insn, &SET_DEST (old_set),
- ep->from_rtx, 1);
- if (! apply_change_group ())
- {
- SET_SRC (old_set) = src;
- SET_DEST (old_set) = ep->from_rtx;
- }
- lra_update_insn_recog_data (insn);
- /* Add offset note for future updates. */
- add_reg_note (insn, REG_EQUAL, copy_rtx (src));
- return;
- }
- }
-#endif
-
- /* This insn isn't serving a useful purpose. We delete it
- when REPLACE is set. */
- if (delete_p)
- lra_delete_dead_insn (insn);
- return;
- }
- }
-
/* We allow one special case which happens to work on all machines we
currently support: a single set with the source or a REG_EQUAL
note being a PLUS of an eliminable register and a constant. */
diff --git a/gcc/reload1.c b/gcc/reload1.c
index 3c0c9ff982fcbd4daba481c62a486e02b74ffbdd..9117325e5b56146ecaa3da2278c36e5435678c56 100644
--- a/gcc/reload1.c
+++ b/gcc/reload1.c
@@ -3234,96 +3234,6 @@ eliminate_regs_in_insn (rtx_insn *insn, int replace)
return 0;
}
- if (old_set != 0 && REG_P (SET_DEST (old_set))
- && REGNO (SET_DEST (old_set)) < FIRST_PSEUDO_REGISTER)
- {
- /* Check for setting an eliminable register. */
- for (ep = reg_eliminate; ep < ®_eliminate[NUM_ELIMINABLE_REGS]; ep++)
- if (ep->from_rtx == SET_DEST (old_set) && ep->can_eliminate)
- {
- /* If this is setting the frame pointer register to the
- hardware frame pointer register and this is an elimination
- that will be done (tested above), this insn is really
- adjusting the frame pointer downward to compensate for
- the adjustment done before a nonlocal goto. */
- if (!HARD_FRAME_POINTER_IS_FRAME_POINTER
- && ep->from == FRAME_POINTER_REGNUM
- && ep->to == HARD_FRAME_POINTER_REGNUM)
- {
- rtx base = SET_SRC (old_set);
- rtx_insn *base_insn = insn;
- HOST_WIDE_INT offset = 0;
-
- while (base != ep->to_rtx)
- {
- rtx_insn *prev_insn;
- rtx prev_set;
-
- if (GET_CODE (base) == PLUS
- && CONST_INT_P (XEXP (base, 1)))
- {
- offset += INTVAL (XEXP (base, 1));
- base = XEXP (base, 0);
- }
- else if ((prev_insn = prev_nonnote_insn (base_insn)) != 0
- && (prev_set = single_set (prev_insn)) != 0
- && rtx_equal_p (SET_DEST (prev_set), base))
- {
- base = SET_SRC (prev_set);
- base_insn = prev_insn;
- }
- else
- break;
- }
-
- if (base == ep->to_rtx)
- {
- rtx src = plus_constant (Pmode, ep->to_rtx,
- offset - ep->offset);
-
- new_body = old_body;
- if (! replace)
- {
- new_body = copy_insn (old_body);
- if (REG_NOTES (insn))
- REG_NOTES (insn) = copy_insn_1 (REG_NOTES (insn));
- }
- PATTERN (insn) = new_body;
- old_set = single_set (insn);
-
- /* First see if this insn remains valid when we
- make the change. If not, keep the INSN_CODE
- the same and let reload fit it up. */
- validate_change (insn, &SET_SRC (old_set), src, 1);
- validate_change (insn, &SET_DEST (old_set),
- ep->to_rtx, 1);
- if (! apply_change_group ())
- {
- SET_SRC (old_set) = src;
- SET_DEST (old_set) = ep->to_rtx;
- }
-
- val = 1;
- goto done;
- }
- }
-
- /* In this case this insn isn't serving a useful purpose. We
- will delete it in reload_as_needed once we know that this
- elimination is, in fact, being done.
-
- If REPLACE isn't set, we can't delete this insn, but needn't
- process it since it won't be used unless something changes. */
- if (replace)
- {
- delete_dead_insn (insn);
- return 1;
- }
- val = 1;
- goto done;
- }
- }
-
/* We allow one special case which happens to work on all machines we
currently support: a single set with the source or a REG_EQUAL
note being a PLUS of an eliminable register and a constant. */
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr84521.c b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
new file mode 100644
index 0000000000000000000000000000000000000000..995a30223afd1401186c7eaf541f27606aed59b5
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
@@ -0,0 +1,54 @@
+/* { dg-require-effective-target indirect_jumps } */
+/* { dg-additional-options "-fomit-frame-pointer -fno-inline" } */
+
+extern void abort (void);
+
+void
+broken_longjmp (void *p)
+{
+ __builtin_longjmp (p, 1);
+}
+
+volatile int x = 256;
+void *volatile p = (void*)&x;
+void *volatile p1;
+
+void
+test (void)
+{
+ void *buf[5];
+ void *volatile q = p;
+
+ if (!__builtin_setjmp (buf))
+ broken_longjmp (buf);
+
+ /* Fails if stack pointer corrupted. */
+ if (p != q)
+ abort ();
+}
+
+void
+test2 (void)
+{
+ void *volatile q = p;
+ p1 = __builtin_alloca (x);
+ test ();
+
+ /* Fails if frame pointer corrupted. */
+ if (p != q)
+ abort ();
+}
+
+int
+main (void)
+{
+ void *volatile q = p;
+ test ();
+ test2 ();
+ /* Fails if stack pointer corrupted. */
+ if (p != q)
+ abort ();
+
+ return 0;
+}
+
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR84521
2018-12-14 13:17 [PATCH] Fix PR84521 Wilco Dijkstra
@ 2019-01-10 13:03 ` Wilco Dijkstra
0 siblings, 0 replies; 15+ messages in thread
From: Wilco Dijkstra @ 2019-01-10 13:03 UTC (permalink / raw)
To: GCC Patches; +Cc: nd
ping
From: Wilco Dijkstra
Sent: 14 December 2018 13:16
To: GCC Patches
Cc: nd
Subject: [PATCH] Fix PR84521
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 <wdijkstr@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 = fp 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.
---
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 2ef9c9afcc69fcb775dc6a6fff550025bdc76337..55b78adbc3df8c970083e6d9b548a8ca7dc52600 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -982,7 +982,7 @@ expand_builtin_setjmp_setup (rtx buf_addr, rtx receiver_label)
mem = gen_rtx_MEM (Pmode, buf_addr);
set_mem_alias_set (mem, setjmp_alias_set);
- emit_move_insn (mem, targetm.builtin_setjmp_frame_value ());
+ emit_move_insn (mem, hard_frame_pointer_rtx);
mem = gen_rtx_MEM (Pmode, plus_constant (Pmode, buf_addr,
GET_MODE_SIZE (Pmode))),
@@ -1024,31 +1024,6 @@ expand_builtin_setjmp_receiver (rtx receiver_label)
if (chain && REG_P (chain))
emit_clobber (chain);
- /* Now put in the code to restore the frame pointer, and argument
- pointer, if needed. */
- if (! targetm.have_nonlocal_goto ())
- {
- /* First adjust our frame pointer to its actual value. It was
- previously set to the start of the virtual area corresponding to
- the stacked variables when we branched here and now needs to be
- adjusted to the actual hardware fp value.
-
- Assignments to virtual registers are converted by
- instantiate_virtual_regs into the corresponding assignment
- to the underlying register (fp in this case) that makes
- the original assignment true.
- So the following insn will actually be decrementing fp by
- TARGET_STARTING_FRAME_OFFSET. */
- emit_move_insn (virtual_stack_vars_rtx, hard_frame_pointer_rtx);
-
- /* Restoring the frame pointer also modifies the hard frame pointer.
- Mark it used (so that the previous assignment remains live once
- the frame pointer is eliminated) and clobbered (to represent the
- implicit update from the assignment). */
- emit_use (hard_frame_pointer_rtx);
- emit_clobber (hard_frame_pointer_rtx);
- }
-
if (!HARD_FRAME_POINTER_IS_ARG_POINTER && fixed_regs[ARG_POINTER_REGNUM])
{
/* If the argument pointer can be eliminated in favor of the
diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c
index 55175215bfed6e166a92ae046b36487662c36dd0..9058729a76e7f85f188ed0323190549ddd886fa9 100644
--- a/gcc/config/arc/arc.c
+++ b/gcc/config/arc/arc.c
@@ -689,8 +689,6 @@ static rtx arc_legitimize_address_0 (rtx, rtx, machine_mode mode);
#undef TARGET_MODES_TIEABLE_P
#define TARGET_MODES_TIEABLE_P arc_modes_tieable_p
-#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
-#define TARGET_BUILTIN_SETJMP_FRAME_VALUE arc_builtin_setjmp_frame_value
/* Try to keep the (mov:DF _, reg) as early as possible so
that the d<add/sub/mul>h-lr insns appear together and can
@@ -10964,28 +10962,6 @@ compact_memory_operand_p (rtx op, machine_mode mode,
return false;
}
-/* Return the frame pointer value to be backed up in the setjmp buffer. */
-
-static rtx
-arc_builtin_setjmp_frame_value (void)
-{
- /* We always want to preserve whatever value is currently in the frame
- pointer register. For frames that are using the frame pointer the new
- value of the frame pointer register will have already been computed
- (as part of the prologue). For frames that are not using the frame
- pointer it is important that we backup whatever value is in the frame
- pointer register, as earlier (more outer) frames may have placed a
- value into the frame pointer register. It might be tempting to try
- and use `frame_pointer_rtx` here, however, this is not what we want.
- For frames that are using the frame pointer this will give the
- correct value. However, for frames that are not using the frame
- pointer this will still give the value that _would_ have been the
- frame pointer value for this frame (if the use of the frame pointer
- had not been removed). We really do want the raw frame pointer
- register value. */
- return gen_raw_REG (Pmode, FRAME_POINTER_REGNUM);
-}
-
/* Return nonzero if a jli call should be generated for a call from
the current function to DECL. */
diff --git a/gcc/config/avr/avr.c b/gcc/config/avr/avr.c
index 81c35e7fbc2b06bc06cc7fb7406694bbe48860e5..4fdc38649eb9dc88d33b17f64fe09ffe22f57d1c 100644
--- a/gcc/config/avr/avr.c
+++ b/gcc/config/avr/avr.c
@@ -1302,22 +1302,6 @@ avr_build_builtin_va_list (void)
}
-/* Implement `TARGET_BUILTIN_SETJMP_FRAME_VALUE'. */
-/* Actual start of frame is virtual_stack_vars_rtx this is offset from
- frame pointer by +TARGET_STARTING_FRAME_OFFSET.
- Using saved frame = virtual_stack_vars_rtx - TARGET_STARTING_FRAME_OFFSET
- avoids creating add/sub of offset in nonlocal goto and setjmp. */
-
-static rtx
-avr_builtin_setjmp_frame_value (void)
-{
- rtx xval = gen_reg_rtx (Pmode);
- emit_insn (gen_subhi3 (xval, virtual_stack_vars_rtx,
- gen_int_mode (avr_starting_frame_offset (), Pmode)));
- return xval;
-}
-
-
/* Return contents of MEM at frame pointer + stack size + 1 (+2 if 3-byte PC).
This is return address of function. */
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 1d7b1f0d1b28a7a9368391cef42fae7b6e7fd160..80f92d684ba705de48f3fafa5f4ccb7b937546c2 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -11108,17 +11108,6 @@ ix86_initial_elimination_offset (int from, int to)
}
}
-/* In a dynamically-aligned function, we can't know the offset from
- stack pointer to frame pointer, so we must ensure that setjmp
- eliminates fp against the hard fp (%ebp) rather than trying to
- index from %esp up to the top of the frame across a gap that is
- of unknown (at compile-time) size. */
-static rtx
-ix86_builtin_setjmp_frame_value (void)
-{
- return stack_realign_fp ? hard_frame_pointer_rtx : virtual_stack_vars_rtx;
-}
-
/* Emits a warning for unsupported msabi to sysv pro/epilogues. */
static void warn_once_call_ms2sysv_xlogues (const char *feature)
{
@@ -50934,9 +50923,6 @@ ix86_run_selftests (void)
#undef TARGET_MACHINE_DEPENDENT_REORG
#define TARGET_MACHINE_DEPENDENT_REORG ix86_reorg
-#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
-#define TARGET_BUILTIN_SETJMP_FRAME_VALUE ix86_builtin_setjmp_frame_value
-
#undef TARGET_BUILD_BUILTIN_VA_LIST
#define TARGET_BUILD_BUILTIN_VA_LIST ix86_build_builtin_va_list
diff --git a/gcc/config/pa/pa.md b/gcc/config/pa/pa.md
index 77611503c67732fb71114e7f1d8e7c1a8883ef41..2fcd597d572117f928c618082d9a61a1463ea6c2 100644
--- a/gcc/config/pa/pa.md
+++ b/gcc/config/pa/pa.md
@@ -6909,13 +6909,7 @@ (define_expand "nonlocal_goto"
emit_clobber (gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (VOIDmode)));
emit_clobber (gen_rtx_MEM (BLKmode, hard_frame_pointer_rtx));
- /* Restore the frame pointer. The virtual_stack_vars_rtx is saved
- instead of the hard_frame_pointer_rtx in the save area. As a
- result, an extra instruction is needed to adjust for the offset
- of the virtual stack variables and the hard frame pointer. */
- if (GET_CODE (fp) != REG)
- fp = force_reg (Pmode, fp);
- emit_move_insn (hard_frame_pointer_rtx, plus_constant (Pmode, fp, -8));
+ emit_move_insn (hard_frame_pointer_rtx, fp);
emit_stack_restore (SAVE_NONLOCAL, stack);
diff --git a/gcc/config/sparc/sparc.c b/gcc/config/sparc/sparc.c
index adbef1ab18f532739cf3c0ba1ac80ae5ee288076..093807a8c851024d60d4b96fbb942a9a02e19e8e 100644
--- a/gcc/config/sparc/sparc.c
+++ b/gcc/config/sparc/sparc.c
@@ -679,7 +679,6 @@ static void sparc_output_dwarf_dtprel (FILE *, int, rtx) ATTRIBUTE_UNUSED;
static void sparc_file_end (void);
static bool sparc_frame_pointer_required (void);
static bool sparc_can_eliminate (const int, const int);
-static rtx sparc_builtin_setjmp_frame_value (void);
static void sparc_conditional_register_usage (void);
static bool sparc_use_pseudo_pic_reg (void);
static void sparc_init_pic_reg (void);
@@ -883,9 +882,6 @@ char sparc_hard_reg_printed[8];
#undef TARGET_FRAME_POINTER_REQUIRED
#define TARGET_FRAME_POINTER_REQUIRED sparc_frame_pointer_required
-#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
-#define TARGET_BUILTIN_SETJMP_FRAME_VALUE sparc_builtin_setjmp_frame_value
-
#undef TARGET_CAN_ELIMINATE
#define TARGET_CAN_ELIMINATE sparc_can_eliminate
@@ -12985,14 +12981,6 @@ sparc_can_eliminate (const int from ATTRIBUTE_UNUSED, const int to)
return to == HARD_FRAME_POINTER_REGNUM || !sparc_frame_pointer_required ();
}
-/* Return the hard frame pointer directly to bypass the stack bias. */
-
-static rtx
-sparc_builtin_setjmp_frame_value (void)
-{
- return hard_frame_pointer_rtx;
-}
-
/* If !TARGET_FPU, then make the fp registers and fp cc regs fixed so that
they won't be allocated. */
diff --git a/gcc/config/vax/vax.c b/gcc/config/vax/vax.c
index 631c598d3b2f26f250d83b370e3e5ddc7ade0919..d0594f023002a2cd7a01e99690eda1d0f7a5b160 100644
--- a/gcc/config/vax/vax.c
+++ b/gcc/config/vax/vax.c
@@ -59,7 +59,6 @@ static rtx vax_function_arg (cumulative_args_t, machine_mode,
static void vax_function_arg_advance (cumulative_args_t, machine_mode,
const_tree, bool);
static rtx vax_struct_value_rtx (tree, int);
-static rtx vax_builtin_setjmp_frame_value (void);
static void vax_asm_trampoline_template (FILE *);
static void vax_trampoline_init (rtx, tree, rtx);
static poly_int64 vax_return_pops_args (tree, tree, poly_int64);
@@ -99,9 +98,6 @@ static HOST_WIDE_INT vax_starting_frame_offset (void);
#undef TARGET_STRUCT_VALUE_RTX
#define TARGET_STRUCT_VALUE_RTX vax_struct_value_rtx
-#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
-#define TARGET_BUILTIN_SETJMP_FRAME_VALUE vax_builtin_setjmp_frame_value
-
#undef TARGET_LRA_P
#define TARGET_LRA_P hook_bool_void_false
@@ -1063,12 +1059,6 @@ vax_struct_value_rtx (tree fntype ATTRIBUTE_UNUSED,
return gen_rtx_REG (Pmode, VAX_STRUCT_VALUE_REGNUM);
}
-static rtx
-vax_builtin_setjmp_frame_value (void)
-{
- return hard_frame_pointer_rtx;
-}
-
/* Worker function for NOTICE_UPDATE_CC. */
void
diff --git a/gcc/function.c b/gcc/function.c
index 69523c1d72309b7ba0e7a0e6ccfadcd3974dac75..1013d727cc877cdb39ecda73e2b72b5c29dd4e78 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -5170,7 +5170,7 @@ expand_function_start (tree subr)
r_save = expand_expr (t_save, NULL_RTX, VOIDmode, EXPAND_WRITE);
gcc_assert (GET_MODE (r_save) == Pmode);
- emit_move_insn (r_save, targetm.builtin_setjmp_frame_value ());
+ emit_move_insn (r_save, hard_frame_pointer_rtx);
update_nonlocal_goto_save_area ();
}
diff --git a/gcc/lra-eliminations.c b/gcc/lra-eliminations.c
index d0cfaa8714a8400a6db5da1ffef58b6225f743bd..df8fa9017fa8f9fffbd2e7c4af969609cf8a5266 100644
--- a/gcc/lra-eliminations.c
+++ b/gcc/lra-eliminations.c
@@ -876,32 +876,6 @@ mark_not_eliminable (rtx x, machine_mode mem_mode)
-#ifdef HARD_FRAME_POINTER_REGNUM
-
-/* Search INSN's reg notes to see whether the destination is equal to
- WHAT + C for some constant C. Return true if so, storing C in
- *OFFSET_OUT and removing the reg note. */
-static bool
-remove_reg_equal_offset_note (rtx_insn *insn, rtx what, poly_int64 *offset_out)
-{
- rtx link, *link_loc;
-
- for (link_loc = ®_NOTES (insn);
- (link = *link_loc) != NULL_RTX;
- link_loc = &XEXP (link, 1))
- if (REG_NOTE_KIND (link) == REG_EQUAL
- && GET_CODE (XEXP (link, 0)) == PLUS
- && XEXP (XEXP (link, 0), 0) == what
- && poly_int_rtx_p (XEXP (XEXP (link, 0), 1), offset_out))
- {
- *link_loc = XEXP (link, 1);
- return true;
- }
- return false;
-}
-
-#endif
-
/* Scan INSN and eliminate all eliminable hard registers in it.
If REPLACE_P is true, do the replacement destructively. Also
@@ -939,72 +913,6 @@ eliminate_regs_in_insn (rtx_insn *insn, bool replace_p, bool first_p,
return;
}
- /* Check for setting an eliminable register. */
- if (old_set != 0 && REG_P (SET_DEST (old_set))
- && (ep = get_elimination (SET_DEST (old_set))) != NULL)
- {
- for (ep = reg_eliminate; ep < ®_eliminate[NUM_ELIMINABLE_REGS]; ep++)
- if (ep->from_rtx == SET_DEST (old_set) && ep->can_eliminate)
- {
- bool delete_p = replace_p;
-
-#ifdef HARD_FRAME_POINTER_REGNUM
- if (ep->from == FRAME_POINTER_REGNUM
- && ep->to == HARD_FRAME_POINTER_REGNUM)
- /* If this is setting the frame pointer register to the
- hardware frame pointer register and this is an
- elimination that will be done (tested above), this
- insn is really adjusting the frame pointer downward
- to compensate for the adjustment done before a
- nonlocal goto. */
- {
- rtx src = SET_SRC (old_set);
- poly_int64 offset = 0;
-
- /* We should never process such insn with non-zero
- UPDATE_SP_OFFSET. */
- lra_assert (known_eq (update_sp_offset, 0));
-
- if (remove_reg_equal_offset_note (insn, ep->to_rtx, &offset)
- || strip_offset (src, &offset) == ep->to_rtx)
- {
- if (replace_p)
- {
- SET_DEST (old_set) = ep->to_rtx;
- lra_update_insn_recog_data (insn);
- return;
- }
- offset -= (ep->offset - ep->previous_offset);
- src = plus_constant (Pmode, ep->to_rtx, offset);
-
- /* First see if this insn remains valid when we
- make the change. If not, keep the INSN_CODE
- the same and let the constraint pass fit it
- up. */
- validate_change (insn, &SET_SRC (old_set), src, 1);
- validate_change (insn, &SET_DEST (old_set),
- ep->from_rtx, 1);
- if (! apply_change_group ())
- {
- SET_SRC (old_set) = src;
- SET_DEST (old_set) = ep->from_rtx;
- }
- lra_update_insn_recog_data (insn);
- /* Add offset note for future updates. */
- add_reg_note (insn, REG_EQUAL, copy_rtx (src));
- return;
- }
- }
-#endif
-
- /* This insn isn't serving a useful purpose. We delete it
- when REPLACE is set. */
- if (delete_p)
- lra_delete_dead_insn (insn);
- return;
- }
- }
-
/* We allow one special case which happens to work on all machines we
currently support: a single set with the source or a REG_EQUAL
note being a PLUS of an eliminable register and a constant. */
diff --git a/gcc/reload1.c b/gcc/reload1.c
index 3c0c9ff982fcbd4daba481c62a486e02b74ffbdd..9117325e5b56146ecaa3da2278c36e5435678c56 100644
--- a/gcc/reload1.c
+++ b/gcc/reload1.c
@@ -3234,96 +3234,6 @@ eliminate_regs_in_insn (rtx_insn *insn, int replace)
return 0;
}
- if (old_set != 0 && REG_P (SET_DEST (old_set))
- && REGNO (SET_DEST (old_set)) < FIRST_PSEUDO_REGISTER)
- {
- /* Check for setting an eliminable register. */
- for (ep = reg_eliminate; ep < ®_eliminate[NUM_ELIMINABLE_REGS]; ep++)
- if (ep->from_rtx == SET_DEST (old_set) && ep->can_eliminate)
- {
- /* If this is setting the frame pointer register to the
- hardware frame pointer register and this is an elimination
- that will be done (tested above), this insn is really
- adjusting the frame pointer downward to compensate for
- the adjustment done before a nonlocal goto. */
- if (!HARD_FRAME_POINTER_IS_FRAME_POINTER
- && ep->from == FRAME_POINTER_REGNUM
- && ep->to == HARD_FRAME_POINTER_REGNUM)
- {
- rtx base = SET_SRC (old_set);
- rtx_insn *base_insn = insn;
- HOST_WIDE_INT offset = 0;
-
- while (base != ep->to_rtx)
- {
- rtx_insn *prev_insn;
- rtx prev_set;
-
- if (GET_CODE (base) == PLUS
- && CONST_INT_P (XEXP (base, 1)))
- {
- offset += INTVAL (XEXP (base, 1));
- base = XEXP (base, 0);
- }
- else if ((prev_insn = prev_nonnote_insn (base_insn)) != 0
- && (prev_set = single_set (prev_insn)) != 0
- && rtx_equal_p (SET_DEST (prev_set), base))
- {
- base = SET_SRC (prev_set);
- base_insn = prev_insn;
- }
- else
- break;
- }
-
- if (base == ep->to_rtx)
- {
- rtx src = plus_constant (Pmode, ep->to_rtx,
- offset - ep->offset);
-
- new_body = old_body;
- if (! replace)
- {
- new_body = copy_insn (old_body);
- if (REG_NOTES (insn))
- REG_NOTES (insn) = copy_insn_1 (REG_NOTES (insn));
- }
- PATTERN (insn) = new_body;
- old_set = single_set (insn);
-
- /* First see if this insn remains valid when we
- make the change. If not, keep the INSN_CODE
- the same and let reload fit it up. */
- validate_change (insn, &SET_SRC (old_set), src, 1);
- validate_change (insn, &SET_DEST (old_set),
- ep->to_rtx, 1);
- if (! apply_change_group ())
- {
- SET_SRC (old_set) = src;
- SET_DEST (old_set) = ep->to_rtx;
- }
-
- val = 1;
- goto done;
- }
- }
-
- /* In this case this insn isn't serving a useful purpose. We
- will delete it in reload_as_needed once we know that this
- elimination is, in fact, being done.
-
- If REPLACE isn't set, we can't delete this insn, but needn't
- process it since it won't be used unless something changes. */
- if (replace)
- {
- delete_dead_insn (insn);
- return 1;
- }
- val = 1;
- goto done;
- }
- }
-
/* We allow one special case which happens to work on all machines we
currently support: a single set with the source or a REG_EQUAL
note being a PLUS of an eliminable register and a constant. */
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr84521.c b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
new file mode 100644
index 0000000000000000000000000000000000000000..995a30223afd1401186c7eaf541f27606aed59b5
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
@@ -0,0 +1,54 @@
+/* { dg-require-effective-target indirect_jumps } */
+/* { dg-additional-options "-fomit-frame-pointer -fno-inline" } */
+
+extern void abort (void);
+
+void
+broken_longjmp (void *p)
+{
+ __builtin_longjmp (p, 1);
+}
+
+volatile int x = 256;
+void *volatile p = (void*)&x;
+void *volatile p1;
+
+void
+test (void)
+{
+ void *buf[5];
+ void *volatile q = p;
+
+ if (!__builtin_setjmp (buf))
+ broken_longjmp (buf);
+
+ /* Fails if stack pointer corrupted. */
+ if (p != q)
+ abort ();
+}
+
+void
+test2 (void)
+{
+ void *volatile q = p;
+ p1 = __builtin_alloca (x);
+ test ();
+
+ /* Fails if frame pointer corrupted. */
+ if (p != q)
+ abort ();
+}
+
+int
+main (void)
+{
+ void *volatile q = p;
+ test ();
+ test2 ();
+ /* Fails if stack pointer corrupted. */
+ if (p != q)
+ abort ();
+
+ return 0;
+}
+
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR84521
2019-06-19 4:08 ` Max Filippov
@ 2019-06-19 13:06 ` Wilco Dijkstra
0 siblings, 0 replies; 15+ messages in thread
From: Wilco Dijkstra @ 2019-06-19 13:06 UTC (permalink / raw)
To: Max Filippov; +Cc: Jeff Law, GCC Patches, nd, Sterling Augustine
Hi Max,
> On Tue, Jun 18, 2019 at 4:53 PM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> > > It would work if a frame pointer was initialized in the function test, but
> > > it wasn't:
> >
> > Right, because it unwinds, it needs a valid frame pointer since we no
> > longer store the stack pointer. So xtensa_frame_pointer_required
> > should do something like:
> >
> > if (cfun->machine->accesses_prev_frame || cfun->has_nonlocal_label)
> > return true;
>
> You're right, with this change things are back to normal.
Great, I've added this to the commit. Thanks for the proactive testing!
Wilco
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR84521
2019-06-18 23:53 ` Wilco Dijkstra
@ 2019-06-19 4:08 ` Max Filippov
2019-06-19 13:06 ` Wilco Dijkstra
0 siblings, 1 reply; 15+ messages in thread
From: Max Filippov @ 2019-06-19 4:08 UTC (permalink / raw)
To: Wilco Dijkstra; +Cc: Jeff Law, GCC Patches, nd, Sterling Augustine
On Tue, Jun 18, 2019 at 4:53 PM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> > It would work if a frame pointer was initialized in the function test, but
> > it wasn't:
>
> Right, because it unwinds, it needs a valid frame pointer since we no
> longer store the stack pointer. So xtensa_frame_pointer_required
> should do something like:
>
> if (cfun->machine->accesses_prev_frame || cfun->has_nonlocal_label)
> return true;
You're right, with this change things are back to normal.
--
Thanks.
-- Max
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR84521
2019-06-18 23:36 ` Max Filippov
@ 2019-06-18 23:53 ` Wilco Dijkstra
2019-06-19 4:08 ` Max Filippov
0 siblings, 1 reply; 15+ messages in thread
From: Wilco Dijkstra @ 2019-06-18 23:53 UTC (permalink / raw)
To: Max Filippov; +Cc: Jeff Law, GCC Patches, nd, Sterling Augustine
Hi Max,
> It would work if a frame pointer was initialized in the function test, but
> it wasn't:
Right, because it unwinds, it needs a valid frame pointer since we no
longer store the stack pointer. So xtensa_frame_pointer_required
should do something like:
if (cfun->machine->accesses_prev_frame || cfun->has_nonlocal_label)
return true;
Wilco
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR84521
2019-06-18 17:07 ` Wilco Dijkstra
@ 2019-06-18 23:36 ` Max Filippov
2019-06-18 23:53 ` Wilco Dijkstra
0 siblings, 1 reply; 15+ messages in thread
From: Max Filippov @ 2019-06-18 23:36 UTC (permalink / raw)
To: Wilco Dijkstra; +Cc: Jeff Law, GCC Patches, nd, Sterling Augustine
On Tue, Jun 18, 2019 at 10:07 AM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> > The testcase from the patch passes with the trunk xtensa-linux-gcc
> > with windowed ABI. But with the changes in this patch a lot of tests
> > that use longjmp are failing on xtensa-linux.
>
> Interesting. I looked at the _xtensa_nonlocal_goto implementation in
> libgcc/config/xtensa/lib2funcs.S, and it should work fine given it already
> checks for the frame pointer to be within the bounds of a frame.
It would work if a frame pointer was initialized in the function test, but
it wasn't:
test:
entry sp, 64
l32r a2, .LC1
memw
l32i.n a2, a2, 0
memw
s32i.n a2, sp, 20
s32i.n a7, sp, 0 <----
l32r a2, .LC2
s32i.n a2, sp, 4
original version stored the sp there.
--
Thanks.
-- Max
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR84521
2019-06-18 17:55 ` Andreas Schwab
@ 2019-06-18 18:11 ` Wilco Dijkstra
0 siblings, 0 replies; 15+ messages in thread
From: Wilco Dijkstra @ 2019-06-18 18:11 UTC (permalink / raw)
To: Andreas Schwab, H.J. Lu; +Cc: GCC Patches, nd
Hi,
> > Is this test valid? Can jmp buffer be allowed on stack?
>
> Sure, the contents of the jmp buffer is only valid during the lifetime
> of the call frame anyway.
Indeed. The issue with jmp buffer being on the stack causing incorrect
restore when doing longjmp has just been fixed (PR64242).
Wilco
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR84521
2019-06-18 16:47 ` H.J. Lu
@ 2019-06-18 17:55 ` Andreas Schwab
2019-06-18 18:11 ` Wilco Dijkstra
0 siblings, 1 reply; 15+ messages in thread
From: Andreas Schwab @ 2019-06-18 17:55 UTC (permalink / raw)
To: H.J. Lu; +Cc: Wilco Dijkstra, GCC Patches, nd
On Jun 18 2019, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>> +void
>> +test (void)
>> +{
>> + void *buf[5];
>> + void *volatile q = p;
>> +
>> + if (!__builtin_setjmp (buf))
>> + broken_longjmp (buf);
>
> Is this test valid? Can jmp buffer be allowed on stack?
Sure, the contents of the jmp buffer is only valid during the lifetime
of the call frame anyway.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR84521
2019-06-18 16:27 ` Max Filippov
@ 2019-06-18 17:07 ` Wilco Dijkstra
2019-06-18 23:36 ` Max Filippov
0 siblings, 1 reply; 15+ messages in thread
From: Wilco Dijkstra @ 2019-06-18 17:07 UTC (permalink / raw)
To: Max Filippov, Jeff Law; +Cc: GCC Patches, nd, Sterling Augustine
Hi Max,
> The testcase from the patch passes with the trunk xtensa-linux-gcc
> with windowed ABI. But with the changes in this patch a lot of tests
> that use longjmp are failing on xtensa-linux.
Interesting. I looked at the _xtensa_nonlocal_goto implementation in
libgcc/config/xtensa/lib2funcs.S, and it should work fine given it already
checks for the frame pointer to be within the bounds of a frame.
Whether we pass the virtual frame pointer or the hard frame pointer value
shouldn't matter as long as both are >= SP and < prev_SP. Maybe there
is an off by one issue in:
.Lfirstframe:
l32i a7, a6, 4 /* a7 = next */
bgeu a2, a7, .Lnextframe
> call0 and windowed ABI xtensa code are not supposed to work together.
OK so that's not an issue then.
Wilco
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR84521
2019-05-28 19:05 Wilco Dijkstra
2019-06-17 23:04 ` Jeff Law
@ 2019-06-18 16:47 ` H.J. Lu
2019-06-18 17:55 ` Andreas Schwab
1 sibling, 1 reply; 15+ messages in thread
From: H.J. Lu @ 2019-06-18 16:47 UTC (permalink / raw)
To: Wilco Dijkstra; +Cc: GCC Patches, nd
On Tue, May 28, 2019 at 10:37 AM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>
>
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr84521.c b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..995a30223afd1401186c7eaf541f27606aed59b5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
> @@ -0,0 +1,54 @@
> +/* { dg-require-effective-target indirect_jumps } */
> +/* { dg-additional-options "-fomit-frame-pointer -fno-inline" } */
> +
> +extern void abort (void);
> +
> +void
> +broken_longjmp (void *p)
> +{
> + __builtin_longjmp (p, 1);
> +}
> +
> +volatile int x = 256;
> +void *volatile p = (void*)&x;
> +void *volatile p1;
> +
> +void
> +test (void)
> +{
> + void *buf[5];
> + void *volatile q = p;
> +
> + if (!__builtin_setjmp (buf))
> + broken_longjmp (buf);
Is this test valid? Can jmp buffer be allowed on stack?
> + /* Fails if stack pointer corrupted. */
> + if (p != q)
> + abort ();
> +}
> +
> +void
> +test2 (void)
> +{
> + void *volatile q = p;
> + p1 = __builtin_alloca (x);
> + test ();
> +
> + /* Fails if frame pointer corrupted. */
> + if (p != q)
> + abort ();
> +}
> +
> +int
> +main (void)
> +{
> + void *volatile q = p;
> + test ();
> + test2 ();
> + /* Fails if stack pointer corrupted. */
> + if (p != q)
> + abort ();
> +
> + return 0;
> +}
> +
--
H.J.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR84521
2019-06-18 1:10 ` Jeff Law
@ 2019-06-18 16:27 ` Max Filippov
2019-06-18 17:07 ` Wilco Dijkstra
0 siblings, 1 reply; 15+ messages in thread
From: Max Filippov @ 2019-06-18 16:27 UTC (permalink / raw)
To: Jeff Law; +Cc: Wilco Dijkstra, GCC Patches, nd, Sterling Augustine
Hello,
On Mon, Jun 17, 2019 at 6:10 PM Jeff Law <law@redhat.com> wrote:
> On 6/17/19 6:58 PM, Wilco Dijkstra wrote:
> >> You mention that arm, mips and xtensa are still broken. Are they worse
> >> after this patch?
The testcase from the patch passes with the trunk xtensa-linux-gcc
with windowed ABI. But with the changes in this patch a lot of tests
that use longjmp are failing on xtensa-linux.
> >> 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.
call0 and windowed ABI xtensa code are not supposed to work together.
--
Thanks.
-- Max
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR84521
2019-06-18 0:58 ` Wilco Dijkstra
@ 2019-06-18 1:10 ` Jeff Law
2019-06-18 16:27 ` Max Filippov
0 siblings, 1 reply; 15+ messages in thread
From: Jeff Law @ 2019-06-18 1:10 UTC (permalink / raw)
To: Wilco Dijkstra, GCC Patches; +Cc: nd
On 6/17/19 6:58 PM, Wilco Dijkstra wrote:
> Hi Jeff,
>
>> 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?
>
> No it's not an ABI change. It does affect the value stored in the setjmp
> record, but that is entirely local to the function containing setjmp.
> Note that the function containing the longjmp is not affected by this
> change.
OK. Thanks for the clarification.
>
>> 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?
>
> Yes the only remaining issue after this is the fact that these targets can use
> different frame pointers in functions. The generic code doesn't consider this,
> but any function could be Arm or Thumb, mips or mips16. One solution
> would be to pass the frame pointer in an argument register (that would be
> an ABI change).
ACK. I think if we want to make this work (arm <-> thumb or mips <->
mips16) then we should take that up separately.
>
>> 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.
>
> If it isn't possible to call functions with the other frame pointer then it
> won't be affected.
I'd be surprised given it's two distinct abis as opposed to a processor
mode change like we see in the arm/mips cases.
OK for the trunk. Thanks for your patience.
jeff
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR84521
2019-06-17 23:04 ` Jeff Law
@ 2019-06-18 0:58 ` Wilco Dijkstra
2019-06-18 1:10 ` Jeff Law
0 siblings, 1 reply; 15+ messages in thread
From: Wilco Dijkstra @ 2019-06-18 0:58 UTC (permalink / raw)
To: Jeff Law, GCC Patches; +Cc: nd
Hi Jeff,
> 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?
No it's not an ABI change. It does affect the value stored in the setjmp
record, but that is entirely local to the function containing setjmp.
Note that the function containing the longjmp is not affected by this
change.
> 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?
Yes the only remaining issue after this is the fact that these targets can use
different frame pointers in functions. The generic code doesn't consider this,
but any function could be Arm or Thumb, mips or mips16. One solution
would be to pass the frame pointer in an argument register (that would be
an ABI change).
> 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.
If it isn't possible to call functions with the other frame pointer then it
won't be affected.
Wilco
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR84521
2019-05-28 19:05 Wilco Dijkstra
@ 2019-06-17 23:04 ` Jeff Law
2019-06-18 0:58 ` Wilco Dijkstra
2019-06-18 16:47 ` H.J. Lu
1 sibling, 1 reply; 15+ messages in thread
From: Jeff Law @ 2019-06-17 23:04 UTC (permalink / raw)
To: Wilco Dijkstra, GCC Patches; +Cc: nd
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 <wdijkstr@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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] Fix PR84521
@ 2019-05-28 19:05 Wilco Dijkstra
2019-06-17 23:04 ` Jeff Law
2019-06-18 16:47 ` H.J. Lu
0 siblings, 2 replies; 15+ messages in thread
From: Wilco Dijkstra @ 2019-05-28 19:05 UTC (permalink / raw)
To: GCC Patches; +Cc: nd
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 <wdijkstr@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.
--
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 3463ffb153914a58e5baa3896a244842a28eef09..4ecfd49d03cb34600e7ac7afb0bf0511ff7ec1fa 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -981,7 +981,7 @@ expand_builtin_setjmp_setup (rtx buf_addr, rtx receiver_label)
mem = gen_rtx_MEM (Pmode, buf_addr);
set_mem_alias_set (mem, setjmp_alias_set);
- emit_move_insn (mem, targetm.builtin_setjmp_frame_value ());
+ emit_move_insn (mem, hard_frame_pointer_rtx);
mem = gen_rtx_MEM (Pmode, plus_constant (Pmode, buf_addr,
GET_MODE_SIZE (Pmode))),
@@ -1023,31 +1023,6 @@ expand_builtin_setjmp_receiver (rtx receiver_label)
if (chain && REG_P (chain))
emit_clobber (chain);
- /* Now put in the code to restore the frame pointer, and argument
- pointer, if needed. */
- if (! targetm.have_nonlocal_goto ())
- {
- /* First adjust our frame pointer to its actual value. It was
- previously set to the start of the virtual area corresponding to
- the stacked variables when we branched here and now needs to be
- adjusted to the actual hardware fp value.
-
- Assignments to virtual registers are converted by
- instantiate_virtual_regs into the corresponding assignment
- to the underlying register (fp in this case) that makes
- the original assignment true.
- So the following insn will actually be decrementing fp by
- TARGET_STARTING_FRAME_OFFSET. */
- emit_move_insn (virtual_stack_vars_rtx, hard_frame_pointer_rtx);
-
- /* Restoring the frame pointer also modifies the hard frame pointer.
- Mark it used (so that the previous assignment remains live once
- the frame pointer is eliminated) and clobbered (to represent the
- implicit update from the assignment). */
- emit_use (hard_frame_pointer_rtx);
- emit_clobber (hard_frame_pointer_rtx);
- }
-
if (!HARD_FRAME_POINTER_IS_ARG_POINTER && fixed_regs[ARG_POINTER_REGNUM])
{
/* If the argument pointer can be eliminated in favor of the
diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c
index bce189958bc589c0003d2ea20dbbdf921ff715c6..d8282e633d77c6439426a5b164f01d4a59c5cdde 100644
--- a/gcc/config/arc/arc.c
+++ b/gcc/config/arc/arc.c
@@ -689,8 +689,6 @@ static rtx arc_legitimize_address_0 (rtx, rtx, machine_mode mode);
#undef TARGET_MODES_TIEABLE_P
#define TARGET_MODES_TIEABLE_P arc_modes_tieable_p
-#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
-#define TARGET_BUILTIN_SETJMP_FRAME_VALUE arc_builtin_setjmp_frame_value
/* Try to keep the (mov:DF _, reg) as early as possible so
that the d<add/sub/mul>h-lr insns appear together and can
@@ -10979,28 +10977,6 @@ compact_memory_operand_p (rtx op, machine_mode mode,
return false;
}
-/* Return the frame pointer value to be backed up in the setjmp buffer. */
-
-static rtx
-arc_builtin_setjmp_frame_value (void)
-{
- /* We always want to preserve whatever value is currently in the frame
- pointer register. For frames that are using the frame pointer the new
- value of the frame pointer register will have already been computed
- (as part of the prologue). For frames that are not using the frame
- pointer it is important that we backup whatever value is in the frame
- pointer register, as earlier (more outer) frames may have placed a
- value into the frame pointer register. It might be tempting to try
- and use `frame_pointer_rtx` here, however, this is not what we want.
- For frames that are using the frame pointer this will give the
- correct value. However, for frames that are not using the frame
- pointer this will still give the value that _would_ have been the
- frame pointer value for this frame (if the use of the frame pointer
- had not been removed). We really do want the raw frame pointer
- register value. */
- return gen_raw_REG (Pmode, HARD_FRAME_POINTER_REGNUM);
-}
-
/* Return nonzero if a jli call should be generated for a call from
the current function to DECL. */
diff --git a/gcc/config/avr/avr.c b/gcc/config/avr/avr.c
index a9f72b314c2d397d2892a624c87e076682bcac09..f3896f79cd16e67503bd831aebfaff2efa6d02a3 100644
--- a/gcc/config/avr/avr.c
+++ b/gcc/config/avr/avr.c
@@ -1302,22 +1302,6 @@ avr_build_builtin_va_list (void)
}
-/* Implement `TARGET_BUILTIN_SETJMP_FRAME_VALUE'. */
-/* Actual start of frame is virtual_stack_vars_rtx this is offset from
- frame pointer by +TARGET_STARTING_FRAME_OFFSET.
- Using saved frame = virtual_stack_vars_rtx - TARGET_STARTING_FRAME_OFFSET
- avoids creating add/sub of offset in nonlocal goto and setjmp. */
-
-static rtx
-avr_builtin_setjmp_frame_value (void)
-{
- rtx xval = gen_reg_rtx (Pmode);
- emit_insn (gen_subhi3 (xval, virtual_stack_vars_rtx,
- gen_int_mode (avr_starting_frame_offset (), Pmode)));
- return xval;
-}
-
-
/* Return contents of MEM at frame pointer + stack size + 1 (+2 if 3-byte PC).
This is return address of function. */
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 2eddea56b2e9e6470e75dd6dff6e2966078e8434..547e78e43102631e7883bfcbb777dd04c98aa2df 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -22764,9 +22764,6 @@ ix86_run_selftests (void)
#undef TARGET_MACHINE_DEPENDENT_REORG
#define TARGET_MACHINE_DEPENDENT_REORG ix86_reorg
-#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
-#define TARGET_BUILTIN_SETJMP_FRAME_VALUE ix86_builtin_setjmp_frame_value
-
#undef TARGET_BUILD_BUILTIN_VA_LIST
#define TARGET_BUILD_BUILTIN_VA_LIST ix86_build_builtin_va_list
diff --git a/gcc/config/pa/pa.md b/gcc/config/pa/pa.md
index 8308b37461df89200f35f6ec7dbee4798375b9ea..29dd24010536164e12559e7bf09479c02739d21d 100644
--- a/gcc/config/pa/pa.md
+++ b/gcc/config/pa/pa.md
@@ -6909,13 +6909,7 @@ (define_expand "nonlocal_goto"
emit_clobber (gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (VOIDmode)));
emit_clobber (gen_rtx_MEM (BLKmode, hard_frame_pointer_rtx));
- /* Restore the frame pointer. The virtual_stack_vars_rtx is saved
- instead of the hard_frame_pointer_rtx in the save area. As a
- result, an extra instruction is needed to adjust for the offset
- of the virtual stack variables and the hard frame pointer. */
- if (GET_CODE (fp) != REG)
- fp = force_reg (Pmode, fp);
- emit_move_insn (hard_frame_pointer_rtx, plus_constant (Pmode, fp, -8));
+ emit_move_insn (hard_frame_pointer_rtx, fp);
emit_stack_restore (SAVE_NONLOCAL, stack);
diff --git a/gcc/config/sparc/sparc.c b/gcc/config/sparc/sparc.c
index 6d52f83d91c2ac44337bb0c6a9ebc57957ed7578..0227a53861fb4dc820e183db7e7687317d4e711d 100644
--- a/gcc/config/sparc/sparc.c
+++ b/gcc/config/sparc/sparc.c
@@ -679,7 +679,6 @@ static void sparc_output_dwarf_dtprel (FILE *, int, rtx) ATTRIBUTE_UNUSED;
static void sparc_file_end (void);
static bool sparc_frame_pointer_required (void);
static bool sparc_can_eliminate (const int, const int);
-static rtx sparc_builtin_setjmp_frame_value (void);
static void sparc_conditional_register_usage (void);
static bool sparc_use_pseudo_pic_reg (void);
static void sparc_init_pic_reg (void);
@@ -878,9 +877,6 @@ char sparc_hard_reg_printed[8];
#undef TARGET_FRAME_POINTER_REQUIRED
#define TARGET_FRAME_POINTER_REQUIRED sparc_frame_pointer_required
-#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
-#define TARGET_BUILTIN_SETJMP_FRAME_VALUE sparc_builtin_setjmp_frame_value
-
#undef TARGET_CAN_ELIMINATE
#define TARGET_CAN_ELIMINATE sparc_can_eliminate
@@ -13003,14 +12999,6 @@ sparc_can_eliminate (const int from ATTRIBUTE_UNUSED, const int to)
return to == HARD_FRAME_POINTER_REGNUM || !sparc_frame_pointer_required ();
}
-/* Return the hard frame pointer directly to bypass the stack bias. */
-
-static rtx
-sparc_builtin_setjmp_frame_value (void)
-{
- return hard_frame_pointer_rtx;
-}
-
/* If !TARGET_FPU, then make the fp registers and fp cc regs fixed so that
they won't be allocated. */
diff --git a/gcc/config/vax/vax.c b/gcc/config/vax/vax.c
index 547a7e068e49655993670e9a465fbc23f447b959..9559ffb9bb44185adc9b9a124f8cffdb69ba5959 100644
--- a/gcc/config/vax/vax.c
+++ b/gcc/config/vax/vax.c
@@ -59,7 +59,6 @@ static rtx vax_function_arg (cumulative_args_t, machine_mode,
static void vax_function_arg_advance (cumulative_args_t, machine_mode,
const_tree, bool);
static rtx vax_struct_value_rtx (tree, int);
-static rtx vax_builtin_setjmp_frame_value (void);
static void vax_asm_trampoline_template (FILE *);
static void vax_trampoline_init (rtx, tree, rtx);
static poly_int64 vax_return_pops_args (tree, tree, poly_int64);
@@ -99,9 +98,6 @@ static HOST_WIDE_INT vax_starting_frame_offset (void);
#undef TARGET_STRUCT_VALUE_RTX
#define TARGET_STRUCT_VALUE_RTX vax_struct_value_rtx
-#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
-#define TARGET_BUILTIN_SETJMP_FRAME_VALUE vax_builtin_setjmp_frame_value
-
#undef TARGET_LRA_P
#define TARGET_LRA_P hook_bool_void_false
@@ -1067,12 +1063,6 @@ vax_struct_value_rtx (tree fntype ATTRIBUTE_UNUSED,
return gen_rtx_REG (Pmode, VAX_STRUCT_VALUE_REGNUM);
}
-static rtx
-vax_builtin_setjmp_frame_value (void)
-{
- return hard_frame_pointer_rtx;
-}
-
/* Worker function for NOTICE_UPDATE_CC. */
void
diff --git a/gcc/function.c b/gcc/function.c
index e30ee259becc1cffb312fa186339ffd86a60de1c..9a3e9795a9926f37eb9f6ddeb79a6610a42b3eb8 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -5142,7 +5142,7 @@ expand_function_start (tree subr)
r_save = expand_expr (t_save, NULL_RTX, VOIDmode, EXPAND_WRITE);
gcc_assert (GET_MODE (r_save) == Pmode);
- emit_move_insn (r_save, targetm.builtin_setjmp_frame_value ());
+ emit_move_insn (r_save, hard_frame_pointer_rtx);
update_nonlocal_goto_save_area ();
}
diff --git a/gcc/lra-eliminations.c b/gcc/lra-eliminations.c
index ee9fd51f129d2cef1a2766d6bdf0345f906cdf0c..fa0ebcd8ed9b7bdff460957ecba5b50250c796b8 100644
--- a/gcc/lra-eliminations.c
+++ b/gcc/lra-eliminations.c
@@ -876,32 +876,6 @@ mark_not_eliminable (rtx x, machine_mode mem_mode)
\f
-#ifdef HARD_FRAME_POINTER_REGNUM
-
-/* Search INSN's reg notes to see whether the destination is equal to
- WHAT + C for some constant C. Return true if so, storing C in
- *OFFSET_OUT and removing the reg note. */
-static bool
-remove_reg_equal_offset_note (rtx_insn *insn, rtx what, poly_int64 *offset_out)
-{
- rtx link, *link_loc;
-
- for (link_loc = ®_NOTES (insn);
- (link = *link_loc) != NULL_RTX;
- link_loc = &XEXP (link, 1))
- if (REG_NOTE_KIND (link) == REG_EQUAL
- && GET_CODE (XEXP (link, 0)) == PLUS
- && XEXP (XEXP (link, 0), 0) == what
- && poly_int_rtx_p (XEXP (XEXP (link, 0), 1), offset_out))
- {
- *link_loc = XEXP (link, 1);
- return true;
- }
- return false;
-}
-
-#endif
-
/* Scan INSN and eliminate all eliminable hard registers in it.
If REPLACE_P is true, do the replacement destructively. Also
@@ -939,72 +913,6 @@ eliminate_regs_in_insn (rtx_insn *insn, bool replace_p, bool first_p,
return;
}
- /* Check for setting an eliminable register. */
- if (old_set != 0 && REG_P (SET_DEST (old_set))
- && (ep = get_elimination (SET_DEST (old_set))) != NULL)
- {
- for (ep = reg_eliminate; ep < ®_eliminate[NUM_ELIMINABLE_REGS]; ep++)
- if (ep->from_rtx == SET_DEST (old_set) && ep->can_eliminate)
- {
- bool delete_p = replace_p;
-
-#ifdef HARD_FRAME_POINTER_REGNUM
- if (ep->from == FRAME_POINTER_REGNUM
- && ep->to == HARD_FRAME_POINTER_REGNUM)
- /* If this is setting the frame pointer register to the
- hardware frame pointer register and this is an
- elimination that will be done (tested above), this
- insn is really adjusting the frame pointer downward
- to compensate for the adjustment done before a
- nonlocal goto. */
- {
- rtx src = SET_SRC (old_set);
- poly_int64 offset = 0;
-
- /* We should never process such insn with non-zero
- UPDATE_SP_OFFSET. */
- lra_assert (known_eq (update_sp_offset, 0));
-
- if (remove_reg_equal_offset_note (insn, ep->to_rtx, &offset)
- || strip_offset (src, &offset) == ep->to_rtx)
- {
- if (replace_p)
- {
- SET_DEST (old_set) = ep->to_rtx;
- lra_update_insn_recog_data (insn);
- return;
- }
- offset -= (ep->offset - ep->previous_offset);
- src = plus_constant (Pmode, ep->to_rtx, offset);
-
- /* First see if this insn remains valid when we
- make the change. If not, keep the INSN_CODE
- the same and let the constraint pass fit it
- up. */
- validate_change (insn, &SET_SRC (old_set), src, 1);
- validate_change (insn, &SET_DEST (old_set),
- ep->from_rtx, 1);
- if (! apply_change_group ())
- {
- SET_SRC (old_set) = src;
- SET_DEST (old_set) = ep->from_rtx;
- }
- lra_update_insn_recog_data (insn);
- /* Add offset note for future updates. */
- add_reg_note (insn, REG_EQUAL, copy_rtx (src));
- return;
- }
- }
-#endif
-
- /* This insn isn't serving a useful purpose. We delete it
- when REPLACE is set. */
- if (delete_p)
- lra_delete_dead_insn (insn);
- return;
- }
- }
-
/* We allow one special case which happens to work on all machines we
currently support: a single set with the source or a REG_EQUAL
note being a PLUS of an eliminable register and a constant. */
diff --git a/gcc/reload1.c b/gcc/reload1.c
index 5039ceed5816dc5567274ff64e849c2c4f998bb4..3ad6f1d9c1f7a72c4c39ddbaa3e3caf3e903557e 100644
--- a/gcc/reload1.c
+++ b/gcc/reload1.c
@@ -3234,96 +3234,6 @@ eliminate_regs_in_insn (rtx_insn *insn, int replace)
return 0;
}
- if (old_set != 0 && REG_P (SET_DEST (old_set))
- && REGNO (SET_DEST (old_set)) < FIRST_PSEUDO_REGISTER)
- {
- /* Check for setting an eliminable register. */
- for (ep = reg_eliminate; ep < ®_eliminate[NUM_ELIMINABLE_REGS]; ep++)
- if (ep->from_rtx == SET_DEST (old_set) && ep->can_eliminate)
- {
- /* If this is setting the frame pointer register to the
- hardware frame pointer register and this is an elimination
- that will be done (tested above), this insn is really
- adjusting the frame pointer downward to compensate for
- the adjustment done before a nonlocal goto. */
- if (!HARD_FRAME_POINTER_IS_FRAME_POINTER
- && ep->from == FRAME_POINTER_REGNUM
- && ep->to == HARD_FRAME_POINTER_REGNUM)
- {
- rtx base = SET_SRC (old_set);
- rtx_insn *base_insn = insn;
- HOST_WIDE_INT offset = 0;
-
- while (base != ep->to_rtx)
- {
- rtx_insn *prev_insn;
- rtx prev_set;
-
- if (GET_CODE (base) == PLUS
- && CONST_INT_P (XEXP (base, 1)))
- {
- offset += INTVAL (XEXP (base, 1));
- base = XEXP (base, 0);
- }
- else if ((prev_insn = prev_nonnote_insn (base_insn)) != 0
- && (prev_set = single_set (prev_insn)) != 0
- && rtx_equal_p (SET_DEST (prev_set), base))
- {
- base = SET_SRC (prev_set);
- base_insn = prev_insn;
- }
- else
- break;
- }
-
- if (base == ep->to_rtx)
- {
- rtx src = plus_constant (Pmode, ep->to_rtx,
- offset - ep->offset);
-
- new_body = old_body;
- if (! replace)
- {
- new_body = copy_insn (old_body);
- if (REG_NOTES (insn))
- REG_NOTES (insn) = copy_insn_1 (REG_NOTES (insn));
- }
- PATTERN (insn) = new_body;
- old_set = single_set (insn);
-
- /* First see if this insn remains valid when we
- make the change. If not, keep the INSN_CODE
- the same and let reload fit it up. */
- validate_change (insn, &SET_SRC (old_set), src, 1);
- validate_change (insn, &SET_DEST (old_set),
- ep->to_rtx, 1);
- if (! apply_change_group ())
- {
- SET_SRC (old_set) = src;
- SET_DEST (old_set) = ep->to_rtx;
- }
-
- val = 1;
- goto done;
- }
- }
-
- /* In this case this insn isn't serving a useful purpose. We
- will delete it in reload_as_needed once we know that this
- elimination is, in fact, being done.
-
- If REPLACE isn't set, we can't delete this insn, but needn't
- process it since it won't be used unless something changes. */
- if (replace)
- {
- delete_dead_insn (insn);
- return 1;
- }
- val = 1;
- goto done;
- }
- }
-
/* We allow one special case which happens to work on all machines we
currently support: a single set with the source or a REG_EQUAL
note being a PLUS of an eliminable register and a constant. */
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr84521.c b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
new file mode 100644
index 0000000000000000000000000000000000000000..995a30223afd1401186c7eaf541f27606aed59b5
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
@@ -0,0 +1,54 @@
+/* { dg-require-effective-target indirect_jumps } */
+/* { dg-additional-options "-fomit-frame-pointer -fno-inline" } */
+
+extern void abort (void);
+
+void
+broken_longjmp (void *p)
+{
+ __builtin_longjmp (p, 1);
+}
+
+volatile int x = 256;
+void *volatile p = (void*)&x;
+void *volatile p1;
+
+void
+test (void)
+{
+ void *buf[5];
+ void *volatile q = p;
+
+ if (!__builtin_setjmp (buf))
+ broken_longjmp (buf);
+
+ /* Fails if stack pointer corrupted. */
+ if (p != q)
+ abort ();
+}
+
+void
+test2 (void)
+{
+ void *volatile q = p;
+ p1 = __builtin_alloca (x);
+ test ();
+
+ /* Fails if frame pointer corrupted. */
+ if (p != q)
+ abort ();
+}
+
+int
+main (void)
+{
+ void *volatile q = p;
+ test ();
+ test2 ();
+ /* Fails if stack pointer corrupted. */
+ if (p != q)
+ abort ();
+
+ return 0;
+}
+
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-06-19 13:06 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-14 13:17 [PATCH] Fix PR84521 Wilco Dijkstra
2019-01-10 13:03 ` Wilco Dijkstra
2019-05-28 19:05 Wilco Dijkstra
2019-06-17 23:04 ` Jeff Law
2019-06-18 0:58 ` Wilco Dijkstra
2019-06-18 1:10 ` Jeff Law
2019-06-18 16:27 ` Max Filippov
2019-06-18 17:07 ` Wilco Dijkstra
2019-06-18 23:36 ` Max Filippov
2019-06-18 23:53 ` Wilco Dijkstra
2019-06-19 4:08 ` Max Filippov
2019-06-19 13:06 ` Wilco Dijkstra
2019-06-18 16:47 ` H.J. Lu
2019-06-18 17:55 ` Andreas Schwab
2019-06-18 18:11 ` Wilco Dijkstra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).