ART: IRT refactor

IRT creation might fail. Add a path that allows to bypass the aborts
and instead signal validity. Hide this path with a private constructor,
rewrite users to use a static Create method.

Bug: 20110201

Change-Id: I440499c3372cd7557eb970b70ce2c4543da520e4

diff --git a/runtime/indirect_reference_table.cc b/runtime/indirect_reference_table.cc
index cd59365..5012965 100644 (file)
--- a/runtime/indirect_reference_table.cc
+++ b/runtime/indirect_reference_table.cc
@@ -64,7 +64,8 @@ void IndirectReferenceTable::AbortIfNoCheckJNI() {
 }
 
 IndirectReferenceTable::IndirectReferenceTable(size_t initialCount,
-                                               size_t maxCount, IndirectRefKind desiredKind)
+                                               size_t maxCount, IndirectRefKind desiredKind,
+                                               bool abort_on_error)
     : kind_(desiredKind),
       max_entries_(maxCount) {
   CHECK_GT(initialCount, 0U);
@@ -75,16 +76,28 @@ IndirectReferenceTable::IndirectReferenceTable(size_t initialCount,
   const size_t table_bytes = maxCount * sizeof(IrtEntry);
   table_mem_map_.reset(MemMap::MapAnonymous("indirect ref table", nullptr, table_bytes,
                                             PROT_READ | PROT_WRITE, false, false, &error_str));
-  CHECK(table_mem_map_.get() != nullptr) << error_str;
-  CHECK_EQ(table_mem_map_->Size(), table_bytes);
+  if (abort_on_error) {
+    CHECK(table_mem_map_.get() != nullptr) << error_str;
+    CHECK_EQ(table_mem_map_->Size(), table_bytes);
+    CHECK(table_mem_map_->Begin() != nullptr);
+  } else if (table_mem_map_.get() == nullptr ||
+             table_mem_map_->Size() != table_bytes ||
+             table_mem_map_->Begin() == nullptr) {
+    table_mem_map_.reset();
+    LOG(ERROR) << error_str;
+    return;
+  }
   table_ = reinterpret_cast<IrtEntry*>(table_mem_map_->Begin());
-  CHECK(table_ != nullptr);
   segment_state_.all = IRT_FIRST_SEGMENT;
 }
 
 IndirectReferenceTable::~IndirectReferenceTable() {
 }
 
+bool IndirectReferenceTable::IsValid() const {
+  return table_mem_map_.get() != nullptr;
+}
+
 IndirectRef IndirectReferenceTable::Add(uint32_t cookie, mirror::Object* obj) {
   IRTSegmentState prevState;
   prevState.all = cookie;

diff --git a/runtime/indirect_reference_table.h b/runtime/indirect_reference_table.h
index 25b0281..0072184 100644 (file)
--- a/runtime/indirect_reference_table.h
+++ b/runtime/indirect_reference_table.h
@@ -258,10 +258,15 @@ bool inline operator!=(const IrtIterator& lhs, const IrtIterator& rhs) {
 
 class IndirectReferenceTable {
  public:
-  IndirectReferenceTable(size_t initialCount, size_t maxCount, IndirectRefKind kind);
+  // WARNING: When using with abort_on_error = false, the object may be in a partially
+  //          initialized state. Use IsValid() to check.
+  IndirectReferenceTable(size_t initialCount, size_t maxCount, IndirectRefKind kind,
+                         bool abort_on_error = true);
 
   ~IndirectReferenceTable();
 
+  bool IsValid() const;
+
   /*
    * Add a new entry.  "obj" must be a valid non-NULL object reference.
    *

diff --git a/runtime/jni_env_ext.cc b/runtime/jni_env_ext.cc
index b2d3835..84fc404 100644 (file)
--- a/runtime/jni_env_ext.cc
+++ b/runtime/jni_env_ext.cc
@@ -28,11 +28,29 @@ static constexpr size_t kMonitorsMax = 4096;  // Arbitrary sanity check.
 
 static constexpr size_t kLocalsInitial = 64;  // Arbitrary.
 
+// 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 {
+  if (in == nullptr) {
+    return false;
+  }
+  return in->locals.IsValid();
+}
+
+JNIEnvExt* JNIEnvExt::Create(Thread* self_in, JavaVMExt* vm_in) {
+  std::unique_ptr<JNIEnvExt> ret(new JNIEnvExt(self_in, vm_in));
+  if (CheckLocalsValid(ret.get())) {
+    return ret.release();
+  }
+  return nullptr;
+}
+
 JNIEnvExt::JNIEnvExt(Thread* self_in, JavaVMExt* vm_in)
     : self(self_in),
       vm(vm_in),
       local_ref_cookie(IRT_FIRST_SEGMENT),
-      locals(kLocalsInitial, kLocalsMax, kLocal),
+      locals(kLocalsInitial, kLocalsMax, kLocal, false),
       check_jni(false),
       critical(0),
       monitors("monitors", kMonitorsInitial, kMonitorsMax) {

diff --git a/runtime/jni_env_ext.h b/runtime/jni_env_ext.h
index af87cb4..29d912c 100644 (file)
--- a/runtime/jni_env_ext.h
+++ b/runtime/jni_env_ext.h
@@ -34,7 +34,8 @@ class JavaVMExt;
 static constexpr size_t kLocalsMax = 512;
 
 struct JNIEnvExt : public JNIEnv {
-  JNIEnvExt(Thread* self, JavaVMExt* vm);
+  static JNIEnvExt* Create(Thread* self, JavaVMExt* vm);
+
   ~JNIEnvExt();
 
   void DumpReferenceTables(std::ostream& os)
@@ -87,6 +88,11 @@ struct JNIEnvExt : public JNIEnv {
 
   // Used by -Xcheck:jni.
   const JNINativeInterface* unchecked_functions;
+
+ private:
+  // The constructor should not be called directly. It may leave the object in an erronuous state,
+  // and the result needs to be checked.
+  JNIEnvExt(Thread* self, JavaVMExt* vm);
 };
 
 // Used to save and restore the JNIEnvExt state when not going through code created by the JNI

diff --git a/runtime/thread.cc b/runtime/thread.cc
index d1b0464..89fc00e 100644 (file)
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -377,7 +377,11 @@ bool Thread::Init(ThreadList* thread_list, JavaVMExt* java_vm) {
 
   tls32_.thin_lock_thread_id = thread_list->AllocThreadId(this);
 
-  tlsPtr_.jni_env = new JNIEnvExt(this, java_vm);
+  tlsPtr_.jni_env = JNIEnvExt::Create(this, java_vm);
+  if (tlsPtr_.jni_env == nullptr) {
+    return false;
+  }
+
   thread_list->Register(this);
   return true;
 }