| Message ID | 87h8saio7a.fsf_-_@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | RFA: Fix REG_ARGS_SIZE handling when pushing TLS addresses | expand |
On 12/28/2017 01:37 PM, Richard Sandiford wrote: > Andreas Schwab <schwab@linux-m68k.org> writes: >> On Dez 23 2017, Richard Sandiford <richard.sandiford@linaro.org> wrote: >>> 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. >> >> Successfully regtested on m68k-linux. > > Thanks. Now also tested on aarch64-linux-gnu, x86_64-linux-gnu and > powerpc64-linux-gnu (not that that will give mucn coverage). Also > tested with a before-and-after comparison of testsuite output for > a range of targets. OK to install? > > Richard > > > The new assert in add_args_size_note triggered for gcc.dg/tls/opt-3.c > and other on m68k. This looks like a pre-existing bug: 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 bug 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. > > 2017-12-23 Richard Sandiford <richard.sandiford@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. OK. jeff
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. */