Fix null checks on volatile reference field loads on ARM64.
ART's compiler adds a null check HIR instruction before each field
load HIR instruction created in the instruction builder phase. When
implicit null checks are allowed, the compiler elides the null check
if it can be turned into an implicit one (i.e. if the offset is within
a system page range).
On ARM64, the Baker read barrier thunk built for field reference loads
needs to check the lock word of the holder of the field, and thus
includes an explicit null check if no null check has been done before.
However, this was not done for volatile loads (implemented with a
load-acquire instruction on ARM64). This change adds this missing null
check.
Test: art/test/testrunner/testrunner.py --target --64 -t 1004-checker-volatile-ref-load
Bug: 140507091
Bug: 36141117
Change-Id: Ie94f2e73d2f439ae4460549d7b71848401602a21
diff --git a/test/1004-checker-volatile-ref-load/expected.txt b/test/1004-checker-volatile-ref-load/expected.txt
new file mode 100644
index 0000000..b0aad4d
--- /dev/null
+++ b/test/1004-checker-volatile-ref-load/expected.txt
@@ -0,0 +1 @@
+passed
diff --git a/test/1004-checker-volatile-ref-load/info.txt b/test/1004-checker-volatile-ref-load/info.txt
new file mode 100644
index 0000000..1ba4a52
--- /dev/null
+++ b/test/1004-checker-volatile-ref-load/info.txt
@@ -0,0 +1,6 @@
+Regression test for b/140507091: Check that an explicit null check is
+emitted for a volatile field load on ARM64 at the beginning of the
+Baker read barrier thunk, so that a null holder object is properly
+caught (and throws a NullPointerException as expected), instead of
+trigerring a SIGSEGV, when the Concurrent Copying GC is in the marking
+phase.
diff --git a/test/1004-checker-volatile-ref-load/run b/test/1004-checker-volatile-ref-load/run
new file mode 100644
index 0000000..bdaa71b
--- /dev/null
+++ b/test/1004-checker-volatile-ref-load/run
@@ -0,0 +1,18 @@
+#! /bin/bash
+#
+# Copyright (C) 2019 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Limit the managed heap to 16 MiB to force more garbage collections.
+exec ${RUN} $@ --runtime-option -Xmx16m
diff --git a/test/1004-checker-volatile-ref-load/src/Main.java b/test/1004-checker-volatile-ref-load/src/Main.java
new file mode 100644
index 0000000..028bee8
--- /dev/null
+++ b/test/1004-checker-volatile-ref-load/src/Main.java
@@ -0,0 +1,95 @@
+/*
+ * Copyright (C) 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class Foo {
+ volatile Object bar;
+}
+
+public class Main {
+
+ public static void main(String[] args) {
+ Main main = new Main();
+ main.test();
+ System.out.println("passed");
+ }
+
+ // Check that no explicit null check is emitted for the field load of volatile
+ // field `Foo.bar` before entering the Baker read barrier thunk.
+ //
+ // Note: We cannot check the ARM64 assembly code of the Baker read barrier
+ // thunk code, as it is not emitted in the CFG output.
+ //
+ /// CHECK-START-ARM64: void Main.test() disassembly (after)
+ /// CHECK: <<Foo:l\d+>> InstanceFieldGet [{{l\d+}}] field_name:Main.foo field_type:Reference loop:<<Loop:B\d+>>
+ /// CHECK: NullCheck [<<Foo>>] dex_pc:<<PC:\d+>> loop:<<Loop>>
+ /// CHECK-NEXT: InstanceFieldGet [<<Foo>>] dex_pc:<<PC>> field_name:Foo.bar field_type:Reference loop:<<Loop>>
+ /// CHECK-NEXT: add w<<BaseRegNum:\d+>>, {{w\d+}}, #0x8 (8)
+ /// CHECK-NEXT: adr lr, #+0xc
+ // The following instruction (generated by
+ // `art::arm64::CodeGeneratorARM64::EmitBakerReadBarrierCbnz`) checks the
+ // Marking Register (X20) and goes into the Baker read barrier thunk if MR is
+ // not null. The null offset (#+0x0) in the CBNZ instruction is a placeholder
+ // for the offset to the Baker read barrier thunk (which is not yet set when
+ // the CFG output is emitted).
+ /// CHECK-NEXT: cbnz x20, #+0x0
+ /// CHECK-NEXT: ldar {{w\d+}}, [x<<BaseRegNum>>]
+
+ public void test() {
+ // Continually check that reading field `foo.bar` throws a
+ // NullPointerException while allocating over 64 MiB of memory (with heap
+ // size limited to 16 MiB), in order to increase memory pressure and
+ // eventually trigger a concurrent garbage collection, which will start by
+ // putting the GC in marking mode and enable read barriers (when the
+ // Concurrent Copying collector is used).
+ for (int i = 0; i != 64 * 1024; ++i) {
+ allocateAtLeast1KiB();
+ try {
+ // Read volatile field `bar` of `foo`, which is null, and is expected
+ // to produce a NullPointerException. On ARM64, this is implemented as a
+ // load-acquire (LDAR instruction).
+ //
+ // When the Concurrent Copying GC is marking, read barriers are enabled
+ // and the field load executes code from a Baker read barrier thunk.
+ // On ARM64, there used to be a bug in this thunk for the load-acquire
+ // case, where an explicit null check was missing, triggering an
+ // unhandled SIGSEGV when trying to load the lock word from the volatile
+ // field (b/140507091).
+ Object foo_bar = foo.bar;
+ } catch (NullPointerException e) {
+ continue;
+ }
+ // We should not be here.
+ throw new Error("Expected NullPointerException");
+ }
+ }
+
+ // Allocate at least 1 KiB of memory on the managed heap.
+ // Retain some allocated memory and release old allocations so that the
+ // garbage collector has something to do.
+ public static void allocateAtLeast1KiB() {
+ memory[allocationIndex] = new Object[1024 / 4];
+ ++allocationIndex;
+ if (allocationIndex == memory.length) {
+ allocationIndex = 0;
+ }
+ }
+
+ public static Object[] memory = new Object[1024];
+ public static int allocationIndex = 0;
+
+ private Foo foo;
+
+}
diff --git a/test/knownfailures.json b/test/knownfailures.json
index dd23b95..ced3a2e 100644
--- a/test/knownfailures.json
+++ b/test/knownfailures.json
@@ -1248,5 +1248,17 @@
"575-checker-string-init-alias"],
"variant": "baseline",
"description": [ "Working as intended tests that don't pass with baseline." ]
+ },
+ {
+ "tests": ["1004-checker-volatile-ref-load"],
+ "env_vars": {"ART_USE_READ_BARRIER": "false"},
+ "bug": "b/140507091",
+ "description": ["Test containing Checker assertions expecting Baker read barriers."]
+ },
+ {
+ "tests": ["1004-checker-volatile-ref-load"],
+ "env_vars": {"ART_READ_BARRIER_TYPE": "TABLELOOKUP"},
+ "bug": "b/140507091",
+ "description": ["Test containing Checker assertions expecting Baker read barriers."]
}
]