Rename a couple of methods related to redefinition for readability

TransformClasses is a method to actually redefine the method and
TransformClassesDirect is a method to call ClassFileLoadHooks which is
confusing. So rename TransformClassesDirect to CallClassFileLoadHooks
and also add some comments

Also rename GetRedefinitionError to CanRedefineClass.

Test: art/test.py
Change-Id: Ie04927f1aae2880d87bcb9bb19b5f3971911fd43

5 files changed, 85 insertions, 63 deletions

diff --git a/openjdkjvmti/ti_class.cc b/openjdkjvmti/ti_class.cc
index 7ded350c53..5581bc2f02 100644
--- a/openjdkjvmti/ti_class.cc
+++ b/openjdkjvmti/ti_class.cc
@@ -192,8 +192,8 @@ struct ClassCallback : public art::ClassLoadCallback {
     def.InitFirstLoad(descriptor, class_loader, initial_dex_file);
 
     // Call all non-retransformable agents.
-    Transformer::TransformSingleClassDirect<ArtJvmtiEvent::kClassFileLoadHookNonRetransformable>(
-        event_handler, self, &def);
+    Transformer::CallClassFileLoadHooksSingleClass<
+        ArtJvmtiEvent::kClassFileLoadHookNonRetransformable>(event_handler, self, &def);
 
     std::vector<unsigned char> post_non_retransform;
     if (def.IsModified()) {
@@ -203,11 +203,11 @@ struct ClassCallback : public art::ClassLoadCallback {
     }
 
     // Call all structural transformation agents.
-    Transformer::TransformSingleClassDirect<ArtJvmtiEvent::kStructuralDexFileLoadHook>(
+    Transformer::CallClassFileLoadHooksSingleClass<ArtJvmtiEvent::kStructuralDexFileLoadHook>(
         event_handler, self, &def);
     // Call all retransformable agents.
-    Transformer::TransformSingleClassDirect<ArtJvmtiEvent::kClassFileLoadHookRetransformable>(
-        event_handler, self, &def);
+    Transformer::CallClassFileLoadHooksSingleClass<
+        ArtJvmtiEvent::kClassFileLoadHookRetransformable>(event_handler, self, &def);
 
     if (def.IsModified()) {
       VLOG(class_linker) << "Changing class " << descriptor;
diff --git a/openjdkjvmti/ti_redefine.cc b/openjdkjvmti/ti_redefine.cc
index c9efee6c48..8ffdb8e26f 100644
--- a/openjdkjvmti/ti_redefine.cc
+++ b/openjdkjvmti/ti_redefine.cc
@@ -378,7 +378,7 @@ Redefiner::IsModifiableClassGeneric(jvmtiEnv* env, jclass klass, jboolean* is_re
   art::Handle<art::mirror::Class> h_klass(hs.NewHandle(obj->AsClass()));
   std::string err_unused;
   *is_redefinable =
-      Redefiner::GetClassRedefinitionError<kType>(h_klass, &err_unused) != ERR(UNMODIFIABLE_CLASS)
+      Redefiner::CanRedefineClass<kType>(h_klass, &err_unused) != ERR(UNMODIFIABLE_CLASS)
           ? JNI_TRUE
           : JNI_FALSE;
   return OK;
@@ -395,7 +395,7 @@ jvmtiError Redefiner::IsModifiableClass(jvmtiEnv* env, jclass klass, jboolean* i
 }
 
 template <RedefinitionType kType>
-jvmtiError Redefiner::GetClassRedefinitionError(jclass klass, /*out*/ std::string* error_msg) {
+jvmtiError Redefiner::CanRedefineClass(jclass klass, /*out*/ std::string* error_msg) {
   art::Thread* self = art::Thread::Current();
   art::ScopedObjectAccess soa(self);
   art::StackHandleScope<1> hs(self);
@@ -404,12 +404,12 @@ jvmtiError Redefiner::GetClassRedefinitionError(jclass klass, /*out*/ std::strin
     return ERR(INVALID_CLASS);
   }
   art::Handle<art::mirror::Class> h_klass(hs.NewHandle(obj->AsClass()));
-  return Redefiner::GetClassRedefinitionError<kType>(h_klass, error_msg);
+  return Redefiner::CanRedefineClass<kType>(h_klass, error_msg);
 }
 
 template <RedefinitionType kType>
-jvmtiError Redefiner::GetClassRedefinitionError(art::Handle<art::mirror::Class> klass,
-                                                /*out*/ std::string* error_msg) {
+jvmtiError Redefiner::CanRedefineClass(art::Handle<art::mirror::Class> klass,
+                                       /*out*/ std::string* error_msg) {
   art::Thread* self = art::Thread::Current();
   if (!klass->IsResolved()) {
     // It's only a problem to try to retransform/redefine a unprepared class if it's happening on
@@ -526,9 +526,9 @@ jvmtiError Redefiner::GetClassRedefinitionError(art::Handle<art::mirror::Class>
   return OK;
 }
 
-template jvmtiError Redefiner::GetClassRedefinitionError<RedefinitionType::kNormal>(
+template jvmtiError Redefiner::CanRedefineClass<RedefinitionType::kNormal>(
     art::Handle<art::mirror::Class> klass, /*out*/ std::string* error_msg);
-template jvmtiError Redefiner::GetClassRedefinitionError<RedefinitionType::kStructural>(
+template jvmtiError Redefiner::CanRedefineClass<RedefinitionType::kStructural>(
     art::Handle<art::mirror::Class> klass, /*out*/ std::string* error_msg);
 
 // Moves dex data to an anonymous, read-only mmap'd region.
@@ -604,8 +604,8 @@ jvmtiError Redefiner::RedefineClassesGeneric(jvmtiEnv* jenv,
   std::vector<ArtClassDefinition> def_vector;
   def_vector.reserve(class_count);
   for (jint i = 0; i < class_count; i++) {
-    jvmtiError res = Redefiner::GetClassRedefinitionError<RedefinitionType::kNormal>(
-        definitions[i].klass, &error_msg);
+    jvmtiError res =
+        Redefiner::CanRedefineClass<RedefinitionType::kNormal>(definitions[i].klass, &error_msg);
     if (res != OK) {
       JVMTI_LOG(WARNING, env) << "FAILURE TO REDEFINE " << error_msg;
       return res;
@@ -618,11 +618,12 @@ jvmtiError Redefiner::RedefineClassesGeneric(jvmtiEnv* jenv,
     }
     def_vector.push_back(std::move(def));
   }
-  // Call all the transformation events.
-  Transformer::RetransformClassesDirect<kType>(self, &def_vector);
-  if (kType == RedefinitionType::kStructural) {
-    Transformer::RetransformClassesDirect<RedefinitionType::kNormal>(self, &def_vector);
-  }
+
+  // Call necessary hooks. According to the spec we should send class file load hooks here. We
+  // handle it slightly differently to support structural redefinition. Look at the comments
+  // in Transformer::CallClassFileLoadHooks for more details.
+  Transformer::CallClassFileLoadHooks<kType>(self, &def_vector);
+
   jvmtiError res = RedefineClassesDirect(env, runtime, self, def_vector, kType, &error_msg);
   if (res != OK) {
     JVMTI_LOG(WARNING, env) << "FAILURE TO REDEFINE " << error_msg;
@@ -1155,9 +1156,9 @@ bool Redefiner::ClassRedefinition::CheckRedefinable() {
   art::Handle<art::mirror::Class> h_klass(hs.NewHandle(GetMirrorClass()));
   jvmtiError res;
   if (driver_->type_ == RedefinitionType::kStructural && this->IsStructuralRedefinition()) {
-    res = Redefiner::GetClassRedefinitionError<RedefinitionType::kStructural>(h_klass, &err);
+    res = Redefiner::CanRedefineClass<RedefinitionType::kStructural>(h_klass, &err);
   } else {
-    res = Redefiner::GetClassRedefinitionError<RedefinitionType::kNormal>(h_klass, &err);
+    res = Redefiner::CanRedefineClass<RedefinitionType::kNormal>(h_klass, &err);
   }
   if (res != OK) {
     RecordFailure(res, err);
diff --git a/openjdkjvmti/ti_redefine.h b/openjdkjvmti/ti_redefine.h
index 8c2965b75e..46b50fadf1 100644
--- a/openjdkjvmti/ti_redefine.h
+++ b/openjdkjvmti/ti_redefine.h
@@ -103,8 +103,8 @@ class Redefiner {
                                       std::string* error_msg);
 
   // Helper for checking if redefinition/retransformation is allowed.
-  template<RedefinitionType kType = RedefinitionType::kNormal>
-  static jvmtiError GetClassRedefinitionError(jclass klass, /*out*/std::string* error_msg)
+  template <RedefinitionType kType = RedefinitionType::kNormal>
+  static jvmtiError CanRedefineClass(jclass klass, /*out*/ std::string* error_msg)
       REQUIRES(!art::Locks::mutator_lock_);
 
   static jvmtiError StructurallyRedefineClassDirect(jvmtiEnv* env,
@@ -322,9 +322,9 @@ class Redefiner {
   template<RedefinitionType kType = RedefinitionType::kNormal>
   static jvmtiError IsModifiableClassGeneric(jvmtiEnv* env, jclass klass, jboolean* is_redefinable);
 
-  template<RedefinitionType kType = RedefinitionType::kNormal>
-  static jvmtiError GetClassRedefinitionError(art::Handle<art::mirror::Class> klass,
-                                              /*out*/std::string* error_msg)
+  template <RedefinitionType kType = RedefinitionType::kNormal>
+  static jvmtiError CanRedefineClass(art::Handle<art::mirror::Class> klass,
+                                     /*out*/ std::string* error_msg)
       REQUIRES_SHARED(art::Locks::mutator_lock_);
 
   jvmtiError Run() REQUIRES_SHARED(art::Locks::mutator_lock_);
diff --git a/openjdkjvmti/transform.cc b/openjdkjvmti/transform.cc
index bccdcb18af..d3448d9e65 100644
--- a/openjdkjvmti/transform.cc
+++ b/openjdkjvmti/transform.cc
@@ -76,20 +76,23 @@ void Transformer::Register(EventHandler* eh) {
 }
 
 // Initialize templates.
-template
-void Transformer::TransformSingleClassDirect<ArtJvmtiEvent::kClassFileLoadHookNonRetransformable>(
-    EventHandler* event_handler, art::Thread* self, /*in-out*/ArtClassDefinition* def);
-template
-void Transformer::TransformSingleClassDirect<ArtJvmtiEvent::kClassFileLoadHookRetransformable>(
-    EventHandler* event_handler, art::Thread* self, /*in-out*/ArtClassDefinition* def);
-template
-void Transformer::TransformSingleClassDirect<ArtJvmtiEvent::kStructuralDexFileLoadHook>(
-    EventHandler* event_handler, art::Thread* self, /*in-out*/ArtClassDefinition* def);
+template void Transformer::CallClassFileLoadHooksSingleClass<
+    ArtJvmtiEvent::kClassFileLoadHookNonRetransformable>(EventHandler* event_handler,
+                                                         art::Thread* self,
+                                                         /*in-out*/ ArtClassDefinition* def);
+template void Transformer::CallClassFileLoadHooksSingleClass<
+    ArtJvmtiEvent::kClassFileLoadHookRetransformable>(EventHandler* event_handler,
+                                                      art::Thread* self,
+                                                      /*in-out*/ ArtClassDefinition* def);
+template void Transformer::CallClassFileLoadHooksSingleClass<
+    ArtJvmtiEvent::kStructuralDexFileLoadHook>(EventHandler* event_handler,
+                                               art::Thread* self,
+                                               /*in-out*/ ArtClassDefinition* def);
 
-template<ArtJvmtiEvent kEvent>
-void Transformer::TransformSingleClassDirect(EventHandler* event_handler,
-                                             art::Thread* self,
-                                             /*in-out*/ArtClassDefinition* def) {
+template <ArtJvmtiEvent kEvent>
+void Transformer::CallClassFileLoadHooksSingleClass(EventHandler* event_handler,
+                                                    art::Thread* self,
+                                                    /*in-out*/ ArtClassDefinition* def) {
   static_assert(kEvent == ArtJvmtiEvent::kClassFileLoadHookNonRetransformable ||
                 kEvent == ArtJvmtiEvent::kClassFileLoadHookRetransformable ||
                 kEvent == ArtJvmtiEvent::kStructuralDexFileLoadHook,
@@ -115,21 +118,41 @@ void Transformer::TransformSingleClassDirect(EventHandler* event_handler,
 }
 
 template <RedefinitionType kType>
-void Transformer::RetransformClassesDirect(
-    art::Thread* self,
-    /*in-out*/ std::vector<ArtClassDefinition>* definitions) {
-  constexpr ArtJvmtiEvent kEvent = kType == RedefinitionType::kNormal
-                                       ? ArtJvmtiEvent::kClassFileLoadHookRetransformable
-                                       : ArtJvmtiEvent::kStructuralDexFileLoadHook;
-  for (ArtClassDefinition& def : *definitions) {
-    TransformSingleClassDirect<kEvent>(gEventHandler, self, &def);
+void Transformer::CallClassFileLoadHooks(art::Thread* self,
+                                         /*in-out*/ std::vector<ArtClassDefinition>* definitions) {
+  if (kType == RedefinitionType::kNormal) {
+    // For normal redefinition we have to call ClassFileLoadHook according to the spec. We use an
+    // internal event "ClassFileLoadHookRetransformable" for agents that can redefine and a
+    // "ClassFileLoadHookNonRetransformable" for agents that cannot redefine. When an agent is
+    // attached to a non-debuggable environment, we cannot redefine any classes. Splitting the
+    // ClassFileLoadHooks allows us to differentiate between these two cases. This method is only
+    // called when redefinition is allowed so just run ClassFileLoadHookRetransformable hooks.
+    for (ArtClassDefinition& def : *definitions) {
+      CallClassFileLoadHooksSingleClass<ArtJvmtiEvent::kClassFileLoadHookRetransformable>(
+          gEventHandler, self, &def);
+    }
+  } else {
+    // For structural redefinition we call StructualDexFileLoadHook in addition to the
+    // ClassFileLoadHooks. This let's us specify if structural modifications are allowed.
+    // TODO(mythria): The spec only specifies we need to call ClassFileLoadHooks, the
+    // StructuralDexFileLoadHooks is internal to ART. It is not clear if we need to run all
+    // StructuralDexFileHooks before ClassFileLoadHooks. Doing it this way to keep the existing
+    // behaviour.
+    for (ArtClassDefinition& def : *definitions) {
+      CallClassFileLoadHooksSingleClass<ArtJvmtiEvent::kStructuralDexFileLoadHook>(
+          gEventHandler, self, &def);
+    }
+    for (ArtClassDefinition& def : *definitions) {
+      CallClassFileLoadHooksSingleClass<ArtJvmtiEvent::kClassFileLoadHookRetransformable>(
+          gEventHandler, self, &def);
+    }
   }
 }
 
-template void Transformer::RetransformClassesDirect<RedefinitionType::kNormal>(
-      art::Thread* self, /*in-out*/std::vector<ArtClassDefinition>* definitions);
-template void Transformer::RetransformClassesDirect<RedefinitionType::kStructural>(
-      art::Thread* self, /*in-out*/std::vector<ArtClassDefinition>* definitions);
+template void Transformer::CallClassFileLoadHooks<RedefinitionType::kNormal>(
+    art::Thread* self, /*in-out*/ std::vector<ArtClassDefinition>* definitions);
+template void Transformer::CallClassFileLoadHooks<RedefinitionType::kStructural>(
+    art::Thread* self, /*in-out*/ std::vector<ArtClassDefinition>* definitions);
 
 jvmtiError Transformer::RetransformClasses(jvmtiEnv* env,
                                            jint class_count,
@@ -151,7 +174,7 @@ jvmtiError Transformer::RetransformClasses(jvmtiEnv* env,
   std::vector<ArtClassDefinition> definitions;
   jvmtiError res = OK;
   for (jint i = 0; i < class_count; i++) {
-    res = Redefiner::GetClassRedefinitionError<RedefinitionType::kNormal>(classes[i], &error_msg);
+    res = Redefiner::CanRedefineClass<RedefinitionType::kNormal>(classes[i], &error_msg);
     if (res != OK) {
       JVMTI_LOG(WARNING, env) << "FAILURE TO RETRANSFORM " << error_msg;
       return res;
@@ -164,8 +187,8 @@ jvmtiError Transformer::RetransformClasses(jvmtiEnv* env,
     }
     definitions.push_back(std::move(def));
   }
-  RetransformClassesDirect<RedefinitionType::kStructural>(self, &definitions);
-  RetransformClassesDirect<RedefinitionType::kNormal>(self, &definitions);
+
+  CallClassFileLoadHooks<RedefinitionType::kStructural>(self, &definitions);
   RedefinitionType redef_type =
       std::any_of(definitions.cbegin(),
                   definitions.cend(),
diff --git a/openjdkjvmti/transform.h b/openjdkjvmti/transform.h
index a58b50ea10..3b7f8556c1 100644
--- a/openjdkjvmti/transform.h
+++ b/openjdkjvmti/transform.h
@@ -51,16 +51,14 @@ class Transformer {
  public:
   static void Register(EventHandler* eh);
 
-  template<ArtJvmtiEvent kEvent>
-  static void TransformSingleClassDirect(
-      EventHandler* event_handler,
-      art::Thread* self,
-      /*in-out*/ArtClassDefinition* def);
+  template <ArtJvmtiEvent kEvent>
+  static void CallClassFileLoadHooksSingleClass(EventHandler* event_handler,
+                                                art::Thread* self,
+                                                /*in-out*/ ArtClassDefinition* def);
 
-  template<RedefinitionType kType>
-  static void RetransformClassesDirect(
-      art::Thread* self,
-      /*in-out*/std::vector<ArtClassDefinition>* definitions);
+  template <RedefinitionType kType>
+  static void CallClassFileLoadHooks(art::Thread* self,
+                                     /*in-out*/ std::vector<ArtClassDefinition>* definitions);
 
   static jvmtiError RetransformClasses(jvmtiEnv* env,
                                        jint class_count,