Disallow a global address in the x86-64 displacement field.

This applies only to %r15 sandboxed memory references.  The problem
is that if the index register is negative, the sandboxing operation
will cause the index to become a large positive 32-bit value, which
combined with the displacement, will overflow and try to reference
memory outside the sandbox.  This situation may legitimately occur
if the compiler happens to construct a (constant) interior pointer
to the middle of the global struct/array, and then dereferences it
with a variable offset.

After this fix, pnacl/scripts/testsuite_known_failures_pnacl.txt can
be updated to remove the "aha x86-64" known failure.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3517
R=eliben@chromium.org

Review URL: https://codereview.chromium.org/17987002

2 files changed, 104 insertions, 10 deletions

diff --git a/lib/Target/X86/X86ISelDAGToDAG.cpp b/lib/Target/X86/X86ISelDAGToDAG.cpp
index 8df2e7ca2e..7ecdf9eb72 100644
--- a/lib/Target/X86/X86ISelDAGToDAG.cpp
+++ b/lib/Target/X86/X86ISelDAGToDAG.cpp
@@ -79,6 +79,24 @@ namespace {
       return GV != 0 || CP != 0 || ES != 0 || JT != -1 || BlockAddr != 0;
     }
 
+    // @LOCALMOD-BEGIN
+    /// clearSymbolicDisplacement - Remove all sources of symbolic
+    /// constant displacement from the addressing mode.  This is
+    /// needed when the symbolic constant is pulled out of the address
+    /// computation, e.g. when
+    ///   ... DISP(%r15,%rax,1) ...
+    /// is replaced with
+    ///   lea DISP(%rax),%tmp
+    ///   ... (%r15,%tmp,1) ...
+    void clearSymbolicDisplacement() {
+      GV = 0;
+      CP = 0;
+      BlockAddr = 0;
+      ES = 0;
+      JT = -1;
+    }
+    // @LOCALMOD-END
+
     bool hasBaseOrIndexReg() const {
       return IndexReg.getNode() != 0 || Base_Reg.getNode() != 0;
     }
@@ -1647,16 +1665,31 @@ void X86DAGToDAGISel::LegalizeAddressingModeForNaCl(SDValue N,
 
   // Case 2 above comprises two sub-cases:
   // sub-case 1: Prevent negative indexes
-  bool NeedsFixing1 =
-       (AM.BaseType == X86ISelAddressMode::FrameIndexBase || AM.GV || AM.CP) &&
-       AM.IndexReg.getNode() &&
-       AM.Disp > 0;
 
-  // sub-case 2: Both index and base registers are being used
+  // This is relevant only if there is an index register involved.
+  if (!AM.IndexReg.getNode())
+    return;
+
+  // There are two situations to deal with.  The first is as described
+  // above, which is essentially a potentially negative index into an
+  // interior pointer to a stack-allocated structure.  The second is a
+  // potentially negative index into an interior pointer to a global
+  // array.  In this case, the global array will be a symbolic
+  // displacement.  In theory, we could recognize that it is an
+  // interior pointer only when the concrete displacement AM.Disp is
+  // nonzero, but there is at least one test case (aha.c in the LLVM
+  // test suite) that incorrectly uses a negative index to dereference
+  // a global array, so we conservatively apply the translation to all
+  // global dereferences.
+  bool HasSymbolic = AM.hasSymbolicDisplacement();
+  bool NeedsFixing1 = HasSymbolic ||
+    (AM.BaseType == X86ISelAddressMode::FrameIndexBase && AM.Disp > 0);
+
+  // sub-case 2: Both index and base registers are being used.  The
+  // test for index register being used is done above, so we only need
+  // to test for a base register being used.
   bool NeedsFixing2 =
-       (AM.BaseType == X86ISelAddressMode::RegBase) &&
-       AM.Base_Reg.getNode() &&
-       AM.IndexReg.getNode();
+    (AM.BaseType == X86ISelAddressMode::RegBase) && AM.Base_Reg.getNode();
 
   if (!NeedsFixing1 && !NeedsFixing2)
     return;
@@ -1676,8 +1709,18 @@ void X86DAGToDAGISel::LegalizeAddressingModeForNaCl(SDValue N,
     NewNodes.push_back(ShlNode.getNode());
     NewIndex = ShlNode;
   }
-  if (AM.Disp > 0) {
-    SDValue DispNode = CurDAG->getConstant(AM.Disp, N.getValueType());
+  if (AM.Disp > 0 || HasSymbolic) {
+    // If the addressing mode has a potentially problematic
+    // displacement, we pull it out into a new instruction node.  If
+    // it contains a symbolic displacement, we need to express it as a
+    // Wrapper node to make LLVM work.
+    SDValue Base, Scale, Index, Disp, Segment;
+    getAddressOperands(AM, Base, Scale, Index, Disp, Segment);
+    SDValue DispNode;
+    if (HasSymbolic)
+      DispNode = CurDAG->getNode(X86ISD::Wrapper, dl, N.getValueType(), Disp);
+    else
+      DispNode = CurDAG->getConstant(AM.Disp, N.getValueType());
     NewNodes.push_back(DispNode.getNode());
 
     SDValue AddNode = CurDAG->getNode(ISD::ADD, dl, N.getValueType(),
@@ -1694,6 +1737,7 @@ void X86DAGToDAGISel::LegalizeAddressingModeForNaCl(SDValue N,
     AM.setBaseReg(SDValue());
   }
   AM.Disp = 0;
+  AM.clearSymbolicDisplacement();
   AM.Scale = 1;
   AM.IndexReg = NewIndex;
 
diff --git a/test/CodeGen/X86/no-global-in-disp-x86-64.ll b/test/CodeGen/X86/no-global-in-disp-x86-64.ll
new file mode 100644
index 0000000000..db911ccff9
--- /dev/null
+++ b/test/CodeGen/X86/no-global-in-disp-x86-64.ll
@@ -0,0 +1,50 @@
+; RUN: pnacl-llc -O2 -mtriple=x86_64-none-nacl < %s | \
+; RUN:     FileCheck %s --check-prefix=NACLON
+; RUN: pnacl-llc -O2 -mtriple=x86_64-linux     < %s | \
+; RUN:     FileCheck %s --check-prefix=NACLOFF
+
+; This test is derived from the following C code:
+;
+; int myglobal[100];
+; void test(int arg)
+; {
+;   myglobal[arg] = arg;
+;   myglobal[arg+1] = arg;
+; }
+; int main(int argc, char **argv)
+; {
+;   test(argc);
+; }
+;
+; The goal is NOT to produce an instruction with "myglobal" as the
+; displacement value in any addressing mode, e.g. this (bad) instruction:
+;
+; movl %eax, %nacl:myglobal(%r15,%rax,4)
+;
+; The NACLOFF test is a canary that tries to ensure that the NACLON test is
+; testing the right thing.  If the NACLOFF test starts failing, it's likely
+; that the LLVM -O2 optimizations are no longer generating the problematic
+; pattern that NACLON tests for.  In that case, the test should be modified.
+
+
+@myglobal = global [100 x i32] zeroinitializer, align 4
+
+define void @test(i32 %arg) #0 {
+entry:
+; NACLON: test:
+; NACLON-NOT: mov{{.*}}nacl:myglobal(
+; NACLOFF: test:
+; NACLOFF: mov{{.*}}myglobal(
+  %arg.addr = alloca i32, align 4
+  store i32 %arg, i32* %arg.addr, align 4
+  %0 = load i32* %arg.addr, align 4
+  %1 = load i32* %arg.addr, align 4
+  %arrayidx = getelementptr inbounds [100 x i32]* @myglobal, i32 0, i32 %1
+  store i32 %0, i32* %arrayidx, align 4
+  %2 = load i32* %arg.addr, align 4
+  %3 = load i32* %arg.addr, align 4
+  %add = add nsw i32 %3, 1
+  %arrayidx1 = getelementptr inbounds [100 x i32]* @myglobal, i32 0, i32 %add
+  store i32 %2, i32* %arrayidx1, align 4
+  ret void
+}