Address comments I missed on a previous CL
I accidentally missed some comments on
android-review.googlesource.com/c/305518 when I submitted it. This
addresses those comments.
Test: mma -j40 test-art-host
Change-Id: Icd8ff65dee1730d10489f25e75bddbd455c68413
diff --git a/runtime/art_method.cc b/runtime/art_method.cc
index eeece90..ce1cf23 100644
--- a/runtime/art_method.cc
+++ b/runtime/art_method.cc
@@ -68,7 +68,8 @@
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 259bbbe..7c6a710 100644
--- a/runtime/mirror/class_ext.cc
+++ b/runtime/mirror/class_ext.cc
@@ -46,8 +46,9 @@
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 a1110d9..8e3fce3 100644
--- a/runtime/modifiers.h
+++ b/runtime/modifiers.h
@@ -67,7 +67,7 @@
// 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 69bd887..d0349b9 100644
--- a/runtime/openjdkjvmti/ti_redefine.cc
+++ b/runtime/openjdkjvmti/ti_redefine.cc
@@ -66,7 +66,8 @@
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 f3a5834..c819acd 100644
--- a/runtime/openjdkjvmti/ti_redefine.h
+++ b/runtime/openjdkjvmti/ti_redefine.h
@@ -68,7 +68,7 @@
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 @@
// 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;
}