PATCH COMMITTED: Fix prefetch with -fdelete-null-pointer-checks

[Ian Lance Taylor-3]

Xinliang David Li discovered a compiler bug compiling the Linux
kernel.  The kernel code has a loop like this:

 for (node = (head)->first; node && ({ prefetch(node->next); 1; }); node = node->next) {

where prefetch is:

static inline __attribute__((always_inline)) void prefetch(void *x)
{
 asm volatile("prefetcht0 %0" :: "m" (*(unsigned long *)x));
}

The bug is that the -fdelete-null-pointer-checks optimization thinks
that the prefetch is a memory load, and that therefore gcc can delete
the test of "node" in the loop.  The effect is that the kernel starts
loading from address 0 and bad things happen.  I'm sure David had a
lot of fun tracking down the miscompilation.

This problem can only happen with asm statements.  It can not happen
with __builtin_prefetch, as in that case the compiler knows that it is
passing an address, not dereferencing one.

The kernel code could be fixed by writing the asm statement as
  asm volatile("prefetcht0 %a0" :: "p" (*(unsigned long *)x));
But this is still clearly a bug in the compiler.  The fix is simply to
ignore asm statements for the purposes of detecting loads and stores
for -fdelete-null-pointer-checks.  That is appropriate in all cases,
not just prefetch, as there is no guarantee that a given asm statement
actually executes any given operand, and thus the can not assume that
the null pointer was checked merely because it is passed to an asm
statement.

I've bootstrapped and tested the appended patch on i686-linux-gnu.  I
committed the patch to trunk and to the 4.3 branch.  There is no open
bug for this, but it is a regression.

The test case is, naturally, i386 specific, as it requires an asm
statement, so I put it in gcc.target/i386.

Ian


gcc/ChangeLog:
2008-07-23  Ian Lance Taylor  <iant@...>

        * tree-vrp.c (infer_value_range): Ignore asm statements when
        looking for memory accesses for -fdelete-null-pointer-checks.

gcc/testsuite/ChangeLog:
2008-07-23  Ian Lance Taylor  <iant@...>

        * gcc.target/i386/20080723-1.c: New test.



Index: tree-vrp.c
===================================================================
--- tree-vrp.c (revision 138106)
+++ tree-vrp.c (revision 138107)
@@ -3456,7 +3456,9 @@ infer_value_range (tree stmt, tree op, e
 
   /* We can only assume that a pointer dereference will yield
      non-NULL if -fdelete-null-pointer-checks is enabled.  */
-  if (flag_delete_null_pointer_checks && POINTER_TYPE_P (TREE_TYPE (op)))
+  if (flag_delete_null_pointer_checks
+      && POINTER_TYPE_P (TREE_TYPE (op))
+      && TREE_CODE (stmt) != ASM_EXPR)
     {
       unsigned num_uses, num_loads, num_stores;
 
Index: testsuite/gcc.target/i386/20080723-1.c
===================================================================
--- testsuite/gcc.target/i386/20080723-1.c (revision 0)
+++ testsuite/gcc.target/i386/20080723-1.c (revision 138107)
@@ -0,0 +1,49 @@
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+extern void abort (void);
+extern void exit (int);
+
+static inline __attribute__((always_inline))
+void
+prefetch (void *x)
+{
+  asm volatile("prefetcht0 %0" : : "m" (*(unsigned long *)x));
+}
+
+struct hlist_head
+{
+  struct hlist_node *first;
+};
+
+struct hlist_node
+{
+  struct hlist_node *next;
+  unsigned long i_ino;
+};
+
+struct hlist_node * find_inode_fast(struct hlist_head *head, unsigned long ino)
+{
+  struct hlist_node *node;
+
+  for (node = head->first;
+       node && (prefetch (node->next), 1);
+       node = node->next)
+    {
+      if (node->i_ino == ino)
+ break;
+    }
+  return node ? node : 0;
+}
+
+struct hlist_node g2;
+struct hlist_node g1 = { &g2 };
+struct hlist_head h = { &g1 };
+
+int
+main()
+{
+  if (find_inode_fast (&h, 1) != 0)
+    abort ();
+  exit (0);
+}