Clean up `IndirectReferenceTable` construction.
Split the parts that can fail to a separate function.
Test: m test-art-host-gtest
Test: testrunner.py --host --optimizing
Bug: 172332525
Change-Id: I95200a31cc757e4593d9cc7f956dd4d5ef624f92
diff --git a/runtime/indirect_reference_table.cc b/runtime/indirect_reference_table.cc
index ebf382f..1e1adf0 100644
--- a/runtime/indirect_reference_table.cc
+++ b/runtime/indirect_reference_table.cc
@@ -113,23 +113,24 @@
small_irt_freelist_ = unneeded;
}
-IndirectReferenceTable::IndirectReferenceTable(size_t max_count,
- IndirectRefKind desired_kind,
- ResizableCapacity resizable,
- std::string* error_msg)
+IndirectReferenceTable::IndirectReferenceTable(IndirectRefKind desired_kind,
+ ResizableCapacity resizable)
: segment_state_(kIRTFirstSegment),
table_(nullptr),
kind_(desired_kind),
- max_entries_(max_count),
+ max_entries_(0u),
current_num_holes_(0),
resizable_(resizable) {
- CHECK(error_msg != nullptr);
CHECK_NE(desired_kind, kJniTransitionOrInvalid);
+}
+
+bool IndirectReferenceTable::Initialize(size_t max_count, std::string* error_msg) {
+ CHECK(error_msg != nullptr);
// Overflow and maximum check.
CHECK_LE(max_count, kMaxTableSizeInBytes / sizeof(IrtEntry));
- if (max_entries_ <= kSmallIrtEntries) {
+ if (max_count <= kSmallIrtEntries) {
table_ = Runtime::Current()->GetSmallIrtAllocator()->Allocate(error_msg);
if (table_ != nullptr) {
max_entries_ = kSmallIrtEntries;
@@ -139,20 +140,18 @@
if (table_ == nullptr) {
const size_t table_bytes = RoundUp(max_count * sizeof(IrtEntry), kPageSize);
table_mem_map_ = NewIRTMap(table_bytes, error_msg);
- if (!table_mem_map_.IsValid() && error_msg->empty()) {
- *error_msg = "Unable to map memory for indirect ref table";
+ if (!table_mem_map_.IsValid()) {
+ DCHECK(!error_msg->empty());
+ return false;
}
- if (table_mem_map_.IsValid()) {
- table_ = reinterpret_cast<IrtEntry*>(table_mem_map_.Begin());
- } else {
- table_ = nullptr;
- }
+ table_ = reinterpret_cast<IrtEntry*>(table_mem_map_.Begin());
// Take into account the actual length.
max_entries_ = table_bytes / sizeof(IrtEntry);
}
segment_state_ = kIRTFirstSegment;
last_known_previous_state_ = kIRTFirstSegment;
+ return true;
}
IndirectReferenceTable::~IndirectReferenceTable() {
diff --git a/runtime/indirect_reference_table.h b/runtime/indirect_reference_table.h
index e279422..18d0730 100644
--- a/runtime/indirect_reference_table.h
+++ b/runtime/indirect_reference_table.h
@@ -257,17 +257,14 @@
kYes
};
- // WARNING: Construction of the IndirectReferenceTable may fail.
- // error_msg must not be null. If error_msg is set by the constructor, then
- // construction has failed and the IndirectReferenceTable will be in an
- // invalid state. Use IsValid to check whether the object is in an invalid
- // state.
+ // Constructs an uninitialized indirect reference table. Use `Initialize()` to initialize it.
+ IndirectReferenceTable(IndirectRefKind kind, ResizableCapacity resizable);
+
+ // Initialize the indirect reference table.
+ //
// Max_count is the minimum initial capacity (resizable), or minimum total capacity
// (not resizable). A value of 1 indicates an implementation-convenient small size.
- IndirectReferenceTable(size_t max_count,
- IndirectRefKind kind,
- ResizableCapacity resizable,
- std::string* error_msg);
+ bool Initialize(size_t max_count, std::string* error_msg);
~IndirectReferenceTable();
diff --git a/runtime/indirect_reference_table_test.cc b/runtime/indirect_reference_table_test.cc
index af0ebae..8e04469 100644
--- a/runtime/indirect_reference_table_test.cc
+++ b/runtime/indirect_reference_table_test.cc
@@ -60,11 +60,9 @@
ScopedObjectAccess soa(Thread::Current());
static const size_t kTableMax = 20;
std::string error_msg;
- IndirectReferenceTable irt(kTableMax,
- kGlobal,
- IndirectReferenceTable::ResizableCapacity::kNo,
- &error_msg);
- ASSERT_TRUE(irt.IsValid()) << error_msg;
+ IndirectReferenceTable irt(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo);
+ bool success = irt.Initialize(kTableMax, &error_msg);
+ ASSERT_TRUE(success) << error_msg;
StackHandleScope<5> hs(soa.Self());
Handle<mirror::Class> c =
@@ -302,11 +300,9 @@
// 1) Segment with holes (current_num_holes_ > 0), push new segment, add/remove reference.
{
- IndirectReferenceTable irt(kTableMax,
- kGlobal,
- IndirectReferenceTable::ResizableCapacity::kNo,
- &error_msg);
- ASSERT_TRUE(irt.IsValid()) << error_msg;
+ IndirectReferenceTable irt(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo);
+ bool success = irt.Initialize(kTableMax, &error_msg);
+ ASSERT_TRUE(success) << error_msg;
const IRTSegmentState cookie0 = kIRTFirstSegment;
@@ -333,11 +329,9 @@
// 2) Segment with holes (current_num_holes_ > 0), pop segment, add/remove reference
{
- IndirectReferenceTable irt(kTableMax,
- kGlobal,
- IndirectReferenceTable::ResizableCapacity::kNo,
- &error_msg);
- ASSERT_TRUE(irt.IsValid()) << error_msg;
+ IndirectReferenceTable irt(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo);
+ bool success = irt.Initialize(kTableMax, &error_msg);
+ ASSERT_TRUE(success) << error_msg;
const IRTSegmentState cookie0 = kIRTFirstSegment;
@@ -369,11 +363,9 @@
// 3) Segment with holes (current_num_holes_ > 0), push new segment, pop segment, add/remove
// reference.
{
- IndirectReferenceTable irt(kTableMax,
- kGlobal,
- IndirectReferenceTable::ResizableCapacity::kNo,
- &error_msg);
- ASSERT_TRUE(irt.IsValid()) << error_msg;
+ IndirectReferenceTable irt(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo);
+ bool success = irt.Initialize(kTableMax, &error_msg);
+ ASSERT_TRUE(success) << error_msg;
const IRTSegmentState cookie0 = kIRTFirstSegment;
@@ -408,11 +400,9 @@
// 4) Empty segment, push new segment, create a hole, pop a segment, add/remove a reference.
{
- IndirectReferenceTable irt(kTableMax,
- kGlobal,
- IndirectReferenceTable::ResizableCapacity::kNo,
- &error_msg);
- ASSERT_TRUE(irt.IsValid()) << error_msg;
+ IndirectReferenceTable irt(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo);
+ bool success = irt.Initialize(kTableMax, &error_msg);
+ ASSERT_TRUE(success) << error_msg;
const IRTSegmentState cookie0 = kIRTFirstSegment;
@@ -451,11 +441,9 @@
// 5) Base segment, push new segment, create a hole, pop a segment, push new segment, add/remove
// reference
{
- IndirectReferenceTable irt(kTableMax,
- kGlobal,
- IndirectReferenceTable::ResizableCapacity::kNo,
- &error_msg);
- ASSERT_TRUE(irt.IsValid()) << error_msg;
+ IndirectReferenceTable irt(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo);
+ bool success = irt.Initialize(kTableMax, &error_msg);
+ ASSERT_TRUE(success) << error_msg;
const IRTSegmentState cookie0 = kIRTFirstSegment;
@@ -501,11 +489,9 @@
ASSERT_TRUE(obj0 != nullptr);
std::string error_msg;
- IndirectReferenceTable irt(kTableMax,
- kLocal,
- IndirectReferenceTable::ResizableCapacity::kYes,
- &error_msg);
- ASSERT_TRUE(irt.IsValid()) << error_msg;
+ IndirectReferenceTable irt(kLocal, IndirectReferenceTable::ResizableCapacity::kYes);
+ bool success = irt.Initialize(kTableMax, &error_msg);
+ ASSERT_TRUE(success) << error_msg;
CheckDump(&irt, 0, 0);
const IRTSegmentState cookie = kIRTFirstSegment;
diff --git a/runtime/jni/java_vm_ext.cc b/runtime/jni/java_vm_ext.cc
index 8eeb093..248335a 100644
--- a/runtime/jni/java_vm_ext.cc
+++ b/runtime/jni/java_vm_ext.cc
@@ -495,9 +495,7 @@
JII::AttachCurrentThreadAsDaemon
};
-JavaVMExt::JavaVMExt(Runtime* runtime,
- const RuntimeArgumentMap& runtime_options,
- std::string* error_msg)
+JavaVMExt::JavaVMExt(Runtime* runtime, const RuntimeArgumentMap& runtime_options)
: runtime_(runtime),
check_jni_abort_hook_(nullptr),
check_jni_abort_hook_data_(nullptr),
@@ -506,13 +504,10 @@
tracing_enabled_(runtime_options.Exists(RuntimeArgumentMap::JniTrace)
|| VLOG_IS_ON(third_party_jni)),
trace_(runtime_options.GetOrDefault(RuntimeArgumentMap::JniTrace)),
- globals_(kGlobalsMax, kGlobal, IndirectReferenceTable::ResizableCapacity::kNo, error_msg),
+ globals_(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo),
libraries_(new Libraries),
unchecked_functions_(&gJniInvokeInterface),
- weak_globals_(kWeakGlobalsMax,
- kWeakGlobal,
- IndirectReferenceTable::ResizableCapacity::kNo,
- error_msg),
+ weak_globals_(kWeakGlobal, IndirectReferenceTable::ResizableCapacity::kNo),
allow_accessing_weak_globals_(true),
weak_globals_add_condition_("weak globals add condition",
(CHECK(Locks::jni_weak_globals_lock_ != nullptr),
@@ -527,21 +522,23 @@
SetCheckJniEnabled(runtime_options.Exists(RuntimeArgumentMap::CheckJni) || kIsDebugBuild);
}
+bool JavaVMExt::Initialize(std::string* error_msg) {
+ return globals_.Initialize(kGlobalsMax, error_msg) &&
+ weak_globals_.Initialize(kWeakGlobalsMax, error_msg);
+}
+
JavaVMExt::~JavaVMExt() {
UnloadBootNativeLibraries();
}
-// Checking "globals" and "weak_globals" usually requires locks, but we
-// don't need the locks to check for validity when constructing the
-// object. Use NO_THREAD_SAFETY_ANALYSIS for this.
std::unique_ptr<JavaVMExt> JavaVMExt::Create(Runtime* runtime,
const RuntimeArgumentMap& runtime_options,
- std::string* error_msg) NO_THREAD_SAFETY_ANALYSIS {
- std::unique_ptr<JavaVMExt> java_vm(new JavaVMExt(runtime, runtime_options, error_msg));
- if (java_vm && java_vm->globals_.IsValid() && java_vm->weak_globals_.IsValid()) {
- return java_vm;
+ std::string* error_msg) {
+ std::unique_ptr<JavaVMExt> java_vm(new JavaVMExt(runtime, runtime_options));
+ if (!java_vm->Initialize(error_msg)) {
+ return nullptr;
}
- return nullptr;
+ return java_vm;
}
jint JavaVMExt::HandleGetEnv(/*out*/void** env, jint version) {
diff --git a/runtime/jni/java_vm_ext.h b/runtime/jni/java_vm_ext.h
index 08de18b..b97088a 100644
--- a/runtime/jni/java_vm_ext.h
+++ b/runtime/jni/java_vm_ext.h
@@ -218,9 +218,12 @@
static jstring GetLibrarySearchPath(JNIEnv* env, jobject class_loader);
private:
- // The constructor should not be called directly. It may leave the object in
- // an erroneous state, and the result needs to be checked.
- JavaVMExt(Runtime* runtime, const RuntimeArgumentMap& runtime_options, std::string* error_msg);
+ // The constructor should not be called directly. Use `Create()` that initializes
+ // the new `JavaVMExt` object by calling `Initialize()`.
+ JavaVMExt(Runtime* runtime, const RuntimeArgumentMap& runtime_options);
+
+ // Initialize the `JavaVMExt` object.
+ bool Initialize(std::string* error_msg);
// Return true if self can currently access weak globals.
bool MayAccessWeakGlobals(Thread* self) const REQUIRES_SHARED(Locks::mutator_lock_);
diff --git a/runtime/jni/jni_env_ext.cc b/runtime/jni/jni_env_ext.cc
index 4510b37..7d522c1 100644
--- a/runtime/jni/jni_env_ext.cc
+++ b/runtime/jni/jni_env_ext.cc
@@ -44,13 +44,6 @@
const JNINativeInterface* JNIEnvExt::table_override_ = nullptr;
-bool JNIEnvExt::CheckLocalsValid(JNIEnvExt* in) NO_THREAD_SAFETY_ANALYSIS {
- if (in == nullptr) {
- return false;
- }
- return in->locals_.IsValid();
-}
-
jint JNIEnvExt::GetEnvHandler(JavaVMExt* vm, /*out*/void** env, jint version) {
UNUSED(vm);
// GetEnv always returns a JNIEnv* for the most current supported JNI version,
@@ -66,18 +59,18 @@
}
JNIEnvExt* JNIEnvExt::Create(Thread* self_in, JavaVMExt* vm_in, std::string* error_msg) {
- std::unique_ptr<JNIEnvExt> ret(new JNIEnvExt(self_in, vm_in, error_msg));
- if (CheckLocalsValid(ret.get())) {
- return ret.release();
+ std::unique_ptr<JNIEnvExt> ret(new JNIEnvExt(self_in, vm_in));
+ if (!ret->Initialize(error_msg)) {
+ return nullptr;
}
- return nullptr;
+ return ret.release();
}
-JNIEnvExt::JNIEnvExt(Thread* self_in, JavaVMExt* vm_in, std::string* error_msg)
+JNIEnvExt::JNIEnvExt(Thread* self_in, JavaVMExt* vm_in)
: self_(self_in),
vm_(vm_in),
local_ref_cookie_(kIRTFirstSegment),
- locals_(1, kLocal, IndirectReferenceTable::ResizableCapacity::kYes, error_msg),
+ locals_(kLocal, IndirectReferenceTable::ResizableCapacity::kYes),
monitors_("monitors", kMonitorsInitial, kMonitorsMax),
critical_(0),
check_jni_(false),
@@ -88,6 +81,10 @@
unchecked_functions_ = GetJniNativeInterface();
}
+bool JNIEnvExt::Initialize(std::string* error_msg) {
+ return locals_.Initialize(/*max_count=*/ 1u, error_msg);
+}
+
void JNIEnvExt::SetFunctionsToRuntimeShutdownFunctions() {
functions = GetRuntimeShutdownNativeInterface();
}
diff --git a/runtime/jni/jni_env_ext.h b/runtime/jni/jni_env_ext.h
index bdde5f8..5952b3b 100644
--- a/runtime/jni/jni_env_ext.h
+++ b/runtime/jni/jni_env_ext.h
@@ -151,20 +151,18 @@
REQUIRES(!Locks::thread_list_lock_, !Locks::jni_function_table_lock_);
private:
- // Checking "locals" requires the mutator lock, but at creation time we're
- // really only interested in validity, which isn't changing. To avoid grabbing
- // the mutator lock, factored out and tagged with NO_THREAD_SAFETY_ANALYSIS.
- static bool CheckLocalsValid(JNIEnvExt* in) NO_THREAD_SAFETY_ANALYSIS;
-
// Override of function tables. This applies to both default as well as instrumented (CheckJNI)
// function tables.
static const JNINativeInterface* table_override_ GUARDED_BY(Locks::jni_function_table_lock_);
- // The constructor should not be called directly. It may leave the object in an erroneous state,
- // and the result needs to be checked.
- JNIEnvExt(Thread* self, JavaVMExt* vm, std::string* error_msg)
+ // The constructor should not be called directly. Use `Create()` that initializes
+ // the new `JNIEnvExt` object by calling `Initialize()`.
+ JNIEnvExt(Thread* self, JavaVMExt* vm)
REQUIRES(!Locks::jni_function_table_lock_);
+ // Initialize the `JNIEnvExt` object.
+ bool Initialize(std::string* error_msg);
+
// Link to Thread::Current().
Thread* const self_;
@@ -175,7 +173,7 @@
IRTSegmentState local_ref_cookie_;
// JNI local references.
- IndirectReferenceTable locals_ GUARDED_BY(Locks::mutator_lock_);
+ IndirectReferenceTable locals_;
// Stack of cookies corresponding to PushLocalFrame/PopLocalFrame calls.
// TODO: to avoid leaks (and bugs), we need to clear this vector on entry (or return)
diff --git a/runtime/jni/jni_internal_test.cc b/runtime/jni/jni_internal_test.cc
index eca5aec..a41043b 100644
--- a/runtime/jni/jni_internal_test.cc
+++ b/runtime/jni/jni_internal_test.cc
@@ -2580,11 +2580,10 @@
// by modifying memory.
// The parameters don't really matter here.
std::string error_msg;
- IndirectReferenceTable irt(5,
- IndirectRefKind::kGlobal,
- IndirectReferenceTable::ResizableCapacity::kNo,
- &error_msg);
- ASSERT_TRUE(irt.IsValid()) << error_msg;
+ IndirectReferenceTable irt(IndirectRefKind::kGlobal,
+ IndirectReferenceTable::ResizableCapacity::kNo);
+ bool success = irt.Initialize(/*max_count=*/ 5, &error_msg);
+ ASSERT_TRUE(success) << error_msg;
IRTSegmentState old_state = irt.GetSegmentState();
// Write some new state directly. We invert parts of old_state to ensure a new value.