Revert^2 "Don't wait with mutator lock""
This reverts commit f2298b784e3b57905fdc4eb720df54cb2a257a23.
PS1 is the same as aosp/2199339
PS2 fixes a bug in CompileForTest that caused failures in
MakeInitializedClassesVisiblyInitialized. Due to incomplete stack
traces, it is unclear whether this was the only cause of failures.
It also adds a check in DoCompilation that may help with tracking
down others.
PS3-4 fix a code typo and the commit message.
PS5 adds a thread suspension around the call to
MakeInitializedClassesVisiblyInitialized in EnterTransactionMode.
PS6 fixes a small problem with the PS5 change.
PS7 was an incorrect attempt to undo PS5 and PS6 and instead weaken
the assertion in MakeInitializedClassesVisiblyInitialized.
PS8 redoes this correctly by just not checking for the EnterTransaction
call. This is a stopgap, since PS5 results (not entirely unexpectedly)
in heap posisoning failures.
PS9 also disables checking inside ClassLinkerTest::AssertObjectClass.
Test: Build and boot AOSP.
Bug: 243086427
Bug: 253691761
Change-Id: I3673b0cbee4b156338088d9783269dc2a0388cba
diff --git a/compiler/jni/jni_compiler_test.cc b/compiler/jni/jni_compiler_test.cc
index d05324b..e341f8d 100644
--- a/compiler/jni/jni_compiler_test.cc
+++ b/compiler/jni/jni_compiler_test.cc
@@ -236,13 +236,14 @@
bool direct,
const char* method_name,
const char* method_sig) {
- ScopedObjectAccess soa(Thread::Current());
- StackHandleScope<2> hs(soa.Self());
+ Thread* self = Thread::Current();
+ ScopedObjectAccess soa(self);
+ StackHandleScope<2> hs(self);
Handle<mirror::ClassLoader> loader(
hs.NewHandle(soa.Decode<mirror::ClassLoader>(class_loader)));
// Compile the native method before starting the runtime
Handle<mirror::Class> c =
- hs.NewHandle(class_linker_->FindClass(soa.Self(), "LMyClassNatives;", loader));
+ hs.NewHandle(class_linker_->FindClass(self, "LMyClassNatives;", loader));
const auto pointer_size = class_linker_->GetImagePointerSize();
ArtMethod* method = c->FindClassMethod(method_name, method_sig, pointer_size);
ASSERT_TRUE(method != nullptr) << method_name << " " << method_sig;
@@ -251,8 +252,11 @@
// Class initialization could replace the entrypoint, so force
// the initialization before we set up the entrypoint below.
class_linker_->EnsureInitialized(
- soa.Self(), c, /*can_init_fields=*/ true, /*can_init_parents=*/ true);
- class_linker_->MakeInitializedClassesVisiblyInitialized(soa.Self(), /*wait=*/ true);
+ self, c, /*can_init_fields=*/ true, /*can_init_parents=*/ true);
+ {
+ ScopedThreadSuspension sts(self, ThreadState::kNative);
+ class_linker_->MakeInitializedClassesVisiblyInitialized(self, /*wait=*/ true);
+ }
}
if (check_generic_jni_) {
method->SetEntryPointFromQuickCompiledCode(class_linker_->GetRuntimeQuickGenericJniStub());
@@ -1307,7 +1311,7 @@
CompileForTestWithCurrentJni(class_loader_, false, "synchronizedThrowException", "()V");
}
}
- // Start runtime to avoid re-initialization in SetupForTest.
+ // Start runtime to avoid re-initialization in SetUpForTest.
Thread::Current()->TransitionFromSuspendedToRunnable();
bool started = runtime_->Start();
CHECK(started);
diff --git a/dex2oat/dex2oat.cc b/dex2oat/dex2oat.cc
index 16f24be..57a5acf 100644
--- a/dex2oat/dex2oat.cc
+++ b/dex2oat/dex2oat.cc
@@ -1820,7 +1820,7 @@
}
// Set up and create the compiler driver and then invoke it to compile all the dex files.
- jobject Compile() {
+ jobject Compile() REQUIRES(!Locks::mutator_lock_) {
ClassLinker* const class_linker = Runtime::Current()->GetClassLinker();
TimingLogger::ScopedTiming t("dex2oat Compile", timings_);
@@ -3059,7 +3059,8 @@
jobject obj_;
};
-static dex2oat::ReturnCode DoCompilation(Dex2Oat& dex2oat) {
+static dex2oat::ReturnCode DoCompilation(Dex2Oat& dex2oat) REQUIRES(!Locks::mutator_lock_) {
+ Locks::mutator_lock_->AssertNotHeld(Thread::Current());
dex2oat.LoadClassProfileDescriptors();
jobject class_loader = dex2oat.Compile();
// Keep the class loader that was used for compilation live for the rest of the compilation
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 756c07a..663b2b9 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -316,12 +316,20 @@
std::forward_list<Barrier*> barriers_;
};
-void ClassLinker::MakeInitializedClassesVisiblyInitialized(Thread* self, bool wait) {
+void ClassLinker::MakeInitializedClassesVisiblyInitialized(Thread* self,
+ bool wait,
+ bool allowLockChecking) {
if (kRuntimeISA == InstructionSet::kX86 || kRuntimeISA == InstructionSet::kX86_64) {
return; // Nothing to do. Thanks to the x86 memory model classes skip the initialized status.
}
std::optional<Barrier> maybe_barrier; // Avoid constructing the Barrier for `wait == false`.
if (wait) {
+ // TODO(b/253691761): The following conditional and the parameter should be removed when
+ // possible so that AssertNotHeld() becomes unconditional. Currently EnterTransaction()
+ // violates the assertion.
+ if (allowLockChecking) {
+ Locks::mutator_lock_->AssertNotHeld(self);
+ }
maybe_barrier.emplace(0);
}
int wait_count = 0;
diff --git a/runtime/class_linker.h b/runtime/class_linker.h
index eb97140..ce67b7f 100644
--- a/runtime/class_linker.h
+++ b/runtime/class_linker.h
@@ -807,7 +807,9 @@
return cha_.get();
}
- void MakeInitializedClassesVisiblyInitialized(Thread* self, bool wait);
+ void MakeInitializedClassesVisiblyInitialized(Thread* self,
+ bool wait /* ==> no locks held */,
+ bool allowLockChecking = true /* b/253691761 */);
// Registers the native method and returns the new entry point. NB The returned entry point
// might be different from the native_method argument if some MethodCallback modifies it.
diff --git a/runtime/class_linker_test.cc b/runtime/class_linker_test.cc
index a6cf10c..bd654bb 100644
--- a/runtime/class_linker_test.cc
+++ b/runtime/class_linker_test.cc
@@ -131,7 +131,12 @@
EXPECT_TRUE(JavaLangObject->GetSuperClass() == nullptr);
EXPECT_FALSE(JavaLangObject->HasSuperClass());
EXPECT_TRUE(JavaLangObject->GetClassLoader() == nullptr);
- class_linker_->MakeInitializedClassesVisiblyInitialized(Thread::Current(), /*wait=*/ true);
+ // TODO(b/253691761): Remove the last argument to MakeInitializedClassesVisiblyInitial when
+ // possible. We should not be holding the mutator lock here, but temporarily releasing it
+ // would compromise the ObjPtr argument.
+ class_linker_->MakeInitializedClassesVisiblyInitialized(Thread::Current(),
+ /*wait=*/ true,
+ /*allowLockChecking=*/ false);
EXPECT_EQ(ClassStatus::kVisiblyInitialized, JavaLangObject->GetStatus());
EXPECT_FALSE(JavaLangObject->IsErroneous());
EXPECT_TRUE(JavaLangObject->IsLoaded());
diff --git a/runtime/common_runtime_test.h b/runtime/common_runtime_test.h
index adfd764..ad2d0f4 100644
--- a/runtime/common_runtime_test.h
+++ b/runtime/common_runtime_test.h
@@ -221,7 +221,7 @@
static std::string GetImageLocation();
static std::string GetSystemImageFile();
- static void EnterTransactionMode();
+ static void EnterTransactionMode() REQUIRES_SHARED(Locks::mutator_lock_);
static void ExitTransactionMode();
static void RollbackAndExitTransactionMode() REQUIRES_SHARED(Locks::mutator_lock_);
static bool IsTransactionAborted();
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index 1f3ee80..85249de 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -753,13 +753,16 @@
GetJit()->PreZygoteFork();
}
if (!heap_->HasZygoteSpace()) {
+ Thread* self = Thread::Current();
// This is the first fork. Update ArtMethods in the boot classpath now to
// avoid having forked apps dirty the memory.
- ScopedObjectAccess soa(Thread::Current());
+
// Ensure we call FixupStaticTrampolines on all methods that are
// initialized.
- class_linker_->MakeInitializedClassesVisiblyInitialized(soa.Self(), /*wait=*/ true);
- UpdateMethodsPreFirstForkVisitor visitor(soa.Self(), class_linker_);
+ class_linker_->MakeInitializedClassesVisiblyInitialized(self, /*wait=*/ true);
+
+ ScopedObjectAccess soa(self);
+ UpdateMethodsPreFirstForkVisitor visitor(self, class_linker_);
class_linker_->VisitClasses(&visitor);
}
heap_->PreZygoteFork();
@@ -2777,7 +2780,13 @@
// Make initialized classes visibly initialized now. If that happened during the transaction
// and then the transaction was aborted, we would roll back the status update but not the
// ClassLinker's bookkeeping structures, so these classes would never be visibly initialized.
- GetClassLinker()->MakeInitializedClassesVisiblyInitialized(Thread::Current(), /*wait=*/ true);
+ // TODO(b/253691761): We should normally be in a suspended state to call
+ // MakeInitializedClassesVisiblyInitialized with wait == true. Here we are not. Suspending
+ // here causes failures with heap poisoning, so this is apparently called where suspension is
+ // not allowed. Explain why this is safe as is, or fix.
+ GetClassLinker()->MakeInitializedClassesVisiblyInitialized(Thread::Current(),
+ /*wait=*/ true,
+ /*allowLockChecking=*/ false);
// Pass the runtime `ArenaPool` to the transaction.
arena_pool = GetArenaPool();
} else {
diff --git a/runtime/runtime.h b/runtime/runtime.h
index ccae637..c155489 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -612,7 +612,8 @@
// Transaction support.
bool IsActiveTransaction() const;
- void EnterTransactionMode(bool strict, mirror::Class* root);
+ // EnterTransactionMode may suspend.
+ void EnterTransactionMode(bool strict, mirror::Class* root) REQUIRES_SHARED(Locks::mutator_lock_);
void ExitTransactionMode();
void RollbackAllTransactions() REQUIRES_SHARED(Locks::mutator_lock_);
// Transaction rollback and exit transaction are always done together, it's convenience to