From b93a16517dce12b83e9d5d3b599446e25fcc157a Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Mon, 27 Jun 2016 10:03:29 +0100 Subject: [PATCH] Do not remove loads/store with unresolved accesses. Due to AOT and compiling classes that are not ready yet (eg missing a super class), we need to be conservative when accessing fields and can end up in a situation where the same field can be resolved and unresolved within the same method (because of inlining). Therefore, disable removing loads and stores in a method when there are unresolved accesses. bug:29433999 Change-Id: I8fcfb52c584222474a8220eb16c6581350b702e0 --- compiler/optimizing/load_store_elimination.cc | 10 ++ test/608-checker-unresolved-lse/expected.txt | 0 test/608-checker-unresolved-lse/info.txt | 3 + test/608-checker-unresolved-lse/run | 18 +++ .../src-dex2oat-unresolved/MissingSuperClass.java | 18 +++ test/608-checker-unresolved-lse/src/Main.java | 127 +++++++++++++++++++++ 6 files changed, 176 insertions(+) create mode 100644 test/608-checker-unresolved-lse/expected.txt create mode 100644 test/608-checker-unresolved-lse/info.txt create mode 100644 test/608-checker-unresolved-lse/run create mode 100644 test/608-checker-unresolved-lse/src-dex2oat-unresolved/MissingSuperClass.java create mode 100644 test/608-checker-unresolved-lse/src/Main.java diff --git a/compiler/optimizing/load_store_elimination.cc b/compiler/optimizing/load_store_elimination.cc index 8a75a90cf..734768683 100644 --- a/compiler/optimizing/load_store_elimination.cc +++ b/compiler/optimizing/load_store_elimination.cc @@ -65,6 +65,16 @@ class ReferenceInfo : public ArenaObject { is_singleton_and_not_returned_ = false; return; } + if ((user->IsUnresolvedInstanceFieldGet() && (reference_ == user->InputAt(0))) || + (user->IsUnresolvedInstanceFieldSet() && (reference_ == user->InputAt(0)))) { + // The field is accessed in an unresolved way. We mark the object as a singleton to + // disable load/store optimizations on it. + // Note that we could optimize this case and still perform some optimizations until + // we hit the unresolved access, but disabling is the simplest. + is_singleton_ = false; + is_singleton_and_not_returned_ = false; + return; + } if (user->IsReturn()) { is_singleton_and_not_returned_ = false; } diff --git a/test/608-checker-unresolved-lse/expected.txt b/test/608-checker-unresolved-lse/expected.txt new file mode 100644 index 000000000..e69de29bb diff --git a/test/608-checker-unresolved-lse/info.txt b/test/608-checker-unresolved-lse/info.txt new file mode 100644 index 000000000..466d5f44e --- /dev/null +++ b/test/608-checker-unresolved-lse/info.txt @@ -0,0 +1,3 @@ +Regression test for the load store elimination optimization, +which used to wrongly remove field stores in the presence of +unresolved accesses. diff --git a/test/608-checker-unresolved-lse/run b/test/608-checker-unresolved-lse/run new file mode 100644 index 000000000..226891f65 --- /dev/null +++ b/test/608-checker-unresolved-lse/run @@ -0,0 +1,18 @@ +#!/bin/bash +# +# Copyright (C) 2016 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. + +# Use secondary switch to add secondary dex file to class path. +exec ${RUN} "${@}" --secondary diff --git a/test/608-checker-unresolved-lse/src-dex2oat-unresolved/MissingSuperClass.java b/test/608-checker-unresolved-lse/src-dex2oat-unresolved/MissingSuperClass.java new file mode 100644 index 000000000..b11b9be78 --- /dev/null +++ b/test/608-checker-unresolved-lse/src-dex2oat-unresolved/MissingSuperClass.java @@ -0,0 +1,18 @@ +/* + * Copyright (C) 2016 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. + */ + +public class MissingSuperClass { +} diff --git a/test/608-checker-unresolved-lse/src/Main.java b/test/608-checker-unresolved-lse/src/Main.java new file mode 100644 index 000000000..c6f8854b4 --- /dev/null +++ b/test/608-checker-unresolved-lse/src/Main.java @@ -0,0 +1,127 @@ +/* + * Copyright (C) 2016 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. + */ + +// We make Main extend an unresolved super class. This will lead to an +// unresolved access to Foo.field, as we won't know if Main can access +// a package private field. +public class Main extends MissingSuperClass { + + public static void main(String[] args) { + instanceFieldTest(); + staticFieldTest(); + instanceFieldTest2(); + } + + /// CHECK-START: void Main.instanceFieldTest() inliner (before) + /// CHECK-NOT: InstanceFieldSet + + /// CHECK-START: void Main.instanceFieldTest() inliner (after) + /// CHECK: InstanceFieldSet + /// CHECK: UnresolvedInstanceFieldGet + + // Load store elimination used to remove the InstanceFieldSet, thinking + // that the UnresolvedInstanceFieldGet was not related. However inlining + // can put you in a situation where the UnresolvedInstanceFieldGet resolves + // to the same field as the one in InstanceFieldSet. So the InstanceFieldSet + // must be preserved. + + /// CHECK-START: void Main.instanceFieldTest() load_store_elimination (after) + /// CHECK: InstanceFieldSet + /// CHECK: UnresolvedInstanceFieldGet + public static void instanceFieldTest() { + Foo f = new Foo(); + if (f.iField != 42) { + throw new Error("Expected 42, got " + f.iField); + } + } + + /// CHECK-START: void Main.instanceFieldTest2() inliner (before) + /// CHECK-NOT: InstanceFieldSet + /// CHECK-NOT: InstanceFieldGet + + /// CHECK-START: void Main.instanceFieldTest2() inliner (after) + /// CHECK: InstanceFieldSet + /// CHECK: InstanceFieldGet + /// CHECK: UnresolvedInstanceFieldSet + /// CHECK: InstanceFieldGet + + // Load store elimination will eliminate the first InstanceFieldGet because + // it simply follows an InstanceFieldSet. It must however not eliminate the second + // InstanceFieldGet, as the UnresolvedInstanceFieldSet might resolve to the same + // field. + + /// CHECK-START: void Main.instanceFieldTest2() load_store_elimination (after) + /// CHECK: InstanceFieldSet + /// CHECK-NOT: InstanceFieldGet + /// CHECK: UnresolvedInstanceFieldSet + /// CHECK: InstanceFieldGet + public static void instanceFieldTest2() { + Foo f = new Foo(); + int a = f.$inline$GetInstanceField(); + f.iField = 43; + a = f.$inline$GetInstanceField(); + if (a != 43) { + throw new Error("Expected 43, got " + a); + } + } + + /// CHECK-START: void Main.staticFieldTest() inliner (before) + /// CHECK-NOT: StaticFieldSet + + /// CHECK-START: void Main.staticFieldTest() inliner (after) + /// CHECK: StaticFieldSet + /// CHECK: StaticFieldSet + /// CHECK: UnresolvedStaticFieldGet + + /// CHECK-START: void Main.staticFieldTest() load_store_elimination (after) + /// CHECK: StaticFieldSet + /// CHECK: StaticFieldSet + /// CHECK: UnresolvedStaticFieldGet + public static void staticFieldTest() { + // Ensure Foo is initialized. + Foo f = new Foo(); + f.$inline$StaticSet42(); + f.$inline$StaticSet43(); + if (Foo.sField != 43) { + throw new Error("Expected 43, got " + Foo.sField); + } + } +} + +class Foo { + // field needs to be package-private to make the access in Main.main + // unresolved. + int iField; + static int sField; + + public void $inline$StaticSet42() { + sField = 42; + } + + public void $inline$StaticSet43() { + sField = 43; + } + + public int $inline$GetInstanceField() { + return iField; + } + + // Constructor needs to be public to get it resolved in Main.main + // and therefore inlined. + public Foo() { + iField = 42; + } +} -- 2.11.0