[PATCH] Fix asm goto miscompilation of Linux kernel (PR middle-end/58670)

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

The following testcase distilled from Linux kernel is miscompiled,
in *.optimized we end up with asm goto whose only label points to
the fallthru basic block.  During expansion, we unfortunately
put further instructions after it into the same basic block
(one in expand_gimple_block:
  /* Expanded RTL can create a jump in the last instruction of block.
     This later might be assumed to be a jump to successor and break edge insertion.
     We need to insert dummy move to prevent this. PR41440. */
  if (single_succ_p (bb)
      && (single_succ_edge (bb)->flags & EDGE_FALLTHRU)
      && (last = get_last_insn ())
      && JUMP_P (last))
    {
      rtx dummy = gen_reg_rtx (SImode);
      emit_insn_after_noloc (gen_move_insn (dummy, dummy), last, NULL);
    }
and another one an unconditional jump to the fallthru basic block),
then commit any edge insertions (where we have a pseudo initialization
from constant for a PHI queued for the fallthru edge) and only afterwards
do find_many_sub_basic_blocks.  The edge insertion sees an unconditional
jump as the last insn in that bb, thus happily inserts the assignment
right before the unconditional jump, but then the jump out of the
asm goto will bypass that.  The easiest fix for this I came up with
is just special case labels pointing to the fallthru basic block, rather
than letting them point at the fallthru's bb label and let there be
extra insns in between asm goto and another branch to that label
in the fallthru path the patch emits an extra CODE_LABEL right after
the asm goto and redirects the labels there.  Commit edge insertions
then does the right thing and find_many_sub_basic_blocks just splits
the bb after the asm goto and before the label (which it would do anyway
even if there is no label).

Also, just from code inspection, don't have a testcase for that,
if commit_one_edge_insertion is called with asm goto being the last
insn in a bb, it could happily emit the insertions before asm goto, which is
wrong.  IMHO emitting those before the jump is only safe if it is
simplejump_p.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk and 4.8?

2013-10-10  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/58670
	* stmt.c (expand_asm_operands): Add FALLTHRU_BB argument,
	if any labels are in FALLTHRU_BB, use a special label emitted
	immediately after the asm goto insn rather than label_rtx
	of the LABEL_DECL.
	(expand_asm_stmt): Adjust caller.
	* cfgrtl.c (commit_one_edge_insertion): Force splitting of
	edge if the last insn in predecessor is a jump with single successor,
	but it isn't simplejump_p.

	* gcc.dg/torture/pr58670.c: New test.

--- gcc/stmt.c.jj	2013-10-01 15:45:29.000000000 +0200
+++ gcc/stmt.c	2013-10-10 08:28:03.055465091 +0200
@@ -612,6 +612,9 @@ tree_conflicts_with_clobbers_p (tree t,
    CLOBBERS is a list of STRING_CST nodes each naming a hard register
    that is clobbered by this insn.
 
+   LABELS is a list of labels, and if LABELS is non-NULL, FALLTHRU_BB
+   should be the fallthru basic block of the asm goto.
+
    Not all kinds of lvalue that may appear in OUTPUTS can be stored directly.
    Some elements of OUTPUTS may be replaced with trees representing temporary
    values.  The caller should copy those temporary values to the originally
@@ -621,7 +624,8 @@ tree_conflicts_with_clobbers_p (tree t,
 
 static void
 expand_asm_operands (tree string, tree outputs, tree inputs,
-		     tree clobbers, tree labels, int vol, location_t locus)
+		     tree clobbers, tree labels, basic_block fallthru_bb,
+		     int vol, location_t locus)
 {
   rtvec argvec, constraintvec, labelvec;
   rtx body;
@@ -642,6 +646,7 @@ expand_asm_operands (tree string, tree o
   enum machine_mode *inout_mode = XALLOCAVEC (enum machine_mode, noutputs);
   const char **constraints = XALLOCAVEC (const char *, noutputs + ninputs);
   int old_generating_concat_p = generating_concat_p;
+  rtx fallthru_label = NULL_RTX;
 
   /* An ASM with no outputs needs to be treated as volatile, for now.  */
   if (noutputs == 0)
@@ -942,8 +947,24 @@ expand_asm_operands (tree string, tree o
 
   /* Copy labels to the vector.  */
   for (i = 0, tail = labels; i < nlabels; ++i, tail = TREE_CHAIN (tail))
-    ASM_OPERANDS_LABEL (body, i)
-      = gen_rtx_LABEL_REF (Pmode, label_rtx (TREE_VALUE (tail)));
+    {
+      rtx r;
+      /* If asm goto has any labels in the fallthru basic block, use
+	 a label that we emit immediately after the asm goto.  Expansion
+	 may insert further instructions into the same basic block after
+	 asm goto and if we don't do this, insertion of instructions on
+	 the fallthru edge might misbehave.  See PR58670.  */
+      if (fallthru_bb
+	  && label_to_block_fn (cfun, TREE_VALUE (tail)) == fallthru_bb)
+	{
+	  if (fallthru_label == NULL_RTX)
+	    fallthru_label = gen_label_rtx ();
+	  r = fallthru_label;
+	}
+      else
+	r = label_rtx (TREE_VALUE (tail));
+      ASM_OPERANDS_LABEL (body, i) = gen_rtx_LABEL_REF (Pmode, r);
+    }
 
   generating_concat_p = old_generating_concat_p;
 
@@ -1067,6 +1088,9 @@ expand_asm_operands (tree string, tree o
 	emit_insn (body);
     }
 
+  if (fallthru_label)
+    emit_label (fallthru_label);
+
   /* For any outputs that needed reloading into registers, spill them
      back to where they belong.  */
   for (i = 0; i < noutputs; ++i)
@@ -1087,6 +1111,7 @@ expand_asm_stmt (gimple stmt)
   const char *s;
   tree str, out, in, cl, labels;
   location_t locus = gimple_location (stmt);
+  basic_block fallthru_bb = NULL;
 
   /* Meh... convert the gimple asm operands into real tree lists.
      Eventually we should make all routines work on the vectors instead
@@ -1122,6 +1147,9 @@ expand_asm_stmt (gimple stmt)
   n = gimple_asm_nlabels (stmt);
   if (n > 0)
     {
+      edge fallthru = find_fallthru_edge (gimple_bb (stmt)->succs);
+      if (fallthru)
+	fallthru_bb = fallthru->dest;
       t = labels = gimple_asm_label_op (stmt, 0);
       for (i = 1; i < n; i++)
 	t = TREE_CHAIN (t) = gimple_asm_label_op (stmt, i);
@@ -1147,7 +1175,7 @@ expand_asm_stmt (gimple stmt)
 
   /* Generate the ASM_OPERANDS insn; store into the TREE_VALUEs of
      OUTPUTS some trees for where the values were actually stored.  */
-  expand_asm_operands (str, outputs, in, cl, labels,
+  expand_asm_operands (str, outputs, in, cl, labels, fallthru_bb,
 		       gimple_asm_volatile_p (stmt), locus);
 
   /* Copy all the intermediate outputs into the specified outputs.  */
--- gcc/cfgrtl.c.jj	2013-09-30 22:13:56.000000000 +0200
+++ gcc/cfgrtl.c	2013-10-10 09:02:53.143817967 +0200
@@ -1959,10 +1959,18 @@ commit_one_edge_insertion (edge e)
     }
 
   /* If the source has one successor and the edge is not abnormal,
-     insert there.  Except for the entry block.  */
+     insert there.  Except for the entry block.
+     Don't do this if the predecessor ends in a jump other than
+     unconditional simple jump.  E.g. for asm goto that points all
+     its labels at the fallthru basic block, we can't insert instructions
+     before the asm goto, as the asm goto can have various of side effects,
+     and can't emit instructions after the asm goto, as it must end
+     the basic block.  */
   else if ((e->flags & EDGE_ABNORMAL) == 0
 	   && single_succ_p (e->src)
-	   && e->src != ENTRY_BLOCK_PTR)
+	   && e->src != ENTRY_BLOCK_PTR
+	   && (!JUMP_P (BB_END (e->src))
+	       || simplejump_p (BB_END (e->src))))
     {
       bb = e->src;
 
--- gcc/testsuite/gcc.dg/torture/pr58670.c.jj	2013-10-10 09:17:32.082303415 +0200
+++ gcc/testsuite/gcc.dg/torture/pr58670.c	2013-10-10 09:23:19.446521098 +0200
@@ -0,0 +1,47 @@
+/* PR middle-end/58670 */
+/* { dg-do run { target i?86-*-* x86_64-*-* } } */
+
+#if defined (__i386__) || defined (__x86_64__)
+#define ASM_STR "bts $1, %0; jc %l[lab]"
+#endif
+
+__attribute__((noinline, noclone)) int
+foo (int a, int b)
+{
+  if (a)
+    return -3;
+#ifdef ASM_STR
+  asm volatile goto (ASM_STR : : "m" (b) : "memory" : lab);
+  return 0;
+lab:
+#endif
+  return 0;
+}
+
+int
+bar (int a, int b)
+{
+  if (a)
+    return -3;
+#ifdef ASM_STR
+  asm volatile goto (ASM_STR : : "m" (b) : "memory" : lab);
+  return 0;
+lab:
+#endif
+  return 0;
+}
+
+int
+main ()
+{
+  if (foo (1, 0) != -3
+      || foo (0, 3) != 0
+      || foo (1, 0) != -3
+      || foo (0, 0) != 0
+      || bar (1, 0) != -3
+      || bar (0, 3) != 0
+      || bar (1, 0) != -3
+      || bar (0, 0) != 0)
+    __builtin_abort ();
+  return 0;
+}

	Jakub