Fix PR48542: reload register contents reuse crossing setjmp

Fix PR48542: reload register contents reuse crossing setjmp

[Hans-Peter Nilsson]

A generic bug found in one of the less common targets.  The MMIX
port usually saves the return-address from the special-register rJ to
a general call-saved register (helped by get_hard_reg_initial_val),
restored to rJ immediately on return from the call.  I know, the
"restore immediately on return" is odd and I don't remember exactly
why I did it that way and not in the "return" insn; I might change that,
but that's only incidental.  IRA dutifully refuses to allocate a
register for the return-address seeing that it's live across the setjmp,
but reload comes to the "rescue" and when fixing up the accesses to the
resulting stack-slot, reuses the reload register across the setjmp, so
the generated code looks just like an ordinary register allocation,
save for the sometimes-unused stack-slot (in sjtest, completely
unused; in sjtest3 just not used across the setjmp).  The test-case is
slightly changed from the submitted original to avoid the
miscompilation causing an endless loop and a test-timeout.

There are two fixes, because when register contents is invalidated for
the main path (the patch to reload1.c), there's fallback code (patch
to reload.c) that also iterates over code, and which also lacks proper
handling of setjmp-type calls.

You will see this bug on other targets if you're unlucky enough to get
a reuse of a register that held something live at the time of the
setjmp but which died and the register reused for something else some
time after the setjmp call but before the longjmp call, and the
original value is live again after the second setjmp return.  This
just naturally happens more often for MMIX (read: always when there's
a temporary held in a register like the loop counter in the
test-case), given that (set rJ (mem stack-slot)) is emitted after every
call and always needs a reload through a general register, and is always
reloaded from the same stack-slot.  A trivial attempt failed to come
up with a test-case that fails for x86_64.

When working on the test-case, I noticed IPA discovering the
noreturn-ness of (calls to) functions calling longjmp but which were
declared noinline; naturally I didn't want gcc to look at the called
function or assume anything of its contents.  Adding attribute weak
helped, but that's a kludge that LTO will (at some time) see through.
So, isn't there now a need for a "noipa" attribute?  (Better call it
"extern" or "foreign" or some other color to avoid leaking internal
terms.)  The attribute would create the effect of a function that gcc
doesn't inspect (or really, ignores knowledge at calls), so we can
keep having self-contained tests in a single-file now and for future
IPA-like improvements.

Regtested mmix all open branches and native x86_64-linux trunk.

Ok?
For all open branches (after native regtesting)?

gcc/testsuite:
	PR rtl-optimization/48542
	* gcc.dg/torture/pr48542.c: New test.

gcc:
	PR rtl-optimization/48542
	* reload.c (find_equiv_reg): Stop looking when finding a
	setjmp-type call.
	* reload1.c (reload_as_needed): Invalidate all reload
	registers when crossing a setjmp-type call.


Index: gcc/testsuite/gcc.dg/torture/pr48542.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr48542.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr48542.c	(revision 0)
@@ -0,0 +1,57 @@
+/* { dg-do run } */
+/* The return-address was clobbered.  */
+#include <stdlib.h>
+#include <setjmp.h>
+
+jmp_buf env;
+extern void sub(void);
+extern void sub3(void);
+int called;
+__attribute__ ((__noinline__))
+int sjtest()
+{
+  int i;
+  if (setjmp(env))
+    return 99;
+
+  for (i = 0; i < 10; i++)
+    sub();
+
+  longjmp(env, 1);
+}
+
+__attribute__ ((__noinline__))
+void sub(void)
+{
+  called++;
+}
+
+int called3;
+__attribute__ ((__noinline__))
+int sjtest3()
+{
+  int i;
+  if (setjmp(env))
+    return 42;
+
+  for (i = 0; i < 10; i++)
+    sub3();
+  return 0;
+}
+
+__attribute__ ((__noinline__))
+void sub3(void)
+{
+  called3++;
+  if (called3 == 10)
+    longjmp (env, 1);
+}
+
+int main(void)
+{
+  if (sjtest() != 99 || called != 10)
+    abort();
+  if (sjtest3() != 42 || called3 != 10)
+    abort();
+  exit (0);
+}
Index: gcc/reload.c
===================================================================
--- gcc/reload.c	(revision 174961)
+++ gcc/reload.c	(working copy)
@@ -6791,6 +6791,15 @@ find_equiv_reg (rtx goal, rtx insn, enum
 	  || num > PARAM_VALUE (PARAM_MAX_RELOAD_SEARCH_INSNS))
 	return 0;

+      /* Don't reuse register contents from before a setjmp-type
+	 function call; on the second return (from the longjmp) it
+	 might have been clobbered by a later reuse.  It doesn't
+	 seem worthwhile to actually go and see if it is actually
+	 reused even if that information would be readily available;
+	 just don't reuse it across the setjmp call.  */
+      if (CALL_P (p) && find_reg_note (p, REG_SETJMP, NULL_RTX))
+	return 0;
+
       if (NONJUMP_INSN_P (p)
 	  /* If we don't want spill regs ...  */
 	  && (! (reload_reg_p != 0
Index: gcc/reload1.c
===================================================================
--- gcc/reload1.c	(revision 174961)
+++ gcc/reload1.c	(working copy)
@@ -4844,6 +4844,13 @@ reload_as_needed (int live_known)
 	{
 	  AND_COMPL_HARD_REG_SET (reg_reloaded_valid, call_used_reg_set);
 	  AND_COMPL_HARD_REG_SET (reg_reloaded_valid, reg_reloaded_call_part_clobbered);
+
+	  /* If this is a call to a setjmp-type function, we must not
+	     reuse any reload reg contents across the call; that will
+	     just be clobbered by other uses of the register in later
+	     code, before the longjmp.  */
+	  if (find_reg_note (insn, REG_SETJMP, NULL_RTX))
+	    CLEAR_HARD_REG_SET (reg_reloaded_valid);
 	}
     }


brgds, H-P

Re: Fix PR48542: reload register contents reuse crossing setjmp

[Jeff Law]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/15/11 21:46, Hans-Peter Nilsson wrote:
> A generic bug found in one of the less common targets.  The MMIX
> port usually saves the return-address from the special-register rJ to
> a general call-saved register (helped by get_hard_reg_initial_val),
> restored to rJ immediately on return from the call.  I know, the
> "restore immediately on return" is odd and I don't remember exactly
> why I did it that way and not in the "return" insn; I might change that,
> but that's only incidental.  IRA dutifully refuses to allocate a
> register for the return-address seeing that it's live across the setjmp,
> but reload comes to the "rescue" and when fixing up the accesses to the
> resulting stack-slot, reuses the reload register across the setjmp, so
> the generated code looks just like an ordinary register allocation,
> save for the sometimes-unused stack-slot (in sjtest, completely
> unused; in sjtest3 just not used across the setjmp).  The test-case is
> slightly changed from the submitted original to avoid the
> miscompilation causing an endless loop and a test-timeout.
> 
> There are two fixes, because when register contents is invalidated for
> the main path (the patch to reload1.c), there's fallback code (patch
> to reload.c) that also iterates over code, and which also lacks proper
> handling of setjmp-type calls.
> 
> You will see this bug on other targets if you're unlucky enough to get
> a reuse of a register that held something live at the time of the
> setjmp but which died and the register reused for something else some
> time after the setjmp call but before the longjmp call, and the
> original value is live again after the second setjmp return.  This
> just naturally happens more often for MMIX (read: always when there's
> a temporary held in a register like the loop counter in the
> test-case), given that (set rJ (mem stack-slot)) is emitted after every
> call and always needs a reload through a general register, and is always
> reloaded from the same stack-slot.  A trivial attempt failed to come
> up with a test-case that fails for x86_64.
> 
> When working on the test-case, I noticed IPA discovering the
> noreturn-ness of (calls to) functions calling longjmp but which were
> declared noinline; naturally I didn't want gcc to look at the called
> function or assume anything of its contents.  Adding attribute weak
> helped, but that's a kludge that LTO will (at some time) see through.
> So, isn't there now a need for a "noipa" attribute?  (Better call it
> "extern" or "foreign" or some other color to avoid leaking internal
> terms.)  The attribute would create the effect of a function that gcc
> doesn't inspect (or really, ignores knowledge at calls), so we can
> keep having self-contained tests in a single-file now and for future
> IPA-like improvements.
> 
> Regtested mmix all open branches and native x86_64-linux trunk.
> 
> Ok?
> For all open branches (after native regtesting)?
> 
> gcc/testsuite:
> 	PR rtl-optimization/48542
> 	* gcc.dg/torture/pr48542.c: New test.
> 
> gcc:
> 	PR rtl-optimization/48542
> 	* reload.c (find_equiv_reg): Stop looking when finding a
> 	setjmp-type call.
> 	* reload1.c (reload_as_needed): Invalidate all reload
> 	registers when crossing a setjmp-type call.
OK.
Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJN+n4AAAoJEBRtltQi2kC7QzkH/RjqeAN+7a4jJxyUsgMBJ/1l
N3iLC0JlZ50WmKBeT0R358wuoO17+J/RfM3Im3qmLYlf/16g70DxfrkT+Bs/LtEm
3wuGyt67kJrSXagw/7+cF2JW5hNDPMJKq1fWZNnyJYudPmdLY3s3Iki/D21c95nI
NZeMRKzbNUVMFrilrjVKtcAtQj7UM/BHwNgOFen0Q3WlSwvVa5eB1xQqiHigjXwh
g03Vtax1iXaK/8ziQG7Ww7E0fgejCcfMFSTKIH1BK6mjmAjy1flGvYidlCSKmd9P
OrDJ7SQvPGeKrLzoymrMUJWw+EmMaOor8f7rW3xnwbkDtyBRgPyI0OUqESzGxcg=
=72ZM
-----END PGP SIGNATURE-----

Re: Fix PR48542: reload register contents reuse crossing setjmp

[Ulrich Weigand]

Jeff Law wrote:
> On 06/15/11 21:46, Hans-Peter Nilsson wrote:
> > 	PR rtl-optimization/48542
> > 	* reload.c (find_equiv_reg): Stop looking when finding a
> > 	setjmp-type call.
> > 	* reload1.c (reload_as_needed): Invalidate all reload
> > 	registers when crossing a setjmp-type call.
> OK.
> Jeff

I see that this went already in, but I'm wondering why this
change should be necessary.  As far as register use is
concerned, setjmp ought to behave just like a regular function:
if a register is call-clobbered, reload will not attempt to
use it across a function call (*any* function call) anyway;
but if the register is call-saved, setjmp really ought to
restore the old value, *both* times it returns, and so reuse
ought to be allowed ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: Fix PR48542: reload register contents reuse crossing setjmp

[Jeff Law]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/14/11 10:16, Ulrich Weigand wrote:
> Jeff Law wrote:
>> On 06/15/11 21:46, Hans-Peter Nilsson wrote:
>>> 	PR rtl-optimization/48542
>>> 	* reload.c (find_equiv_reg): Stop looking when finding a
>>> 	setjmp-type call.
>>> 	* reload1.c (reload_as_needed): Invalidate all reload
>>> 	registers when crossing a setjmp-type call.
>> OK.
>> Jeff
> 
> I see that this went already in, but I'm wondering why this
> change should be necessary.  As far as register use is
> concerned, setjmp ought to behave just like a regular function:
> if a register is call-clobbered, reload will not attempt to
> use it across a function call (*any* function call) anyway;
> but if the register is call-saved, setjmp really ought to
> restore the old value, *both* times it returns, and so reuse
> ought to be allowed ...
Good point.

Perhaps the mmix guys can chime in...

jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJOJLblAAoJEBRtltQi2kC7MooH/jtwOEUwgcQIcpwARVrw53z9
geVKsQopLPkAx8WAPNxxJqwpmD61laQN9ZPPWE2E4SvXiGrnp9uBZYliG64/AC/G
lmUPwjYJHICHzVfhewvS1nX0u93de9U0q8hLSqignJCA/FOjxwzr4BXe8rd0089Y
xf84bwaIECb1r0gaG/W5MQodzA0OwOHuRj3YLDd1EZYb4TZQf2XCsNVe1eDmDj4C
ddhCx187nITOZT1S1CvohI9aUenZxyt+qDEm7VmjZqI62lZqphiN2+caaVjzwlV1
TaVk6Beg+hdT0lqL92/7vUvcEZanzrdpWa1I22oXlTAK/01zVENZ6uOS8oi4s+A=
=BYni
-----END PGP SIGNATURE-----

Re: Fix PR48542: reload register contents reuse crossing setjmp

[Hans-Peter Nilsson]

On Thu, 14 Jul 2011, Ulrich Weigand wrote:
> Jeff Law wrote:
> > On 06/15/11 21:46, Hans-Peter Nilsson wrote:
> > > 	PR rtl-optimization/48542
> > > 	* reload.c (find_equiv_reg): Stop looking when finding a
> > > 	setjmp-type call.
> > > 	* reload1.c (reload_as_needed): Invalidate all reload
> > > 	registers when crossing a setjmp-type call.
> > OK.
> > Jeff
>
> I see that this went already in, but I'm wondering why this
> change should be necessary.  As far as register use is
> concerned, setjmp ought to behave just like a regular function:
> if a register is call-clobbered, reload will not attempt to
> use it across a function call (*any* function call) anyway;
> but if the register is call-saved, setjmp really ought to
> restore the old value, *both* times it returns, and so reuse
> ought to be allowed ...

First of all, if this changes (back) in reload, then it should
change in IRA too.

Right; you do have a point.  I *think* my excuse would be that I
stopped thinking further when I saw that IRA doesn't allocate
registers across setjmp.

Changing this will make a difference for architectures (ABIs)
that don't save call-saved registers in the prologue but rather
for each call.  Crazy and suboptimal?  Not if you use register
windows or a register stack where you just (automatically as
part of the call insn or sequence) update a register number or
pointer for the call "save".  So MMIX is one, I guess sparc and
ia64 could be others, it seems so at a glance.  It all depends
on their setjmp/longjmp implementations of course, I don't have
the whole picture.

If you change this, such ports will then have to handle calls to
setjmp differently to other functions; not call-save across
those setjmp calls or else the call-saved register will be
picked up instead of the one saved at setjmp (restored at
longjmp).

The difference would be better code across calls to setjmp (and
related secondary effects in the setjmp-calling function) for
everyone else, but what is in place with the committed patch now
*is* consistent and correct and not needing special handling
regarding register-saving, AFAICT.

brgds, H-P

Re: Fix PR48542: reload register contents reuse crossing setjmp

[Ulrich Weigand]

Hans-Peter Nilsson wrote:
> On Thu, 14 Jul 2011, Ulrich Weigand wrote:
> > I see that this went already in, but I'm wondering why this
> > change should be necessary.  As far as register use is
> > concerned, setjmp ought to behave just like a regular function:
> > if a register is call-clobbered, reload will not attempt to
> > use it across a function call (*any* function call) anyway;
> > but if the register is call-saved, setjmp really ought to
> > restore the old value, *both* times it returns, and so reuse
> > ought to be allowed ...
> 
> First of all, if this changes (back) in reload, then it should
> change in IRA too.
> 
> Right; you do have a point.  I *think* my excuse would be that I
> stopped thinking further when I saw that IRA doesn't allocate
> registers across setjmp.
> 
> Changing this will make a difference for architectures (ABIs)
> that don't save call-saved registers in the prologue but rather
> for each call.  Crazy and suboptimal?  Not if you use register
> windows or a register stack where you just (automatically as
> part of the call insn or sequence) update a register number or
> pointer for the call "save".  So MMIX is one, I guess sparc and
> ia64 could be others, it seems so at a glance.  It all depends
> on their setjmp/longjmp implementations of course, I don't have
> the whole picture.
> 
> If you change this, such ports will then have to handle calls to
> setjmp differently to other functions; not call-save across
> those setjmp calls or else the call-saved register will be
> picked up instead of the one saved at setjmp (restored at
> longjmp).

Well, the way I see it is that "setjmp" ought to be using the
same ABI as regular function calls, whatever that is on any
platform.  The implementation of setjmp/longjmp then has to
take care to set up things so that, on the second return from
setjmp, the caller does not notice any differences, so that it
can use the same code to implement the call to setjmp as it
would to implement any other calls.

On many systems, this simply involves restoring the values of
all call-saved registers.  On systems with more complex ABI,
this might instead involve more complex operations like fiddlling
with a register stack / register windows to set them up so they
look as required to continue with a regular function return.

But whatever it is, all that "weirdness" ought to be confined
to within the implementation of setjmp / longjmp.


That said, you are right that there is existing code in GCC
that treats "setjmp" specially.  This is not just IRA, but in
fact all over the place: CSE, regstat, scheduler, ...

It appears all this is to handle the problem that at least on
some platforms, setjmp seems to violate the rules I thought
it was supposed to follow.  See e.g. the comment in CSE:

  /* Don't cse over a call to setjmp; on some machines (eg VAX)
     the regs restored by the longjmp come from a later time
     than the setjmp.  */

Now this is IMHO a bit unfortunate.  However, I agree that if
we do treat setjmp differently in some places, we need to do
so *everywhere*.

Given that, I withdraw my objection to the reload patch.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com