diff mbox

Fix PR48542: reload register contents reuse crossing setjmp

Message ID alpine.BSF.2.00.1106152239550.62222@dair.pair.com
State New
Headers show

Commit Message

Hans-Peter Nilsson June 16, 2011, 3:46 a.m. UTC 
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.



brgds, H-P

Comments

Jeff Law June 16, 2011, 10:04 p.m. UTC | #1 
-----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-----
diff mbox

Patch

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);
 	}
     }