Hi
On 31/07/18 22:48, Andrew Pinski wrote:
On Tue, Jul 31, 2018 at 2:43 PM James Greenhalgh
<james.greenha...@arm.com> wrote:
On Thu, Jul 12, 2018 at 12:01:09PM -0500, Sudakshina Das wrote:
Hi Eric
On 27/06/18 12:22, Wilco Dijkstra wrote:
Eric Botcazou wrote:
This test can easily be changed not to use optimize since it doesn't look
like it needs it. We really need to tests these builtins properly,
otherwise they will continue to fail on most targets.
As far as I can see PR target/84521 has been reported only for Aarch64 so I'd
just leave the other targets alone (and avoid propagating FUD if possible).
It's quite obvious from PR84521 that this is an issue affecting all targets.
Adding better generic tests for __builtin_setjmp can only be a good thing.
Wilco
This conversation seems to have died down and I would like to
start it again. I would agree with Wilco's suggestion about
keeping the test in the generic folder. I have removed the
optimize attribute and the effect is still the same. It passes
on AArch64 with this patch and it currently fails on x86
trunk (gcc version 9.0.0 20180712 (experimental) (GCC))
on -O1 and above.
I don't see where the FUD comes in here; either this builtin has a defined
semantics across targets and they are adhered to, or the builtin doesn't have
well defined semantics, or the targets fail to implement those semantics.
The problem comes from the fact the builtins are not documented at all.
See PR59039 for the issue on them not being documented.
Thanks @James for bringing this up again.
I tried to revive the conversation on PR59039 while working on this as
well but that conversation mainly focused on documenting if we are
allowed to use __builtin_setjmp and __builtin_longjmp on the same
function and with the same jmp buffer or not. This patch and this test
case however does not involve that issue. There are other holes in the
documentation/implementation of these builtins. For now as advised by
James, I have posted the test case on the PR. I personally don't see why
this test case should go on the AArch64 tests when it clearly fails on
other targets as well. But if we can not come to an agreement on that, I
am willing to move it to AArch64 tests and maybe open a new bug report
which is not marked as "target" with the same test case.
Thanks
Sudi
Thanks,
Andrew
I think this should go in as is. If other targets are unhappy with the
failing test they should fix their target or skip the test if it is not
appropriate.
You may want to CC some of the maintainers of platforms you know to fail as
a courtesy on the PR (add your testcase, and add failing targets and their
maintainers to that PR) before committing so it doesn't come as a complete
surprise.
This is OK with some attempt to get target maintainers involved in the
conversation before commit.
Thanks,
James
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index f284e74..9792d28 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -473,7 +473,9 @@ extern unsigned aarch64_architecture_version;
#define EH_RETURN_STACKADJ_RTX gen_rtx_REG (Pmode, R4_REGNUM)
#define EH_RETURN_HANDLER_RTX aarch64_eh_return_handler_rtx ()
-/* Don't use __builtin_setjmp until we've defined it. */
+/* Don't use __builtin_setjmp until we've defined it.
+ CAUTION: This macro is only used during exception unwinding.
+ Don't fall for its name. */
#undef DONT_USE_BUILTIN_SETJMP
#define DONT_USE_BUILTIN_SETJMP 1
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 01f35f8..4266a3d 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3998,7 +3998,7 @@ static bool
aarch64_needs_frame_chain (void)
{
/* Force a frame chain for EH returns so the return address is at FP+8. */
- if (frame_pointer_needed || crtl->calls_eh_return)
+ if (frame_pointer_needed || crtl->calls_eh_return ||
cfun->has_nonlocal_label)
return true;
/* A leaf function cannot have calls or write LR. */
@@ -12218,6 +12218,13 @@ aarch64_expand_builtin_va_start (tree valist, rtx
nextarg ATTRIBUTE_UNUSED)
expand_expr (t, const0_rtx, VOIDmode, EXPAND_NORMAL);
}
+/* Implement TARGET_BUILTIN_SETJMP_FRAME_VALUE. */
+static rtx
+aarch64_builtin_setjmp_frame_value (void)
+{
+ return hard_frame_pointer_rtx;
+}
+
/* Implement TARGET_GIMPLIFY_VA_ARG_EXPR. */
static tree
@@ -17744,6 +17751,9 @@ aarch64_run_selftests (void)
#undef TARGET_FOLD_BUILTIN
#define TARGET_FOLD_BUILTIN aarch64_fold_builtin
+#undef TARGET_BUILTIN_SETJMP_FRAME_VALUE
+#define TARGET_BUILTIN_SETJMP_FRAME_VALUE aarch64_builtin_setjmp_frame_value
+
#undef TARGET_FUNCTION_ARG
#define TARGET_FUNCTION_ARG aarch64_function_arg
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index a014a01..d5f33d8 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -6087,6 +6087,30 @@
DONE;
})
+;; This is broadly similar to the builtins.c except that it uses
+;; temporaries to load the incoming SP and FP.
+(define_expand "nonlocal_goto"
+ [(use (match_operand 0 "general_operand"))
+ (use (match_operand 1 "general_operand"))
+ (use (match_operand 2 "general_operand"))
+ (use (match_operand 3 "general_operand"))]
+ ""
+{
+ rtx label_in = copy_to_reg (operands[1]);
+ rtx fp_in = copy_to_reg (operands[3]);
+ rtx sp_in = copy_to_reg (operands[2]);
+
+ emit_move_insn (hard_frame_pointer_rtx, fp_in);
+ emit_stack_restore (SAVE_NONLOCAL, sp_in);
+
+ emit_use (hard_frame_pointer_rtx);
+ emit_use (stack_pointer_rtx);
+
+ emit_indirect_jump (label_in);
+
+ DONE;
+})
+
;; Helper for aarch64.c code.
(define_expand "set_clobber_cc"
[(parallel [(set (match_operand 0)
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 0000000..564ef14
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr84521.c
@@ -0,0 +1,53 @@
+/* { dg-require-effective-target indirect_jumps } */
+
+#include <string.h>
+#include <alloca.h>
+#include <setjmp.h>
+
+jmp_buf buf;
+
+int uses_longjmp (void)
+{
+ jmp_buf buf2;
+ memcpy (buf2, buf, sizeof (buf));
+ __builtin_longjmp (buf2, 1);
+}
+
+int gl;
+void after_longjmp (void)
+{
+ gl = 5;
+}
+
+int
+test_1 (int n)
+{
+ volatile int *p = alloca (n);
+ if (__builtin_setjmp (buf))
+ {
+ after_longjmp ();
+ }
+ else
+ {
+ uses_longjmp ();
+ }
+
+ return 0;
+}
+
+int
+test_2 (int n)
+{
+ int i;
+ int *ptr = (int *)__builtin_alloca (sizeof (int) * n);
+ for (i = 0; i < n; i++)
+ ptr[i] = i;
+ test_1 (n);
+ return 0;
+}
+
+int main (int argc, const char **argv)
+{
+ __builtin_memset (&buf, 0xaf, sizeof (buf));
+ test_2 (100);
+}
