ART: Refactor Agent into Agent and AgentSpec

Separate configuration/loading and runtime state.

Test: m test-art-host
Change-Id: I69bb91e13ef10b9e1ce313f45b0b809b913d8e10
diff --git a/cmdline/cmdline_types.h b/cmdline/cmdline_types.h
index 529fe2b..d0d6bfd 100644
--- a/cmdline/cmdline_types.h
+++ b/cmdline/cmdline_types.h
@@ -328,19 +328,19 @@
 };
 
 template <>
-struct CmdlineType<std::list<ti::Agent>> : CmdlineTypeParser<std::list<ti::Agent>> {
+struct CmdlineType<std::list<ti::AgentSpec>> : CmdlineTypeParser<std::list<ti::AgentSpec>> {
   Result Parse(const std::string& args) {
     assert(false && "Use AppendValues() for an Agent list type");
     return Result::Failure("Unconditional failure: Agent list must be appended: " + args);
   }
 
   Result ParseAndAppend(const std::string& args,
-                        std::list<ti::Agent>& existing_value) {
+                        std::list<ti::AgentSpec>& existing_value) {
     existing_value.emplace_back(args);
     return Result::SuccessNoValue();
   }
 
-  static const char* Name() { return "std::list<ti::Agent>"; }
+  static const char* Name() { return "std::list<ti::AgentSpec>"; }
 };
 
 template <>
diff --git a/runtime/java_vm_ext.cc b/runtime/java_vm_ext.cc
index 104fb66..e159436 100644
--- a/runtime/java_vm_ext.cc
+++ b/runtime/java_vm_ext.cc
@@ -1052,17 +1052,17 @@
 static void* FindCodeForNativeMethodInAgents(ArtMethod* m) REQUIRES_SHARED(Locks::mutator_lock_) {
   std::string jni_short_name(m->JniShortName());
   std::string jni_long_name(m->JniLongName());
-  for (const ti::Agent& agent : Runtime::Current()->GetAgents()) {
-    void* fn = agent.FindSymbol(jni_short_name);
+  for (const std::unique_ptr<ti::Agent>& agent : Runtime::Current()->GetAgents()) {
+    void* fn = agent->FindSymbol(jni_short_name);
     if (fn != nullptr) {
       VLOG(jni) << "Found implementation for " << m->PrettyMethod()
-                << " (symbol: " << jni_short_name << ") in " << agent;
+                << " (symbol: " << jni_short_name << ") in " << *agent;
       return fn;
     }
-    fn = agent.FindSymbol(jni_long_name);
+    fn = agent->FindSymbol(jni_long_name);
     if (fn != nullptr) {
       VLOG(jni) << "Found implementation for " << m->PrettyMethod()
-                << " (symbol: " << jni_long_name << ") in " << agent;
+                << " (symbol: " << jni_long_name << ") in " << *agent;
       return fn;
     }
   }
diff --git a/runtime/parsed_options.cc b/runtime/parsed_options.cc
index 47309ed..3ac3d03 100644
--- a/runtime/parsed_options.cc
+++ b/runtime/parsed_options.cc
@@ -103,7 +103,7 @@
       //     .WithType<std::vector<ti::Agent>>().AppendValues()
       //     .IntoKey(M::AgentLib)
       .Define("-agentpath:_")
-          .WithType<std::list<ti::Agent>>().AppendValues()
+          .WithType<std::list<ti::AgentSpec>>().AppendValues()
           .IntoKey(M::AgentPath)
       .Define("-Xms_")
           .WithType<MemoryKiB>()
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index 54aa9e5..37ba9e4 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -366,7 +366,7 @@
 
   // TODO Maybe do some locking.
   for (auto& agent : agents_) {
-    agent.Unload();
+    agent->Unload();
   }
 
   // TODO Maybe do some locking
@@ -1166,7 +1166,7 @@
   madvise_random_access_ = runtime_options.GetOrDefault(Opt::MadviseRandomAccess);
 
   plugins_ = runtime_options.ReleaseOrDefault(Opt::Plugins);
-  agents_ = runtime_options.ReleaseOrDefault(Opt::AgentPath);
+  agent_specs_ = runtime_options.ReleaseOrDefault(Opt::AgentPath);
   // TODO Add back in -agentlib
   // for (auto lib : runtime_options.ReleaseOrDefault(Opt::AgentLib)) {
   //   agents_.push_back(lib);
@@ -1498,16 +1498,32 @@
 
   // Startup agents
   // TODO Maybe we should start a new thread to run these on. Investigate RI behavior more.
-  for (auto& agent : agents_) {
+  for (auto& agent_spec : agent_specs_) {
     // TODO Check err
     int res = 0;
     std::string err = "";
-    ti::Agent::LoadError result = agent.Load(&res, &err);
-    if (result == ti::Agent::kInitializationError) {
-      LOG(FATAL) << "Unable to initialize agent!";
-    } else if (result != ti::Agent::kNoError) {
-      LOG(ERROR) << "Unable to load an agent: " << err;
+    ti::LoadError error;
+    std::unique_ptr<ti::Agent> agent = agent_spec.Load(&res, &error, &err);
+
+    if (agent != nullptr) {
+      agents_.push_back(std::move(agent));
+      continue;
     }
+
+    switch (error) {
+      case ti::LoadError::kInitializationError:
+        LOG(FATAL) << "Unable to initialize agent!";
+        UNREACHABLE();
+
+      case ti::LoadError::kLoadingError:
+        LOG(ERROR) << "Unable to load an agent: " << err;
+        continue;
+
+      case ti::LoadError::kNoError:
+        break;
+    }
+    LOG(FATAL) << "Unreachable";
+    UNREACHABLE();
   }
   {
     ScopedObjectAccess soa(self);
@@ -1563,15 +1579,16 @@
     return;
   }
 
-  ti::Agent agent(agent_arg);
+  ti::AgentSpec agent_spec(agent_arg);
 
   int res = 0;
-  ti::Agent::LoadError result = agent.Attach(&res, &error_msg);
+  ti::LoadError error;
+  std::unique_ptr<ti::Agent> agent = agent_spec.Attach(&res, &error, &error_msg);
 
-  if (result == ti::Agent::kNoError) {
+  if (agent != nullptr) {
     agents_.push_back(std::move(agent));
   } else {
-    LOG(WARNING) << "Agent attach failed (result=" << result << ") : " << error_msg;
+    LOG(WARNING) << "Agent attach failed (result=" << error << ") : " << error_msg;
     ScopedObjectAccess soa(Thread::Current());
     ThrowIOException("%s", error_msg.c_str());
   }
diff --git a/runtime/runtime.h b/runtime/runtime.h
index 89caac4..ac29ed4 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -66,6 +66,7 @@
 }  // namespace mirror
 namespace ti {
 class Agent;
+class AgentSpec;
 }  // namespace ti
 namespace verifier {
 class MethodVerifier;
@@ -662,7 +663,7 @@
 
   void AttachAgent(const std::string& agent_arg);
 
-  const std::list<ti::Agent>& GetAgents() const {
+  const std::list<std::unique_ptr<ti::Agent>>& GetAgents() const {
     return agents_;
   }
 
@@ -779,7 +780,8 @@
   std::string class_path_string_;
   std::vector<std::string> properties_;
 
-  std::list<ti::Agent> agents_;
+  std::list<ti::AgentSpec> agent_specs_;
+  std::list<std::unique_ptr<ti::Agent>> agents_;
   std::vector<Plugin> plugins_;
 
   // The default stack size for managed threads created by the runtime.
diff --git a/runtime/runtime_options.def b/runtime/runtime_options.def
index 4bc8245..1dd3de5 100644
--- a/runtime/runtime_options.def
+++ b/runtime/runtime_options.def
@@ -124,8 +124,8 @@
 RUNTIME_OPTIONS_KEY (std::string,         CpuAbiList)
 RUNTIME_OPTIONS_KEY (std::string,         Fingerprint)
 RUNTIME_OPTIONS_KEY (ExperimentalFlags,   Experimental,     ExperimentalFlags::kNone) // -Xexperimental:{...}
-RUNTIME_OPTIONS_KEY (std::list<ti::Agent>,         AgentLib)  // -agentlib:<libname>=<options>
-RUNTIME_OPTIONS_KEY (std::list<ti::Agent>,         AgentPath)  // -agentpath:<libname>=<options>
+RUNTIME_OPTIONS_KEY (std::list<ti::AgentSpec>,         AgentLib)  // -agentlib:<libname>=<options>
+RUNTIME_OPTIONS_KEY (std::list<ti::AgentSpec>,         AgentPath)  // -agentpath:<libname>=<options>
 RUNTIME_OPTIONS_KEY (std::vector<Plugin>,            Plugins)  // -Xplugin:<library>
 
 // Not parse-able from command line, but can be provided explicitly.
diff --git a/runtime/ti/agent.cc b/runtime/ti/agent.cc
index 548752e..aa09d84 100644
--- a/runtime/ti/agent.cc
+++ b/runtime/ti/agent.cc
@@ -33,31 +33,54 @@
 const char* AGENT_ON_ATTACH_FUNCTION_NAME = "Agent_OnAttach";
 const char* AGENT_ON_UNLOAD_FUNCTION_NAME = "Agent_OnUnload";
 
+AgentSpec::AgentSpec(const std::string& arg) {
+  size_t eq = arg.find_first_of('=');
+  if (eq == std::string::npos) {
+    name_ = arg;
+  } else {
+    name_ = arg.substr(0, eq);
+    args_ = arg.substr(eq + 1, arg.length());
+  }
+}
+
+std::unique_ptr<Agent> AgentSpec::Load(/*out*/jint* call_res,
+                                       /*out*/LoadError* error,
+                                       /*out*/std::string* error_msg) {
+  VLOG(agents) << "Loading agent: " << name_ << " " << args_;
+  return DoLoadHelper(false, call_res, error, error_msg);
+}
+
+// Tries to attach the agent using its OnAttach method. Returns true on success.
+std::unique_ptr<Agent> AgentSpec::Attach(/*out*/jint* call_res,
+                                         /*out*/LoadError* error,
+                                         /*out*/std::string* error_msg) {
+  VLOG(agents) << "Attaching agent: " << name_ << " " << args_;
+  return DoLoadHelper(true, call_res, error, error_msg);
+}
+
+
 // TODO We need to acquire some locks probably.
-Agent::LoadError Agent::DoLoadHelper(bool attaching,
-                                     /*out*/jint* call_res,
-                                     /*out*/std::string* error_msg) {
+std::unique_ptr<Agent> AgentSpec::DoLoadHelper(bool attaching,
+                                               /*out*/jint* call_res,
+                                               /*out*/LoadError* error,
+                                               /*out*/std::string* error_msg) {
   ScopedThreadStateChange stsc(Thread::Current(), ThreadState::kNative);
   DCHECK(call_res != nullptr);
   DCHECK(error_msg != nullptr);
 
-  if (IsStarted()) {
-    *error_msg = StringPrintf("the agent at %s has already been started!", name_.c_str());
+  std::unique_ptr<Agent> agent = DoDlOpen(error, error_msg);
+  if (agent == nullptr) {
     VLOG(agents) << "err: " << *error_msg;
-    return kAlreadyStarted;
+    return nullptr;
   }
-  LoadError err = DoDlOpen(error_msg);
-  if (err != kNoError) {
-    VLOG(agents) << "err: " << *error_msg;
-    return err;
-  }
-  AgentOnLoadFunction callback = attaching ? onattach_ : onload_;
+  AgentOnLoadFunction callback = attaching ? agent->onattach_ : agent->onload_;
   if (callback == nullptr) {
     *error_msg = StringPrintf("Unable to start agent %s: No %s callback found",
                               (attaching ? "attach" : "load"),
                               name_.c_str());
     VLOG(agents) << "err: " << *error_msg;
-    return kLoadingError;
+    *error = kLoadingError;
+    return nullptr;
   }
   // Need to let the function fiddle with the array.
   std::unique_ptr<char[]> copied_args(new char[args_.size() + 1]);
@@ -70,44 +93,36 @@
     *error_msg = StringPrintf("Initialization of %s returned non-zero value of %d",
                               name_.c_str(), *call_res);
     VLOG(agents) << "err: " << *error_msg;
-    return kInitializationError;
-  } else {
-    return kNoError;
+    *error = kInitializationError;
+    return nullptr;
   }
+  return agent;
 }
 
-void* Agent::FindSymbol(const std::string& name) const {
-  CHECK(IsStarted()) << "Cannot find symbols in an unloaded agent library " << this;
-  return dlsym(dlopen_handle_, name.c_str());
-}
-
-Agent::LoadError Agent::DoDlOpen(/*out*/std::string* error_msg) {
+std::unique_ptr<Agent> AgentSpec::DoDlOpen(/*out*/LoadError* error, /*out*/std::string* error_msg) {
   DCHECK(error_msg != nullptr);
 
-  DCHECK(dlopen_handle_ == nullptr);
-  DCHECK(onload_ == nullptr);
-  DCHECK(onattach_ == nullptr);
-  DCHECK(onunload_ == nullptr);
-
-  dlopen_handle_ = dlopen(name_.c_str(), RTLD_LAZY);
-  if (dlopen_handle_ == nullptr) {
+  void* dlopen_handle = dlopen(name_.c_str(), RTLD_LAZY);
+  if (dlopen_handle == nullptr) {
     *error_msg = StringPrintf("Unable to dlopen %s: %s", name_.c_str(), dlerror());
-    return kLoadingError;
+    *error = kLoadingError;
+    return nullptr;
   }
 
-  onload_ = reinterpret_cast<AgentOnLoadFunction>(FindSymbol(AGENT_ON_LOAD_FUNCTION_NAME));
-  if (onload_ == nullptr) {
-    VLOG(agents) << "Unable to find 'Agent_OnLoad' symbol in " << this;
-  }
-  onattach_ = reinterpret_cast<AgentOnLoadFunction>(FindSymbol(AGENT_ON_ATTACH_FUNCTION_NAME));
-  if (onattach_ == nullptr) {
-    VLOG(agents) << "Unable to find 'Agent_OnAttach' symbol in " << this;
-  }
-  onunload_ = reinterpret_cast<AgentOnUnloadFunction>(FindSymbol(AGENT_ON_UNLOAD_FUNCTION_NAME));
-  if (onunload_ == nullptr) {
-    VLOG(agents) << "Unable to find 'Agent_OnUnload' symbol in " << this;
-  }
-  return kNoError;
+  std::unique_ptr<Agent> agent(new Agent(name_, dlopen_handle));
+  agent->PopulateFunctions();
+  *error = kNoError;
+  return agent;
+}
+
+std::ostream& operator<<(std::ostream &os, AgentSpec const& m) {
+  return os << "AgentSpec { name=\"" << m.name_ << "\", args=\"" << m.args_ << "\" }";
+}
+
+
+void* Agent::FindSymbol(const std::string& name) const {
+  CHECK(dlopen_handle_ != nullptr) << "Cannot find symbols in an unloaded agent library " << this;
+  return dlsym(dlopen_handle_, name.c_str());
 }
 
 // TODO Lock some stuff probably.
@@ -127,58 +142,6 @@
   }
 }
 
-Agent::Agent(const std::string& arg)
-    : dlopen_handle_(nullptr),
-      onload_(nullptr),
-      onattach_(nullptr),
-      onunload_(nullptr) {
-  size_t eq = arg.find_first_of('=');
-  if (eq == std::string::npos) {
-    name_ = arg;
-  } else {
-    name_ = arg.substr(0, eq);
-    args_ = arg.substr(eq + 1, arg.length());
-  }
-}
-
-Agent::Agent(const Agent& other)
-    : dlopen_handle_(nullptr),
-      onload_(nullptr),
-      onattach_(nullptr),
-      onunload_(nullptr) {
-  *this = other;
-}
-
-// Attempting to copy to/from loaded/started agents is a fatal error
-Agent& Agent::operator=(const Agent& other) {
-  if (this != &other) {
-    if (other.dlopen_handle_ != nullptr) {
-      LOG(FATAL) << "Attempting to copy a loaded agent!";
-    }
-
-    if (dlopen_handle_ != nullptr) {
-      LOG(FATAL) << "Attempting to assign into a loaded agent!";
-    }
-
-    DCHECK(other.onload_ == nullptr);
-    DCHECK(other.onattach_ == nullptr);
-    DCHECK(other.onunload_ == nullptr);
-
-    DCHECK(onload_ == nullptr);
-    DCHECK(onattach_ == nullptr);
-    DCHECK(onunload_ == nullptr);
-
-    name_ = other.name_;
-    args_ = other.args_;
-
-    dlopen_handle_ = nullptr;
-    onload_ = nullptr;
-    onattach_ = nullptr;
-    onunload_ = nullptr;
-  }
-  return *this;
-}
-
 Agent::Agent(Agent&& other)
     : dlopen_handle_(nullptr),
       onload_(nullptr),
@@ -190,10 +153,9 @@
 Agent& Agent::operator=(Agent&& other) {
   if (this != &other) {
     if (dlopen_handle_ != nullptr) {
-      dlclose(dlopen_handle_);
+      Unload();
     }
     name_ = std::move(other.name_);
-    args_ = std::move(other.args_);
     dlopen_handle_ = other.dlopen_handle_;
     onload_ = other.onload_;
     onattach_ = other.onattach_;
@@ -206,9 +168,24 @@
   return *this;
 }
 
+void Agent::PopulateFunctions() {
+  onload_ = reinterpret_cast<AgentOnLoadFunction>(FindSymbol(AGENT_ON_LOAD_FUNCTION_NAME));
+  if (onload_ == nullptr) {
+    VLOG(agents) << "Unable to find 'Agent_OnLoad' symbol in " << this;
+  }
+  onattach_ = reinterpret_cast<AgentOnLoadFunction>(FindSymbol(AGENT_ON_ATTACH_FUNCTION_NAME));
+  if (onattach_ == nullptr) {
+    VLOG(agents) << "Unable to find 'Agent_OnAttach' symbol in " << this;
+  }
+  onunload_ = reinterpret_cast<AgentOnUnloadFunction>(FindSymbol(AGENT_ON_UNLOAD_FUNCTION_NAME));
+  if (onunload_ == nullptr) {
+    VLOG(agents) << "Unable to find 'Agent_OnUnload' symbol in " << this;
+  }
+}
+
 Agent::~Agent() {
   if (dlopen_handle_ != nullptr) {
-    dlclose(dlopen_handle_);
+    Unload();
   }
 }
 
@@ -217,8 +194,7 @@
 }
 
 std::ostream& operator<<(std::ostream &os, Agent const& m) {
-  return os << "Agent { name=\"" << m.name_ << "\", args=\"" << m.args_ << "\", handle="
-            << m.dlopen_handle_ << " }";
+  return os << "Agent { name=\"" << m.name_ << "\", handle=" << m.dlopen_handle_ << " }";
 }
 
 }  // namespace ti
diff --git a/runtime/ti/agent.h b/runtime/ti/agent.h
index d6f1f2e..68b4948 100644
--- a/runtime/ti/agent.h
+++ b/runtime/ti/agent.h
@@ -20,11 +20,62 @@
 #include <dlfcn.h>
 #include <jni.h>  // for jint, JavaVM* etc declarations
 
+#include <memory>
+
 #include "base/logging.h"
 
 namespace art {
 namespace ti {
 
+class Agent;
+
+enum LoadError {
+  kNoError,              // No error occurred..
+  kLoadingError,         // dlopen or dlsym returned an error.
+  kInitializationError,  // The entrypoint did not return 0. This might require an abort.
+};
+
+class AgentSpec {
+ public:
+  explicit AgentSpec(const std::string& arg);
+
+  const std::string& GetName() const {
+    return name_;
+  }
+
+  const std::string& GetArgs() const {
+    return args_;
+  }
+
+  bool HasArgs() const {
+    return !GetArgs().empty();
+  }
+
+  std::unique_ptr<Agent> Load(/*out*/jint* call_res,
+                              /*out*/LoadError* error,
+                              /*out*/std::string* error_msg);
+
+  // Tries to attach the agent using its OnAttach method. Returns true on success.
+  std::unique_ptr<Agent> Attach(/*out*/jint* call_res,
+                                /*out*/LoadError* error,
+                                /*out*/std::string* error_msg);
+
+ private:
+  std::unique_ptr<Agent> DoDlOpen(/*out*/LoadError* error, /*out*/std::string* error_msg);
+
+  std::unique_ptr<Agent> DoLoadHelper(bool attaching,
+                                      /*out*/jint* call_res,
+                                      /*out*/LoadError* error,
+                                      /*out*/std::string* error_msg);
+
+  std::string name_;
+  std::string args_;
+
+  friend std::ostream& operator<<(std::ostream &os, AgentSpec const& m);
+};
+
+std::ostream& operator<<(std::ostream &os, AgentSpec const& m);
+
 using AgentOnLoadFunction = jint (*)(JavaVM*, const char*, void*);
 using AgentOnUnloadFunction = void (*)(JavaVM*);
 
@@ -38,64 +89,32 @@
 // TODO Support native-bridge. Currently agents can only be the actual runtime ISA of the device.
 class Agent {
  public:
-  enum LoadError {
-    kNoError,              // No error occurred..
-    kAlreadyStarted,       // The agent has already been loaded.
-    kLoadingError,         // dlopen or dlsym returned an error.
-    kInitializationError,  // The entrypoint did not return 0. This might require an abort.
-  };
-
-  bool IsStarted() const {
-    return dlopen_handle_ != nullptr;
-  }
-
   const std::string& GetName() const {
     return name_;
   }
 
-  const std::string& GetArgs() const {
-    return args_;
-  }
-
-  bool HasArgs() const {
-    return !GetArgs().empty();
-  }
-
   void* FindSymbol(const std::string& name) const;
 
-  LoadError Load(/*out*/jint* call_res, /*out*/std::string* error_msg) {
-    VLOG(agents) << "Loading agent: " << name_ << " " << args_;
-    return DoLoadHelper(false, call_res, error_msg);
-  }
-
   // TODO We need to acquire some locks probably.
   void Unload();
 
-  // Tries to attach the agent using its OnAttach method. Returns true on success.
-  LoadError Attach(/*out*/jint* call_res, /*out*/std::string* error_msg) {
-    VLOG(agents) << "Attaching agent: " << name_ << " " << args_;
-    return DoLoadHelper(true, call_res, error_msg);
-  }
-
-  explicit Agent(const std::string& arg);
-
-  Agent(const Agent& other);
-  Agent& operator=(const Agent& other);
-
   Agent(Agent&& other);
   Agent& operator=(Agent&& other);
 
   ~Agent();
 
  private:
-  LoadError DoDlOpen(/*out*/std::string* error_msg);
+  Agent(const std::string& name, void* dlopen_handle) : name_(name),
+                                                        dlopen_handle_(dlopen_handle),
+                                                        onload_(nullptr),
+                                                        onattach_(nullptr),
+                                                        onunload_(nullptr) {
+    DCHECK(dlopen_handle != nullptr);
+  }
 
-  LoadError DoLoadHelper(bool attaching,
-                         /*out*/jint* call_res,
-                         /*out*/std::string* error_msg);
+  void PopulateFunctions();
 
   std::string name_;
-  std::string args_;
   void* dlopen_handle_;
 
   // The entrypoints.
@@ -103,7 +122,10 @@
   AgentOnLoadFunction onattach_;
   AgentOnUnloadFunction onunload_;
 
+  friend class AgentSpec;
   friend std::ostream& operator<<(std::ostream &os, Agent const& m);
+
+  DISALLOW_COPY_AND_ASSIGN(Agent);
 };
 
 std::ostream& operator<<(std::ostream &os, Agent const& m);