[optimizing] Add memory barriers in constructors when needed
If a class has final fields we must add a memory barrier before
returning from constructor. This makes sure the fields are visible to
other threads.
Bug: 19851497
Change-Id: If8c485092fc512efb9636cd568cb0543fb27688e
diff --git a/compiler/driver/compiler_driver.cc b/compiler/driver/compiler_driver.cc
index 641d174..538d969 100644
--- a/compiler/driver/compiler_driver.cc
+++ b/compiler/driver/compiler_driver.cc
@@ -2381,7 +2381,7 @@
}
bool CompilerDriver::RequiresConstructorBarrier(Thread* self, const DexFile* dex_file,
- uint16_t class_def_index) {
+ uint16_t class_def_index) const {
ReaderMutexLock mu(self, freezing_constructor_lock_);
return freezing_constructor_classes_.count(ClassReference(dex_file, class_def_index)) != 0;
}
diff --git a/compiler/driver/compiler_driver.h b/compiler/driver/compiler_driver.h
index 1a4ae13..bd9d7c1 100644
--- a/compiler/driver/compiler_driver.h
+++ b/compiler/driver/compiler_driver.h
@@ -187,7 +187,8 @@
void AddRequiresConstructorBarrier(Thread* self, const DexFile* dex_file,
uint16_t class_def_index);
- bool RequiresConstructorBarrier(Thread* self, const DexFile* dex_file, uint16_t class_def_index);
+ bool RequiresConstructorBarrier(Thread* self, const DexFile* dex_file,
+ uint16_t class_def_index) const;
// Callbacks from compiler to see what runtime checks must be generated.
diff --git a/compiler/optimizing/builder.cc b/compiler/optimizing/builder.cc
index 8a64d81..fff2906 100644
--- a/compiler/optimizing/builder.cc
+++ b/compiler/optimizing/builder.cc
@@ -520,8 +520,19 @@
UpdateLocal(instruction.VRegA(), current_block_->GetLastInstruction());
}
+static bool RequiresConstructorBarrier(const DexCompilationUnit& cu, const CompilerDriver& driver) {
+ Thread* self = Thread::Current();
+ return cu.IsConstructor()
+ && driver.RequiresConstructorBarrier(self, cu.GetDexFile(), cu.GetClassDefIndex());
+}
+
void HGraphBuilder::BuildReturn(const Instruction& instruction, Primitive::Type type) {
if (type == Primitive::kPrimVoid) {
+ // Note that we might insert redundant barriers when inlining `super` calls.
+ // TODO: add a data flow analysis to get rid of duplicate barriers.
+ if (RequiresConstructorBarrier(*dex_compilation_unit_, *compiler_driver_)) {
+ current_block_->AddInstruction(new (arena_) HMemoryBarrier(kStoreStore));
+ }
current_block_->AddInstruction(new (arena_) HReturnVoid());
} else {
HInstruction* value = LoadLocal(instruction.VRegA(), type);
diff --git a/compiler/optimizing/code_generator_arm.cc b/compiler/optimizing/code_generator_arm.cc
index 2ea9203..037a4ae 100644
--- a/compiler/optimizing/code_generator_arm.cc
+++ b/compiler/optimizing/code_generator_arm.cc
@@ -1214,6 +1214,14 @@
UNUSED(constant);
}
+void LocationsBuilderARM::VisitMemoryBarrier(HMemoryBarrier* memory_barrier) {
+ memory_barrier->SetLocations(nullptr);
+}
+
+void InstructionCodeGeneratorARM::VisitMemoryBarrier(HMemoryBarrier* memory_barrier) {
+ GenerateMemoryBarrier(memory_barrier->GetBarrierKind());
+}
+
void LocationsBuilderARM::VisitReturnVoid(HReturnVoid* ret) {
ret->SetLocations(nullptr);
}
diff --git a/compiler/optimizing/code_generator_arm64.cc b/compiler/optimizing/code_generator_arm64.cc
index efc41e7..779340d 100644
--- a/compiler/optimizing/code_generator_arm64.cc
+++ b/compiler/optimizing/code_generator_arm64.cc
@@ -2430,6 +2430,14 @@
}
}
+void LocationsBuilderARM64::VisitMemoryBarrier(HMemoryBarrier* memory_barrier) {
+ memory_barrier->SetLocations(nullptr);
+}
+
+void InstructionCodeGeneratorARM64::VisitMemoryBarrier(HMemoryBarrier* memory_barrier) {
+ GenerateMemoryBarrier(memory_barrier->GetBarrierKind());
+}
+
void LocationsBuilderARM64::VisitReturn(HReturn* instruction) {
LocationSummary* locations = new (GetGraph()->GetArena()) LocationSummary(instruction);
Primitive::Type return_type = instruction->InputAt(0)->GetType();
diff --git a/compiler/optimizing/code_generator_x86.cc b/compiler/optimizing/code_generator_x86.cc
index 879216d..6ac4ad7 100644
--- a/compiler/optimizing/code_generator_x86.cc
+++ b/compiler/optimizing/code_generator_x86.cc
@@ -1120,6 +1120,14 @@
UNUSED(constant);
}
+void LocationsBuilderX86::VisitMemoryBarrier(HMemoryBarrier* memory_barrier) {
+ memory_barrier->SetLocations(nullptr);
+}
+
+void InstructionCodeGeneratorX86::VisitMemoryBarrier(HMemoryBarrier* memory_barrier) {
+ GenerateMemoryBarrier(memory_barrier->GetBarrierKind());
+}
+
void LocationsBuilderX86::VisitReturnVoid(HReturnVoid* ret) {
ret->SetLocations(nullptr);
}
diff --git a/compiler/optimizing/code_generator_x86_64.cc b/compiler/optimizing/code_generator_x86_64.cc
index a3d3490..65b128a 100644
--- a/compiler/optimizing/code_generator_x86_64.cc
+++ b/compiler/optimizing/code_generator_x86_64.cc
@@ -1145,6 +1145,14 @@
UNUSED(constant);
}
+void LocationsBuilderX86_64::VisitMemoryBarrier(HMemoryBarrier* memory_barrier) {
+ memory_barrier->SetLocations(nullptr);
+}
+
+void InstructionCodeGeneratorX86_64::VisitMemoryBarrier(HMemoryBarrier* memory_barrier) {
+ GenerateMemoryBarrier(memory_barrier->GetBarrierKind());
+}
+
void LocationsBuilderX86_64::VisitReturnVoid(HReturnVoid* ret) {
ret->SetLocations(nullptr);
}
diff --git a/compiler/optimizing/dead_code_elimination.cc b/compiler/optimizing/dead_code_elimination.cc
index fc3dd01..9499040 100644
--- a/compiler/optimizing/dead_code_elimination.cc
+++ b/compiler/optimizing/dead_code_elimination.cc
@@ -38,6 +38,7 @@
if (!inst->HasSideEffects()
&& !inst->CanThrow()
&& !inst->IsSuspendCheck()
+ && !inst->IsMemoryBarrier() // If we added an explicit barrier then we should keep it.
&& !inst->HasUses()) {
block->RemoveInstruction(inst);
}
diff --git a/compiler/optimizing/graph_visualizer.cc b/compiler/optimizing/graph_visualizer.cc
index 4c28378..ca9cbc3 100644
--- a/compiler/optimizing/graph_visualizer.cc
+++ b/compiler/optimizing/graph_visualizer.cc
@@ -192,6 +192,10 @@
output_ << " " << phi->GetRegNumber();
}
+ void VisitMemoryBarrier(HMemoryBarrier* barrier) OVERRIDE {
+ output_ << " " << barrier->GetBarrierKind();
+ }
+
bool IsPass(const char* name) {
return strcmp(pass_name_, name) == 0;
}
diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h
index 649038b..c3c35ac 100644
--- a/compiler/optimizing/nodes.h
+++ b/compiler/optimizing/nodes.h
@@ -19,6 +19,7 @@
#include "base/arena_containers.h"
#include "base/arena_object.h"
+#include "dex/compiler_enums.h"
#include "entrypoints/quick/quick_entrypoints_enum.h"
#include "handle.h"
#include "handle_scope.h"
@@ -718,6 +719,7 @@
M(LoadString, Instruction) \
M(Local, Instruction) \
M(LongConstant, Constant) \
+ M(MemoryBarrier, Instruction) \
M(MonitorOperation, Instruction) \
M(Mul, BinaryOperation) \
M(Neg, UnaryOperation) \
@@ -908,6 +910,12 @@
HUseListNode<T>* use_node_;
};
+// TODO: Add better documentation to this class and maybe refactor with more suggestive names.
+// - Has(All)SideEffects suggests that all the side effects are present but only ChangesSomething
+// flag is consider.
+// - DependsOn suggests that there is a real dependency between side effects but it only
+// checks DependendsOnSomething flag.
+//
// Represents the side effects an instruction may have.
class SideEffects : public ValueObject {
public:
@@ -3437,6 +3445,22 @@
DISALLOW_COPY_AND_ASSIGN(HCheckCast);
};
+class HMemoryBarrier : public HTemplateInstruction<0> {
+ public:
+ explicit HMemoryBarrier(MemBarrierKind barrier_kind)
+ : HTemplateInstruction(SideEffects::None()),
+ barrier_kind_(barrier_kind) {}
+
+ MemBarrierKind GetBarrierKind() { return barrier_kind_; }
+
+ DECLARE_INSTRUCTION(MemoryBarrier);
+
+ private:
+ const MemBarrierKind barrier_kind_;
+
+ DISALLOW_COPY_AND_ASSIGN(HMemoryBarrier);
+};
+
class HMonitorOperation : public HTemplateInstruction<1> {
public:
enum OperationKind {
diff --git a/test/476-checker-ctor-memory-barrier/expected.txt b/test/476-checker-ctor-memory-barrier/expected.txt
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/476-checker-ctor-memory-barrier/expected.txt
diff --git a/test/476-checker-ctor-memory-barrier/info.txt b/test/476-checker-ctor-memory-barrier/info.txt
new file mode 100644
index 0000000..9bd311f
--- /dev/null
+++ b/test/476-checker-ctor-memory-barrier/info.txt
@@ -0,0 +1,2 @@
+Tests if we add memory barriers on constructors when needed (i.e when the
+class has final fields).
diff --git a/test/476-checker-ctor-memory-barrier/src/Main.java b/test/476-checker-ctor-memory-barrier/src/Main.java
new file mode 100644
index 0000000..10aa2ab
--- /dev/null
+++ b/test/476-checker-ctor-memory-barrier/src/Main.java
@@ -0,0 +1,147 @@
+/*
+* Copyright (C) 2015 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 ClassWithoutFinals {
+ // CHECK-START: void ClassWithoutFinals.<init>() register (after)
+ // CHECK-NOT: MemoryBarrier {{StoreStore}}
+ public ClassWithoutFinals() {}
+}
+
+class ClassWithFinals {
+ public final int x;
+ public ClassWithFinals obj;
+
+ // CHECK-START: void ClassWithFinals.<init>(boolean) register (after)
+ // CHECK: MemoryBarrier {{StoreStore}}
+ // CHECK-NOT: {{.*}}
+ // CHECK: ReturnVoid
+ public ClassWithFinals(boolean cond) {
+ x = 0;
+ if (cond) {
+ // avoid inlining
+ throw new RuntimeException();
+ }
+ }
+
+ // CHECK-START: void ClassWithFinals.<init>() register (after)
+ // CHECK: MemoryBarrier {{StoreStore}}
+ // CHECK-NOT: {{.*}}
+ // CHECK: ReturnVoid
+ public ClassWithFinals() {
+ x = 0;
+ }
+
+ // CHECK-START: void ClassWithFinals.<init>(int) register (after)
+ // CHECK: MemoryBarrier {{StoreStore}}
+ // CHECK: MemoryBarrier {{StoreStore}}
+ // CHECK-NOT: {{.*}}
+ // CHECK: ReturnVoid
+ public ClassWithFinals(int x) {
+ // This should have two barriers:
+ // - one for the constructor
+ // - one for the `new` which should be inlined.
+ obj = new ClassWithFinals();
+ this.x = x;
+ }
+}
+
+class InheritFromClassWithFinals extends ClassWithFinals {
+ // CHECK-START: void InheritFromClassWithFinals.<init>() register (after)
+ // CHECK: MemoryBarrier {{StoreStore}}
+ // CHECK-NOT: {{.*}}
+ // CHECK: ReturnVoid
+
+ // CHECK-START: void InheritFromClassWithFinals.<init>() register (after)
+ // CHECK-NOT: InvokeStaticOrDirect
+ public InheritFromClassWithFinals() {
+ // Should inline the super constructor.
+ }
+
+ // CHECK-START: void InheritFromClassWithFinals.<init>(boolean) register (after)
+ // CHECK: InvokeStaticOrDirect
+
+ // CHECK-START: void InheritFromClassWithFinals.<init>(boolean) register (after)
+ // CHECK-NOT: MemoryBarrier {{StoreStore}}
+ public InheritFromClassWithFinals(boolean cond) {
+ super(cond);
+ // should not inline the super constructor
+ }
+}
+
+class HaveFinalsAndInheritFromClassWithFinals extends ClassWithFinals {
+ final int y;
+
+ // CHECK-START: void HaveFinalsAndInheritFromClassWithFinals.<init>() register (after)
+ // CHECK: MemoryBarrier {{StoreStore}}
+ // CHECK: MemoryBarrier {{StoreStore}}
+ // CHECK-NOT: {{.*}}
+ // CHECK: ReturnVoid
+
+ // CHECK-START: void HaveFinalsAndInheritFromClassWithFinals.<init>() register (after)
+ // CHECK-NOT: InvokeStaticOrDirect
+ public HaveFinalsAndInheritFromClassWithFinals() {
+ // Should inline the super constructor.
+ y = 0;
+ }
+
+ // CHECK-START: void HaveFinalsAndInheritFromClassWithFinals.<init>(boolean) register (after)
+ // CHECK: InvokeStaticOrDirect
+ // CHECK: MemoryBarrier {{StoreStore}}
+ // CHECK-NOT: {{.*}}
+ // CHECK: ReturnVoid
+ public HaveFinalsAndInheritFromClassWithFinals(boolean cond) {
+ super(cond);
+ // should not inline the super constructor
+ y = 0;
+ }
+}
+
+public class Main {
+
+ // CHECK-START: ClassWithFinals Main.noInlineNoConstructorBarrier() register (after)
+ // CHECK: InvokeStaticOrDirect
+
+ // CHECK-START: ClassWithFinals Main.noInlineNoConstructorBarrier() register (after)
+ // CHECK-NOT: MemoryBarrier {{StoreStore}}
+ public static ClassWithFinals noInlineNoConstructorBarrier() {
+ return new ClassWithFinals(false);
+ }
+
+ // CHECK-START: ClassWithFinals Main.inlineConstructorBarrier() register (after)
+ // CHECK: MemoryBarrier {{StoreStore}}
+ // CHECK-NOT: {{.*}}
+ // CHECK: Return
+
+ // CHECK-START: ClassWithFinals Main.inlineConstructorBarrier() register (after)
+ // CHECK-NOT: InvokeStaticOrDirect
+ public static ClassWithFinals inlineConstructorBarrier() {
+ return new ClassWithFinals();
+ }
+
+ // CHECK-START: InheritFromClassWithFinals Main.doubleInlineConstructorBarrier() register (after)
+ // CHECK: MemoryBarrier {{StoreStore}}
+ // CHECK-NOT: {{.*}}
+ // CHECK: Return
+
+ // CHECK-START: InheritFromClassWithFinals Main.doubleInlineConstructorBarrier() register (after)
+ // CHECK-NOT: InvokeStaticOrDirect
+ public static InheritFromClassWithFinals doubleInlineConstructorBarrier() {
+ return new InheritFromClassWithFinals();
+ }
+
+ public static void main(String[] args) { }
+}
