RFA: Fix REG_ARGS_SIZE handling when pushing TLS addresses

Message ID 87h8saio7a.fsf_-_@linaro.org
State New
Headers show
Series
  • RFA: Fix REG_ARGS_SIZE handling when pushing TLS addresses
Related show

Commit Message

Richard Sandiford Dec. 28, 2017, 8:37 p.m.
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.

Comments

Jeff Law Jan. 2, 2018, 7:07 p.m. | #1
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

Patch

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.  */