[GR-4454] Possible race condition in Truffle code installation.

axel22 · axel22 · commit 28b5474a1f68 · 2017-06-13T10:52:31.000-07:00
diff --git a/compiler/src/org.graalvm.compiler.asm.aarch64/src/org/graalvm/compiler/asm/aarch64/AArch64Assembler.java b/compiler/src/org.graalvm.compiler.asm.aarch64/src/org/graalvm/compiler/asm/aarch64/AArch64Assembler.java
@@ -94,6 +94,7 @@
 import static org.graalvm.compiler.asm.aarch64.AArch64Assembler.Instruction.STXR;
 import static org.graalvm.compiler.asm.aarch64.AArch64Assembler.Instruction.SUB;
 import static org.graalvm.compiler.asm.aarch64.AArch64Assembler.Instruction.SUBS;
+import static org.graalvm.compiler.asm.aarch64.AArch64Assembler.Instruction.TBZ;
 import static org.graalvm.compiler.asm.aarch64.AArch64Assembler.Instruction.UBFM;
 import static org.graalvm.compiler.asm.aarch64.AArch64Assembler.Instruction.UDIV;
 import static org.graalvm.compiler.asm.aarch64.AArch64Assembler.InstructionType.FP32;
@@ -475,6 +476,7 @@ public enum Instruction {
         BCOND(0x54000000),
         CBNZ(0x01000000),
         CBZ(0x00000000),
+        TBZ(0x36000000),
 
         B(0x00000000),
         BL(0x80000000),
@@ -808,6 +810,25 @@ protected void cbz(int size, Register reg, int imm21, int pos) {
         conditionalBranchInstruction(reg, imm21, generalFromSize(size), Instruction.CBZ, pos);
     }
 
+    /**
+     * Test a single bit and branch if the bit is zero.
+     *
+     * @param reg general purpose register. May not be null, zero-register or stackpointer.
+     * @param size Instruction size in bits. Should be either 32 or 64.
+     * @param uimm6 Unsigned 6-bit bit index.
+     * @param pos Position at which instruction is inserted into buffer. -1 means insert at end.
+     */
+    protected void tbz(int size, Register reg, int uimm6, int pos) {
+        assert reg.getRegisterCategory().equals(CPU);
+        InstructionType type = generalFromSize(size);
+        int encoding = type.encoding | TBZ.encoding | (uimm6 << 18) | rd(reg);
+        if (pos == -1) {
+            emitInt(encoding);
+        } else {
+            emitInt(encoding, pos);
+        }
+    }
+
     private void conditionalBranchInstruction(Register reg, int imm21, InstructionType type, Instruction instr, int pos) {
         assert reg.getRegisterCategory().equals(CPU);
         int instrEncoding = instr.encoding | CompareBranchOp;
diff --git a/compiler/src/org.graalvm.compiler.asm.aarch64/src/org/graalvm/compiler/asm/aarch64/AArch64MacroAssembler.java b/compiler/src/org.graalvm.compiler.asm.aarch64/src/org/graalvm/compiler/asm/aarch64/AArch64MacroAssembler.java
@@ -1294,6 +1294,26 @@ public void cbz(int size, Register cmp, Label label) {
         }
     }
 
+    /**
+     * Test a single bit and branch if the bit is zero.
+     *
+     * @param cmp general purpose register. May not be null, zero-register or stackpointer.
+     * @param size Instruction size in bits. Should be either 32 or 64.
+     * @param uimm6 Unsigned 6-bit bit index.
+     * @param label Can only handle 21-bit word-aligned offsets for now. May be unbound. Non null.
+     */
+    public void tbz(int size, Register cmp, int uimm6, Label label) {
+        if (label.isBound()) {
+            int offset = label.position() - position();
+            super.tbz(size, cmp, uimm6, offset);
+        } else {
+            label.addPatchAt(position());
+            int regEncoding = cmp.encoding << (PatchLabelKind.INFORMATION_OFFSET + 1);
+            int sizeEncoding = (size == 64 ? 1 : 0) << PatchLabelKind.INFORMATION_OFFSET;
+            emitInt(PatchLabelKind.BRANCH_CONDITIONALLY.encoding | regEncoding | sizeEncoding);
+        }
+    }
+
     /**
      * Branches to label if condition is true.
      *
diff --git a/compiler/src/org.graalvm.compiler.asm.amd64/src/org/graalvm/compiler/asm/amd64/AMD64Assembler.java b/compiler/src/org.graalvm.compiler.asm.amd64/src/org/graalvm/compiler/asm/amd64/AMD64Assembler.java
@@ -3492,6 +3492,14 @@ public final void testq(Register dst, Register src) {
         emitByte(0xC0 | encode);
     }
 
+    public final void btrq(Register src, int imm8) {
+        int encode = prefixqAndEncode(src.encoding);
+        emitByte(0x0F);
+        emitByte(0xBA);
+        emitByte(0xF0 | encode);
+        emitByte(imm8);
+    }
+
     public final void xaddl(AMD64Address dst, Register src) {
         prefix(dst, src);
         emitByte(0x0F);
diff --git a/compiler/src/org.graalvm.compiler.asm.sparc/src/org/graalvm/compiler/asm/sparc/SPARCMacroAssembler.java b/compiler/src/org.graalvm.compiler.asm.sparc/src/org/graalvm/compiler/asm/sparc/SPARCMacroAssembler.java
@@ -81,6 +81,10 @@ public void jmp(Label l) {
         nop();  // delay slot
     }
 
+    public void bz(Label l) {
+        BPCC.emit(this, Xcc, ConditionFlag.Zero, NOT_ANNUL, PREDICT_NOT_TAKEN, l);
+    }
+
     @Override
     protected final void patchJumpTarget(int branch, int branchTarget) {
         final int disp = (branchTarget - branch) / 4;
diff --git a/compiler/src/org.graalvm.compiler.truffle.hotspot.aarch64/src/org/graalvm/compiler/truffle/hotspot/aarch64/AArch64OptimizedCallTargetInstumentationFactory.java b/compiler/src/org.graalvm.compiler.truffle.hotspot.aarch64/src/org/graalvm/compiler/truffle/hotspot/aarch64/AArch64OptimizedCallTargetInstumentationFactory.java
@@ -24,6 +24,8 @@
 
 import static jdk.vm.ci.hotspot.HotSpotCallingConventionType.JavaCall;
 import static jdk.vm.ci.meta.JavaKind.Object;
+import static org.graalvm.compiler.asm.aarch64.AArch64Assembler.BarrierKind.LOAD_LOAD;
+import static org.graalvm.compiler.asm.aarch64.AArch64Assembler.BarrierKind.LOAD_STORE;
 
 import org.graalvm.compiler.asm.Assembler;
 import org.graalvm.compiler.asm.Label;
@@ -62,7 +64,11 @@ protected void injectTailCallCode() {
                     AArch64Address entryPointAddress = AArch64Address.createPairUnscaledImmediateAddress(thisRegister, getFieldOffset("entryPoint", InstalledCode.class));
 
                     masm.ldr(64, spillRegister, entryPointAddress);
+                    masm.dmb(LOAD_LOAD);
+                    masm.dmb(LOAD_STORE);
                     masm.cbz(64, spillRegister, doProlog);
+                    masm.tbz(64, spillRegister, 0, doProlog);
+                    masm.eor(64, spillRegister, spillRegister, 1);
                     masm.jmp(spillRegister);
                     masm.nop();
                     masm.bind(doProlog);
diff --git a/compiler/src/org.graalvm.compiler.truffle.hotspot.amd64/src/org/graalvm/compiler/truffle/hotspot/amd64/AMD64OptimizedCallTargetInstrumentationFactory.java b/compiler/src/org.graalvm.compiler.truffle.hotspot.amd64/src/org/graalvm/compiler/truffle/hotspot/amd64/AMD64OptimizedCallTargetInstrumentationFactory.java
@@ -26,6 +26,7 @@
 import jdk.vm.ci.amd64.AMD64;
 import jdk.vm.ci.code.CodeCacheProvider;
 import jdk.vm.ci.code.InstalledCode;
+import jdk.vm.ci.code.MemoryBarriers;
 import jdk.vm.ci.code.Register;
 import jdk.vm.ci.meta.JavaKind;
 
@@ -69,8 +70,11 @@ protected void injectTailCallCode() {
                  */
                 asm.movq(spillRegister, codeBlobAddress, true);
                 assert asm.position() - pos >= AMD64HotSpotBackend.PATCHED_VERIFIED_ENTRY_POINT_INSTRUCTION_SIZE;
+                asm.membar(MemoryBarriers.JMM_POST_VOLATILE_READ);
                 asm.testq(spillRegister, spillRegister);
                 asm.jcc(ConditionFlag.Equal, doProlog);
+                asm.btrq(spillRegister, 0);
+                asm.jcc(ConditionFlag.CarryClear, doProlog);
                 asm.jmp(spillRegister);
 
                 asm.bind(doProlog);
diff --git a/compiler/src/org.graalvm.compiler.truffle.hotspot.sparc/src/org/graalvm/compiler/truffle/hotspot/sparc/SPARCOptimizedCallTargetInstumentationFactory.java b/compiler/src/org.graalvm.compiler.truffle.hotspot.sparc/src/org/graalvm/compiler/truffle/hotspot/sparc/SPARCOptimizedCallTargetInstumentationFactory.java
@@ -31,9 +31,11 @@
 import jdk.vm.ci.code.InstalledCode;
 import jdk.vm.ci.code.Register;
 
+import jdk.vm.ci.sparc.SPARC;
 import org.graalvm.compiler.asm.Assembler;
 import org.graalvm.compiler.asm.Label;
 import org.graalvm.compiler.asm.sparc.SPARCAddress;
+import org.graalvm.compiler.asm.sparc.SPARCAssembler;
 import org.graalvm.compiler.asm.sparc.SPARCMacroAssembler;
 import org.graalvm.compiler.asm.sparc.SPARCMacroAssembler.ScratchRegister;
 import org.graalvm.compiler.code.CompilationResult;
@@ -65,7 +67,11 @@ protected void injectTailCallCode() {
                     SPARCAddress entryPointAddress = new SPARCAddress(thisRegister, getFieldOffset("entryPoint", InstalledCode.class));
 
                     asm.ldx(entryPointAddress, spillRegister);
+                    asm.membar(SPARCAssembler.MEMBAR_LOAD_LOAD | SPARCAssembler.MEMBAR_LOAD_STORE);
                     asm.compareBranch(spillRegister, 0, Equal, Xcc, doProlog, PREDICT_NOT_TAKEN, null);
+                    asm.andcc(spillRegister, 1, SPARC.g0);
+                    asm.bz(doProlog);
+                    asm.xor(spillRegister, 1, spillRegister);
                     asm.jmp(spillRegister);
                     asm.nop();
                     asm.bind(doProlog);
diff --git a/compiler/src/org.graalvm.compiler.truffle.test/src/org/graalvm/compiler/truffle/test/InstrumentBranchesPhaseTest.java b/compiler/src/org.graalvm.compiler.truffle.test/src/org/graalvm/compiler/truffle/test/InstrumentBranchesPhaseTest.java
@@ -110,8 +110,8 @@ public void simpleIfTest() {
         Assert.assertTrue(target.isValid());
         target.call();
         String stackOutput = instrumentation.accessTableToList(getOptions()).get(0);
-        Assert.assertTrue(stackOutput.contains("org.graalvm.compiler.truffle.test.InstrumentBranchesPhaseTest$SimpleIfTestNode.execute(InstrumentBranchesPhaseTest.java"));
-        Assert.assertTrue(stackOutput.contains("[bci: 4]\n[0] state = ELSE(if=0#, else=1#)"));
+        Assert.assertTrue(stackOutput, stackOutput.contains("org.graalvm.compiler.truffle.test.InstrumentBranchesPhaseTest$SimpleIfTestNode.execute(InstrumentBranchesPhaseTest.java"));
+        Assert.assertTrue(stackOutput, stackOutput.contains("[bci: 4]\n[0] state = ELSE(if=0#, else=1#)"));
         String histogramOutput = instrumentation.accessTableToHistogram().get(0);
         Assert.assertEquals("  0: ********************************************************************************", histogramOutput);
     }
@@ -128,10 +128,10 @@ public void twoIfsTest() {
         target.call();
         target.call();
         String stackOutput1 = instrumentation.accessTableToList(getOptions()).get(0);
-        Assert.assertTrue(stackOutput1.contains("org.graalvm.compiler.truffle.test.InstrumentBranchesPhaseTest$TwoIfsTestNode.execute(InstrumentBranchesPhaseTest.java"));
-        Assert.assertTrue(stackOutput1.contains("[bci: 4]\n[1] state = ELSE(if=0#, else=2#)"));
+        Assert.assertTrue(stackOutput1, stackOutput1.contains("org.graalvm.compiler.truffle.test.InstrumentBranchesPhaseTest$TwoIfsTestNode.execute(InstrumentBranchesPhaseTest.java"));
+        Assert.assertTrue(stackOutput1, stackOutput1.contains("[bci: 4]\n[1] state = ELSE(if=0#, else=2#)"));
         String stackOutput2 = instrumentation.accessTableToList(getOptions()).get(1);
-        Assert.assertTrue(stackOutput2.contains("org.graalvm.compiler.truffle.test.InstrumentBranchesPhaseTest$TwoIfsTestNode.execute(InstrumentBranchesPhaseTest.java"));
-        Assert.assertTrue(stackOutput2.contains("[bci: 18]\n[2] state = IF(if=2#, else=0#)"));
+        Assert.assertTrue(stackOutput2, stackOutput2.contains("org.graalvm.compiler.truffle.test.InstrumentBranchesPhaseTest$TwoIfsTestNode.execute(InstrumentBranchesPhaseTest.java"));
+        Assert.assertTrue(stackOutput2, stackOutput2.contains("[bci: 18]\n[2] state = IF(if=2#, else=0#)"));
     }
 }
diff --git a/compiler/src/org.graalvm.compiler.truffle/src/org/graalvm/compiler/truffle/OptimizedCallTarget.java b/compiler/src/org.graalvm.compiler.truffle/src/org/graalvm/compiler/truffle/OptimizedCallTarget.java
@@ -76,8 +76,17 @@
 public class OptimizedCallTarget extends InstalledCode implements RootCallTarget, ReplaceObserver, com.oracle.truffle.api.LoopCountReceiver {
 
     private static final String NODE_REWRITING_ASSUMPTION_NAME = "nodeRewritingAssumption";
+    private static final long ENTRY_POINT_OFFSET;
     static final String CALL_BOUNDARY_METHOD_NAME = "callProxy";
 
+    static {
+        try {
+            ENTRY_POINT_OFFSET = UnsafeAccess.UNSAFE.objectFieldOffset(InstalledCode.class.getDeclaredField("entryPoint"));
+        } catch (NoSuchFieldException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
     /** The AST to be executed when this call target is called. */
     private final RootNode rootNode;
 
@@ -565,6 +574,19 @@ CompilerOptions getCompilerOptions() {
         return DefaultCompilerOptions.INSTANCE;
     }
 
+    public void releaseEntryPoint() {
+        long seenEntryPoint = entryPoint;
+        if (seenEntryPoint == 0) {
+            return;
+        }
+        // No need to retry, since a failure means that the entry point was reset to zero.
+        // The reason is that the current thread is the only thread calling this method,
+        // and the only other thread that is changing the entryPoint is the VM itself.
+        // Furthermore, no other thread will reinstall the call target until the current thread
+        // completes.
+        UnsafeAccess.UNSAFE.compareAndSwapLong(this, ENTRY_POINT_OFFSET, seenEntryPoint, seenEntryPoint | 1);
+    }
+
     private static final class NonTrivialNodeCountVisitor implements NodeVisitor {
         public int nodeCount;
 
diff --git a/compiler/src/org.graalvm.compiler.truffle/src/org/graalvm/compiler/truffle/TruffleCompiler.java b/compiler/src/org.graalvm.compiler.truffle/src/org/graalvm/compiler/truffle/TruffleCompiler.java
@@ -61,7 +61,6 @@
 import com.oracle.truffle.api.nodes.UnexpectedResultException;
 
 import jdk.vm.ci.code.CompilationRequest;
-import jdk.vm.ci.code.InstalledCode;
 import jdk.vm.ci.meta.MetaAccessProvider;
 import jdk.vm.ci.meta.ResolvedJavaMethod;
 import jdk.vm.ci.meta.ResolvedJavaType;
@@ -197,7 +196,7 @@ private static void dequeueInlinedCallSites(TruffleInlining inliningDecision, Op
     }
 
     @SuppressWarnings("try")
-    public CompilationResult compileMethodHelper(StructuredGraph graph, String name, PhaseSuite<HighTierContext> graphBuilderSuite, InstalledCode predefinedInstalledCode,
+    public CompilationResult compileMethodHelper(StructuredGraph graph, String name, PhaseSuite<HighTierContext> graphBuilderSuite, OptimizedCallTarget predefinedInstalledCode,
                     CompilationRequest compilationRequest) {
         try (Scope s = Debug.scope("TruffleFinal")) {
             Debug.dump(Debug.BASIC_LEVEL, graph, "After TruffleTier");
@@ -223,11 +222,11 @@ public CompilationResult compileMethodHelper(StructuredGraph graph, String name,
             throw Debug.handle(e);
         }
 
-        compilationNotify.notifyCompilationGraalTierFinished((OptimizedCallTarget) predefinedInstalledCode, graph);
+        compilationNotify.notifyCompilationGraalTierFinished(predefinedInstalledCode, graph);
 
-        InstalledCode installedCode;
+        OptimizedCallTarget installedCode;
         try (DebugCloseable a = CodeInstallationTime.start(); DebugCloseable c = CodeInstallationMemUse.start()) {
-            installedCode = backend.createInstalledCode(graph.method(), compilationRequest, result, graph.getSpeculationLog(), predefinedInstalledCode, false);
+            installedCode = (OptimizedCallTarget) backend.createInstalledCode(graph.method(), compilationRequest, result, graph.getSpeculationLog(), predefinedInstalledCode, false);
         } catch (Throwable e) {
             throw Debug.handle(e);
         }
@@ -236,6 +235,10 @@ public CompilationResult compileMethodHelper(StructuredGraph graph, String name,
             a.getAssumption().registerInstalledCode(installedCode);
         }
 
+        if (!providers.getCodeCache().getTarget().arch.getName().equals("aarch64")) {
+            installedCode.releaseEntryPoint();
+        }
+
         return result;
     }
 
diff --git a/compiler/src/org.graalvm.compiler.truffle/src/org/graalvm/compiler/truffle/UnsafeAccess.java b/compiler/src/org.graalvm.compiler.truffle/src/org/graalvm/compiler/truffle/UnsafeAccess.java
@@ -0,0 +1,51 @@
+/*
+ * Copyright (c) 2012, 2017, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package org.graalvm.compiler.truffle;
+
+import sun.misc.Unsafe;
+
+import java.lang.reflect.Field;
+
+/**
+ * Package private access to the {@link Unsafe} capability.
+ */
+class UnsafeAccess {
+
+    static final Unsafe UNSAFE = initUnsafe();
+
+    private static Unsafe initUnsafe() {
+        try {
+            // Fast path when we are trusted.
+            return Unsafe.getUnsafe();
+        } catch (SecurityException se) {
+            // Slow path when we are not trusted.
+            try {
+                Field theUnsafe = Unsafe.class.getDeclaredField("theUnsafe");
+                theUnsafe.setAccessible(true);
+                return (Unsafe) theUnsafe.get(Unsafe.class);
+            } catch (Exception e) {
+                throw new RuntimeException("exception while trying to get Unsafe", e);
+            }
+        }
+    }
+}
