Merge "Address comments I missed on a previous CL"

5 files changed, 10 insertions, 6 deletions

diff --git a/runtime/art_method.cc b/runtime/art_method.cc
index 77d799fec4..96b6f18403 100644
--- a/runtime/art_method.cc
+++ b/runtime/art_method.cc
@@ -79,7 +79,8 @@ mirror::DexCache* ArtMethod::GetObsoleteDexCache() {
   DCHECK(ext->GetObsoleteDexCaches() != nullptr);
   int32_t len = obsolete_methods->GetLength();
   DCHECK_EQ(len, ext->GetObsoleteDexCaches()->GetLength());
-  // TODO I think this is fine since images should never have obsolete methods in them.
+  // Using kRuntimePointerSize (instead of using the image's pointer size) is fine since images
+  // should never have obsolete methods in them so they should always be the same.
   PointerSize pointer_size = kRuntimePointerSize;
   DCHECK_EQ(kRuntimePointerSize, Runtime::Current()->GetClassLinker()->GetImagePointerSize());
   for (int32_t i = 0; i < len; i++) {
diff --git a/runtime/mirror/class_ext.cc b/runtime/mirror/class_ext.cc
index 259bbbe174..7c6a710cef 100644
--- a/runtime/mirror/class_ext.cc
+++ b/runtime/mirror/class_ext.cc
@@ -46,8 +46,9 @@ void ClassExt::SetObsoleteArrays(ObjPtr<PointerArray> methods,
   SetFieldObject<false>(obsolete_methods_off, methods.Ptr());
 }
 
-// TODO We really need to be careful how we update this. If we ever in the future make it so that
-// these arrays are written into without all threads being suspended we have a race condition!
+// We really need to be careful how we update this. If we ever in the future make it so that
+// these arrays are written into without all threads being suspended we have a race condition! This
+// race could cause obsolete methods to be missed.
 bool ClassExt::ExtendObsoleteArrays(Thread* self, uint32_t increase) {
   DCHECK_EQ(GetLockOwnerThreadId(), Thread::Current()->GetThreadId())
       << "Obsolete arrays are set without synchronization!";
diff --git a/runtime/modifiers.h b/runtime/modifiers.h
index 8a01043062..ae6b31d2fe 100644
--- a/runtime/modifiers.h
+++ b/runtime/modifiers.h
@@ -67,7 +67,7 @@ static constexpr uint32_t kAccCompileDontBother =     0x01000000;  // method (ru
 
 // Set by the verifier for a method that could not be verified to follow structured locking.
 static constexpr uint32_t kAccMustCountLocks =        0x02000000;  // method (runtime)
-// Set to indicate that the ArtMethod is obsolete and has a different DexCache from it's declaring
+// Set to indicate that the ArtMethod is obsolete and has a different DexCache from its declaring
 // class.
 // TODO Might want to re-arrange some of these so that we can have obsolete + intrinsic methods.
 static constexpr uint32_t kAccObsoleteMethod =        0x04000000;  // method (runtime)
diff --git a/runtime/openjdkjvmti/ti_redefine.cc b/runtime/openjdkjvmti/ti_redefine.cc
index 69bd88768c..d0349b987f 100644
--- a/runtime/openjdkjvmti/ti_redefine.cc
+++ b/runtime/openjdkjvmti/ti_redefine.cc
@@ -66,7 +66,8 @@ std::unique_ptr<art::MemMap> Redefiner::MoveDataToMemMap(const std::string& orig
     return map;
   }
   memcpy(map->Begin(), dex_data, data_len);
-  // Make the dex files mmap read only.
+  // Make the dex files mmap read only. This matches how other DexFiles are mmaped and prevents
+  // programs from corrupting it.
   map->Protect(PROT_READ);
   return map;
 }
diff --git a/runtime/openjdkjvmti/ti_redefine.h b/runtime/openjdkjvmti/ti_redefine.h
index f3a583478b..c819acd5ac 100644
--- a/runtime/openjdkjvmti/ti_redefine.h
+++ b/runtime/openjdkjvmti/ti_redefine.h
@@ -68,7 +68,7 @@ class Redefiner {
  public:
   // Redefine the given class with the given dex data. Note this function does not take ownership of
   // the dex_data pointer. It is not used after this call however and may be freed if desired.
-  // The caller is responsible for freeing it. The runtime makes it's own copy of the data.
+  // The caller is responsible for freeing it. The runtime makes its own copy of the data.
   static jvmtiError RedefineClass(ArtJvmTiEnv* env,
                                   art::Runtime* runtime,
                                   art::Thread* self,
@@ -146,6 +146,7 @@ class Redefiner {
   // This will check that no constraints are violated (more than 1 class in dex file, any changes in
   // number/declaration of methods & fields, changes in access flags, etc.)
   bool EnsureRedefinitionIsValid() {
+    LOG(WARNING) << "Redefinition is not checked for validity currently";
     return true;
   }