Andreas Schwab <sch...@linux-m68k.org> writes:
> This breaks gcc.dg/tls/opt-3.c, gcc.dg/tls/pr47715-3.c and
> gcc.dg/tls/struct-1.c on m68k:
>
> /daten/aranym/gcc/gcc-20171222/gcc/testsuite/gcc.dg/tls/opt-3.c:11:3:
> internal compiler error: in add_args_size_note, at rtlanal.c:2379
> 0xae7aa9 add_args_size_note(rtx_insn*, poly_int<1u, long>)
> ../../gcc/rtlanal.c:2379
> 0x7ea4ca fixup_args_size_notes(rtx_insn*, rtx_insn*, poly_int<1u, long>)
> ../../gcc/expr.c:4105
> 0x7f6a02 emit_single_push_insn
> ../../gcc/expr.c:4225
> 0x7fa412 emit_push_insn(rtx_def*, machine_mode, tree_node*, rtx_def*,
> unsigned int, int, rtx_def*, poly_int<1u, long>, rtx_def*, rtx_def*,
> int, rtx_def*, bool)
> ../../gcc/expr.c:4561
> 0x6b8976 store_one_arg
> ../../gcc/calls.c:5694
> 0x6c15b1 expand_call(tree_node*, rtx_def*, int)
> ../../gcc/calls.c:4030
> 0x7f0485 expand_expr_real_1(tree_node*, rtx_def*, machine_mode,
> expand_modifier, rtx_def**, bool)
> ../../gcc/expr.c:10927
> 0x6d6c97 expand_expr
> ../../gcc/expr.h:276
> 0x6d6c97 expand_call_stmt
> ../../gcc/cfgexpand.c:2690
> 0x6d6c97 expand_gimple_stmt_1
> ../../gcc/cfgexpand.c:3624
> 0x6d6c97 expand_gimple_stmt
> ../../gcc/cfgexpand.c:3790
> 0x6d8058 expand_gimple_tailcall
> ../../gcc/cfgexpand.c:3836
> 0x6d8058 expand_gimple_basic_block
> ../../gcc/cfgexpand.c:5774
> 0x6dd62e execute
> ../../gcc/cfgexpand.c:6403
Bah. Looks like I need to update my scripts to use --enable-tls,
since I'd ended up with emultls for the m68k targets.
I think the assert is catching a pre-existing bug here. If we pushed
a value that needs a call to something like __tls_get_addr, we ended
up with two different REG_ARGS_SIZE notes on the same instruction.
It seems to be OK for emit_single_push_insn to push something that
needs a call to __tls_get_addr:
/* We have to allow non-call_pop patterns for the case
of emit_single_push_insn of a TLS address. */
if (GET_CODE (pat) != PARALLEL)
return 0;
so I think the problem is in the way this is handled rather than the fact
that it occurs at all.
If we're pushing a value X that needs a call C to calculate, we'll
add REG_ARGS_SIZE notes to the pushes and pops for C as part of the
call sequence. Then emit_single_push_insn calls fixup_args_size_notes
on the whole push sequence (the calculation of X, including C,
and the push of X itself). This is where the double notes came from.
But emit_single_push_insn_1 adjusted stack_pointer_delta *before* the
push, so the notes added for C were relative to the situation after
the future push of X rather than before it.
Presumably this didn't matter in practice because the note added
second tended to trump the note added first. But code is allowed to
walk REG_NOTES without having to disregard secondary notes.
This patch seems to fix it for me, but I'm not sure how best to test it.
Thanks,
Richard
2017-12-23 Richard Sandiford <richard.sandif...@linaro.org>
gcc/
* expr.c (fixup_args_size_notes): Check that any existing
REG_ARGS_SIZE notes are correct, and don't try to re-add them.
(emit_single_push_insn_1): Move stack_pointer_delta adjustment to...
(emit_single_push_insn): ...here.
Index: gcc/expr.c
===================================================================
--- gcc/expr.c 2017-12-23 09:29:20.226338285 +0000
+++ gcc/expr.c 2017-12-23 09:29:45.783339673 +0000
@@ -4089,6 +4089,14 @@ fixup_args_size_notes (rtx_insn *prev, r
if (!NONDEBUG_INSN_P (insn))
continue;
+ /* We might have existing REG_ARGS_SIZE notes, e.g. when pushing
+ a call argument containing a TLS address that itself requires
+ a call to __tls_get_addr. The handling of stack_pointer_delta
+ in emit_single_push_insn is supposed to ensure that any such
+ notes are already correct. */
+ rtx note = find_reg_note (insn, REG_ARGS_SIZE, NULL_RTX);
+ gcc_assert (!note || known_eq (args_size, get_args_size (note)));
+
poly_int64 this_delta = find_args_size_adjust (insn);
if (known_eq (this_delta, 0))
{
@@ -4102,7 +4110,8 @@ fixup_args_size_notes (rtx_insn *prev, r
if (known_eq (this_delta, HOST_WIDE_INT_MIN))
saw_unknown = true;
- add_args_size_note (insn, args_size);
+ if (!note)
+ add_args_size_note (insn, args_size);
if (STACK_GROWS_DOWNWARD)
this_delta = -poly_uint64 (this_delta);
@@ -4126,7 +4135,6 @@ emit_single_push_insn_1 (machine_mode mo
rtx dest;
enum insn_code icode;
- stack_pointer_delta += PUSH_ROUNDING (GET_MODE_SIZE (mode));
/* If there is push pattern, use it. Otherwise try old way of throwing
MEM representing push operation to move expander. */
icode = optab_handler (push_optab, mode);
@@ -4213,6 +4221,14 @@ emit_single_push_insn (machine_mode mode
emit_single_push_insn_1 (mode, x, type);
+ /* Adjust stack_pointer_delta to describe the situation after the push
+ we just performed. Note that we must do this after the push rather
+ than before the push in case calculating X needs pushes and pops of
+ its own (e.g. if calling __tls_get_addr). The REG_ARGS_SIZE notes
+ for such pushes and pops must not include the effect of the future
+ push of X. */
+ stack_pointer_delta += PUSH_ROUNDING (GET_MODE_SIZE (mode));
+
last = get_last_insn ();
/* Notice the common case where we emitted exactly one insn. */
