diff mbox series

[committed] Fix rl78 segfault

Message ID 984cddb1-768c-e81d-3556-50d5c36fa6ba@redhat.com
State New
Headers show
Series [committed] Fix rl78 segfault | expand

Commit Message

Jeff Law Oct. 25, 2018, 7:33 p.m. UTC 
Whee, this was fun to debug...

The lr78 port is crippled enough that it has to have its own specialized
register allocation phase.  Essentially all insns have two
implementations, one is virtual and used up through the target specific
register allocation pass and a physical implementation used thereafter.

Which of the two modes is in effect is controlled by a couple variables.


During the rl78 register allocation phase we query each insn to see if
it's going to need target specific register allocation.  This involves
trying to recognize the insn in physical mode then verifying it matches
its constraints.  If both are successful, then the insn needs no target
specific register allocation.

If the insn is not recognized in physical mode it is then re-recognized
in virtual mode to set up recog_operands.  If the insn is recognized in
physical mode, but does not match its contraints, then the insn is not
re-recognized in virtual mode.

Thus, if the insn was recognized in physical mode, but did not satisfy
its constraints, then recog_data still reflects the state from
recognition in physical mode.  This is vitally important as some insns
have fewer operands in physical mode than virtual mode (conditional
branches for example).


When we later start to examine the operands for target specific register
allocation, we can read bogus data out of recog_data, and if operand_loc
in particular contains a bogus pointer, then we'll segfault.  Which is
precisely what happened in my tester while building newlib.

This change twiddles insn_ok_now so that in all cases where it's going
to return false it always re-recognizes the insn in virtual mode before
returning.  Naturally this fixes the problem and allows newlib to build
to completion.

The latent bug was likely triggered by the recent combiner changes, but
I didn't bother to bisect it since once I started looking at what was
happening in the port it was clear something was broken.

The diff looks much larger than it really is due to indention changes.

Installing on the trunk momentarily...

Jeff
* config/rl78/rl78.c (insn_ok_now): Always re-recognize the insn
	if returning false.
diff mbox series

Patch

diff --git a/gcc/config/rl78/rl78.c b/gcc/config/rl78/rl78.c
index 6663e355834..7986e89e3d8 100644
--- a/gcc/config/rl78/rl78.c
+++ b/gcc/config/rl78/rl78.c
@@ -2736,38 +2736,44 @@  insn_ok_now (rtx_insn * insn)
 	    if (GET_CODE (OP (i)) == MEM
 		&& GET_MODE (XEXP (OP (i), 0)) == SImode
 		&& GET_CODE (XEXP (OP (i), 0)) != UNSPEC)
-	      return false;
+	      goto not_ok;
 
 	  return true;
 	}
     }
-  else
-    {
-      /* We need to re-recog the insn with virtual registers to get
-	 the operands.  */
-      cfun->machine->virt_insns_ok = 1;
-      if (recog (pattern, insn, 0) > -1)
-	{
-	  extract_insn (insn);
-	  if (constrain_operands (0, get_preferred_alternatives (insn)))
-	    {
-	      cfun->machine->virt_insns_ok = 0;
-	      return false;
-	    }
-	}
 
-#if DEBUG_ALLOC
-      fprintf (stderr, "\033[41;30m Unrecognized *virtual* insn \033[0m\n");
-      debug_rtx (insn);
-#endif
-      gcc_unreachable ();
-    }
+  /* INSN is not OK as-is.  It may not be recognized in real mode or
+     it might not have satisfied its constraints in real mode.  Either
+     way it will require fixups.
+
+     It is vital we always re-recognize at this point as some insns
+     have fewer operands in real mode than virtual mode.  If we do
+     not re-recognize, then the recog_data will refer to real mode
+     operands and we may read invalid data.  Usually this isn't a
+     problem, but once in a while the data we read is bogus enough
+     to cause a segfault or other undesirable behavior.  */
+ not_ok:
+
+  /* We need to re-recog the insn with virtual registers to get
+     the operands.  */
+    INSN_CODE (insn) = -1;
+    cfun->machine->virt_insns_ok = 1;
+    if (recog (pattern, insn, 0) > -1)
+      {
+	extract_insn (insn);
+	/* In theory this should always be true.  */
+	if (constrain_operands (0, get_preferred_alternatives (insn)))
+	  {
+	    cfun->machine->virt_insns_ok = 0;
+	    return false;
+	  }
+      }
 
 #if DEBUG_ALLOC
-  fprintf (stderr, "\033[31m");
+  fprintf (stderr, "\033[41;30m Unrecognized *virtual* insn \033[0m\n");
   debug_rtx (insn);
-  fprintf (stderr, "\033[0m");
 #endif
+  gcc_unreachable ();
   return false;
 }