optimizing: do not illegally remove constructor barriers after inlining
Remove the illegal optimization that destroyed constructor barriers
after inlining invoke-super constructor calls.
---
According to JLS 7.5.1,
"Note that if one constructor invokes another constructor, and the
invoked constructor sets a final field, the freeze for the final field
takes place at the end of the invoked constructor."
This means if an object is published (stored to a location potentially
visible to another thread) inside of an outer constructor, all final
field stores from any inner constructors must be visible to other
threads.
Test: art/test.py
Bug: 37001605
Change-Id: I3b55f6c628ff1773dab88022a6475d50a1a6f906
diff --git a/compiler/optimizing/inliner.cc b/compiler/optimizing/inliner.cc
index 298ae5c..66948eb 100644
--- a/compiler/optimizing/inliner.cc
+++ b/compiler/optimizing/inliner.cc
@@ -1565,25 +1565,6 @@
/* verified_method */ nullptr,
dex_cache);
- bool requires_ctor_barrier = false;
-
- if (dex_compilation_unit.IsConstructor()) {
- // If it's a super invocation and we already generate a barrier there's no need
- // to generate another one.
- // We identify super calls by looking at the "this" pointer. If its value is the
- // same as the local "this" pointer then we must have a super invocation.
- bool is_super_invocation = invoke_instruction->InputAt(0)->IsParameterValue()
- && invoke_instruction->InputAt(0)->AsParameterValue()->IsThis();
- if (is_super_invocation && graph_->ShouldGenerateConstructorBarrier()) {
- requires_ctor_barrier = false;
- } else {
- Thread* self = Thread::Current();
- requires_ctor_barrier = compiler_driver_->RequiresConstructorBarrier(self,
- dex_compilation_unit.GetDexFile(),
- dex_compilation_unit.GetClassDefIndex());
- }
- }
-
InvokeType invoke_type = invoke_instruction->GetInvokeType();
if (invoke_type == kInterface) {
// We have statically resolved the dispatch. To please the class linker
@@ -1596,7 +1577,6 @@
graph_->GetArena(),
callee_dex_file,
method_index,
- requires_ctor_barrier,
compiler_driver_->GetInstructionSet(),
invoke_type,
graph_->IsDebuggable(),
diff --git a/compiler/optimizing/instruction_builder.cc b/compiler/optimizing/instruction_builder.cc
index 88f67fa..978c6a2 100644
--- a/compiler/optimizing/instruction_builder.cc
+++ b/compiler/optimizing/instruction_builder.cc
@@ -589,6 +589,11 @@
}
static bool RequiresConstructorBarrier(const DexCompilationUnit* cu, CompilerDriver* driver) {
+ // Can be null in unit tests only.
+ if (UNLIKELY(cu == nullptr)) {
+ return false;
+ }
+
Thread* self = Thread::Current();
return cu->IsConstructor()
&& driver->RequiresConstructorBarrier(self, cu->GetDexFile(), cu->GetClassDefIndex());
@@ -634,12 +639,9 @@
Primitive::Type type,
uint32_t dex_pc) {
if (type == Primitive::kPrimVoid) {
- if (graph_->ShouldGenerateConstructorBarrier()) {
- // The compilation unit is null during testing.
- if (dex_compilation_unit_ != nullptr) {
- DCHECK(RequiresConstructorBarrier(dex_compilation_unit_, compiler_driver_))
- << "Inconsistent use of ShouldGenerateConstructorBarrier. Should not generate a barrier.";
- }
+ // This may insert additional redundant constructor fences from the super constructors.
+ // TODO: remove redundant constructor fences (b/36656456).
+ if (RequiresConstructorBarrier(dex_compilation_unit_, compiler_driver_)) {
AppendInstruction(new (arena_) HMemoryBarrier(kStoreStore, dex_pc));
}
AppendInstruction(new (arena_) HReturnVoid(dex_pc));
diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h
index 671f950..c109369 100644
--- a/compiler/optimizing/nodes.h
+++ b/compiler/optimizing/nodes.h
@@ -305,7 +305,6 @@
HGraph(ArenaAllocator* arena,
const DexFile& dex_file,
uint32_t method_idx,
- bool should_generate_constructor_barrier,
InstructionSet instruction_set,
InvokeType invoke_type = kInvalidInvokeType,
bool debuggable = false,
@@ -332,7 +331,6 @@
method_idx_(method_idx),
invoke_type_(invoke_type),
in_ssa_form_(false),
- should_generate_constructor_barrier_(should_generate_constructor_barrier),
number_of_cha_guards_(0),
instruction_set_(instruction_set),
cached_null_constant_(nullptr),
@@ -504,10 +502,6 @@
has_bounds_checks_ = value;
}
- bool ShouldGenerateConstructorBarrier() const {
- return should_generate_constructor_barrier_;
- }
-
bool IsDebuggable() const { return debuggable_; }
// Returns a constant of the given type and value. If it does not exist
@@ -701,8 +695,6 @@
// for non-SSA form (like the number of temporaries).
bool in_ssa_form_;
- const bool should_generate_constructor_barrier_;
-
// Number of CHA guards in the graph. Used to short-circuit the
// CHA guard optimization pass when there is no CHA guard left.
uint32_t number_of_cha_guards_;
@@ -5073,7 +5065,7 @@
const DexFile& GetDexFile() const { return dex_file_; }
dex::TypeIndex GetTypeIndex() const { return type_index_; }
uint8_t GetIndex() const { return index_; }
- bool IsThis() const { return GetPackedFlag<kFlagIsThis>(); }
+ bool IsThis() const ATTRIBUTE_UNUSED { return GetPackedFlag<kFlagIsThis>(); }
bool CanBeNull() const OVERRIDE { return GetPackedFlag<kFlagCanBeNull>(); }
void SetCanBeNull(bool can_be_null) { SetPackedFlag<kFlagCanBeNull>(can_be_null); }
diff --git a/compiler/optimizing/optimizing_compiler.cc b/compiler/optimizing/optimizing_compiler.cc
index e542cbb..8aad539 100644
--- a/compiler/optimizing/optimizing_compiler.cc
+++ b/compiler/optimizing/optimizing_compiler.cc
@@ -930,16 +930,10 @@
/* verified_method */ nullptr,
dex_cache);
- bool requires_barrier = dex_compilation_unit.IsConstructor()
- && compiler_driver->RequiresConstructorBarrier(Thread::Current(),
- dex_compilation_unit.GetDexFile(),
- dex_compilation_unit.GetClassDefIndex());
-
HGraph* graph = new (arena) HGraph(
arena,
dex_file,
method_idx,
- requires_barrier,
compiler_driver->GetInstructionSet(),
kInvalidInvokeType,
compiler_driver->GetCompilerOptions().GetDebuggable(),
diff --git a/compiler/optimizing/optimizing_unit_test.h b/compiler/optimizing/optimizing_unit_test.h
index bf963b8..1cdcbd2 100644
--- a/compiler/optimizing/optimizing_unit_test.h
+++ b/compiler/optimizing/optimizing_unit_test.h
@@ -79,7 +79,9 @@
inline HGraph* CreateGraph(ArenaAllocator* allocator) {
return new (allocator) HGraph(
- allocator, *reinterpret_cast<DexFile*>(allocator->Alloc(sizeof(DexFile))), -1, false,
+ allocator,
+ *reinterpret_cast<DexFile*>(allocator->Alloc(sizeof(DexFile))),
+ /*method_idx*/-1,
kRuntimeISA);
}
diff --git a/test/476-checker-ctor-memory-barrier/src/Main.java b/test/476-checker-ctor-memory-barrier/src/Main.java
index 65486e9..330aa74 100644
--- a/test/476-checker-ctor-memory-barrier/src/Main.java
+++ b/test/476-checker-ctor-memory-barrier/src/Main.java
@@ -37,6 +37,7 @@
/// CHECK: MemoryBarrier kind:StoreStore
/// CHECK-NEXT: ReturnVoid
public ClassWithFinals() {
+ // Exactly one constructor barrier.
x = 0;
}
@@ -45,7 +46,7 @@
/// CHECK: MemoryBarrier kind:StoreStore
/// CHECK-NEXT: ReturnVoid
public ClassWithFinals(int x) {
- // This should have two barriers:
+ // This should have exactly two barriers:
// - one for the constructor
// - one for the `new` which should be inlined.
obj = new ClassWithFinals();
@@ -62,6 +63,8 @@
/// CHECK-NOT: InvokeStaticOrDirect
public InheritFromClassWithFinals() {
// Should inline the super constructor.
+ //
+ // Exactly one constructor barrier here.
}
/// CHECK-START: void InheritFromClassWithFinals.<init>(boolean) register (after)
@@ -77,6 +80,7 @@
/// CHECK-START: void InheritFromClassWithFinals.<init>(int) register (after)
/// CHECK: MemoryBarrier kind:StoreStore
/// CHECK: MemoryBarrier kind:StoreStore
+ /// CHECK-NOT: MemoryBarrier kind:StoreStore
/// CHECK: ReturnVoid
/// CHECK-START: void InheritFromClassWithFinals.<init>(int) register (after)
@@ -94,12 +98,13 @@
/// CHECK-START: void HaveFinalsAndInheritFromClassWithFinals.<init>() register (after)
/// CHECK: MemoryBarrier kind:StoreStore
+ /// CHECK: MemoryBarrier kind:StoreStore
/// CHECK-NEXT: ReturnVoid
/// CHECK-START: void HaveFinalsAndInheritFromClassWithFinals.<init>() register (after)
/// CHECK-NOT: InvokeStaticOrDirect
public HaveFinalsAndInheritFromClassWithFinals() {
- // Should inline the super constructor and remove the memory barrier.
+ // Should inline the super constructor and keep the memory barrier.
y = 0;
}
@@ -117,17 +122,19 @@
/// CHECK: MemoryBarrier kind:StoreStore
/// CHECK: MemoryBarrier kind:StoreStore
/// CHECK: MemoryBarrier kind:StoreStore
+ /// CHECK: MemoryBarrier kind:StoreStore
+ /// CHECK: MemoryBarrier kind:StoreStore
/// CHECK-NEXT: ReturnVoid
/// CHECK-START: void HaveFinalsAndInheritFromClassWithFinals.<init>(int) register (after)
/// CHECK-NOT: InvokeStaticOrDirect
public HaveFinalsAndInheritFromClassWithFinals(int unused) {
- // Should inline the super constructor and keep just one memory barrier.
+ // Should inline the super constructor and keep keep both memory barriers.
y = 0;
- // Should inline new instance and keep one barrier.
+ // Should inline new instance and keep both memory barriers.
new HaveFinalsAndInheritFromClassWithFinals();
- // Should inline new instance and keep one barrier.
+ // Should inline new instance and have exactly one barrier.
new InheritFromClassWithFinals();
}
}
@@ -166,6 +173,7 @@
/// CHECK-START: void Main.inlineNew2() register (after)
/// CHECK: MemoryBarrier kind:StoreStore
+ /// CHECK: MemoryBarrier kind:StoreStore
/// CHECK-NEXT: ReturnVoid
/// CHECK-START: void Main.inlineNew2() register (after)
@@ -177,6 +185,8 @@
/// CHECK-START: void Main.inlineNew3() register (after)
/// CHECK: MemoryBarrier kind:StoreStore
/// CHECK: MemoryBarrier kind:StoreStore
+ /// CHECK: MemoryBarrier kind:StoreStore
+ /// CHECK: MemoryBarrier kind:StoreStore
/// CHECK-NEXT: ReturnVoid
/// CHECK-START: void Main.inlineNew3() register (after)
