Follow up 129144

Passes JDWP options to debugger on runtime init so we no longer need
to keep them on the heap.

Updates ParseJdwpOption to return Result for consistency.

Bug: 19275792
Change-Id: I68b7e58908164d3e4cf9e3fbcc3dfab6ce0579a5

5 files changed, 32 insertions, 40 deletions

diff --git a/cmdline/cmdline_types.h b/cmdline/cmdline_types.h
index 45f9b5635c..180baeced0 100644
--- a/cmdline/cmdline_types.h
+++ b/cmdline/cmdline_types.h
@@ -88,7 +88,6 @@ struct CmdlineType<JDWP::JdwpOptions> : CmdlineTypeParser<JDWP::JdwpOptions> {
     Split(options, ',', &pairs);
 
     JDWP::JdwpOptions jdwp_options;
-    std::stringstream error_stream;
 
     for (const std::string& jdwp_option : pairs) {
       std::string::size_type equals_pos = jdwp_option.find('=');
@@ -97,11 +96,12 @@ struct CmdlineType<JDWP::JdwpOptions> : CmdlineTypeParser<JDWP::JdwpOptions> {
             "Can't parse JDWP option '" + jdwp_option + "' in '" + options + "'");
       }
 
-      if (!ParseJdwpOption(jdwp_option.substr(0, equals_pos),
-                           jdwp_option.substr(equals_pos + 1),
-                           error_stream,
-                           &jdwp_options)) {
-        return Result::Failure(error_stream.str());
+      Result parse_attempt = ParseJdwpOption(jdwp_option.substr(0, equals_pos),
+                                             jdwp_option.substr(equals_pos + 1),
+                                             &jdwp_options);
+      if (parse_attempt.IsError()) {
+        // We fail to parse this JDWP option.
+        return parse_attempt;
       }
     }
 
@@ -115,17 +115,15 @@ struct CmdlineType<JDWP::JdwpOptions> : CmdlineTypeParser<JDWP::JdwpOptions> {
     return Result::Success(std::move(jdwp_options));
   }
 
-  bool ParseJdwpOption(const std::string& name, const std::string& value,
-                       std::ostream& error_stream,
-                       JDWP::JdwpOptions* jdwp_options) {
+  Result ParseJdwpOption(const std::string& name, const std::string& value,
+                         JDWP::JdwpOptions* jdwp_options) {
     if (name == "transport") {
       if (value == "dt_socket") {
         jdwp_options->transport = JDWP::kJdwpTransportSocket;
       } else if (value == "dt_android_adb") {
         jdwp_options->transport = JDWP::kJdwpTransportAndroidAdb;
       } else {
-        error_stream << "JDWP transport not supported: " << value;
-        return false;
+        return Result::Failure("JDWP transport not supported: " + value);
       }
     } else if (name == "server") {
       if (value == "n") {
@@ -133,8 +131,7 @@ struct CmdlineType<JDWP::JdwpOptions> : CmdlineTypeParser<JDWP::JdwpOptions> {
       } else if (value == "y") {
         jdwp_options->server = true;
       } else {
-        error_stream << "JDWP option 'server' must be 'y' or 'n'";
-        return false;
+        return Result::Failure("JDWP option 'server' must be 'y' or 'n'");
       }
     } else if (name == "suspend") {
       if (value == "n") {
@@ -142,8 +139,7 @@ struct CmdlineType<JDWP::JdwpOptions> : CmdlineTypeParser<JDWP::JdwpOptions> {
       } else if (value == "y") {
         jdwp_options->suspend = true;
       } else {
-        error_stream << "JDWP option 'suspend' must be 'y' or 'n'";
-        return false;
+        return Result::Failure("JDWP option 'suspend' must be 'y' or 'n'");
       }
     } else if (name == "address") {
       /* this is either <port> or <host>:<port> */
@@ -157,14 +153,12 @@ struct CmdlineType<JDWP::JdwpOptions> : CmdlineTypeParser<JDWP::JdwpOptions> {
         port_string = value;
       }
       if (port_string.empty()) {
-        error_stream << "JDWP address missing port: " << value;
-        return false;
+        return Result::Failure("JDWP address missing port: " + value);
       }
       char* end;
       uint64_t port = strtoul(port_string.c_str(), &end, 10);
       if (*end != '\0' || port > 0xffff) {
-        error_stream << "JDWP address has junk in port field: " << value;
-        return false;
+        return Result::Failure("JDWP address has junk in port field: " + value);
       }
       jdwp_options->port = port;
     } else if (name == "launch" || name == "onthrow" || name == "oncaught" || name == "timeout") {
@@ -174,7 +168,7 @@ struct CmdlineType<JDWP::JdwpOptions> : CmdlineTypeParser<JDWP::JdwpOptions> {
       LOG(INFO) << "Ignoring unrecognized JDWP option '" << name << "'='" << value << "'";
     }
 
-    return true;
+    return Result::SuccessNoValue();
   }
 
   static const char* Name() { return "JdwpOptions"; }
diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index 678b29a51a..a3d3b470cc 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -297,6 +297,9 @@ static bool gJdwpAllowed = true;
 // Was there a -Xrunjdwp or -agentlib:jdwp= argument on the command line?
 static bool gJdwpConfigured = false;
 
+// JDWP options for debugging. Only valid if IsJdwpConfigured() is true.
+static JDWP::JdwpOptions gJdwpOptions;
+
 // Runtime JDWP state.
 static JDWP::JdwpState* gJdwpState = nullptr;
 static bool gDebuggerConnected;  // debugger or DDMS is connected.
@@ -529,13 +532,11 @@ static bool IsPrimitiveTag(JDWP::JdwpTag tag) {
   }
 }
 
-void Dbg::StartJdwp(const JDWP::JdwpOptions* jdwp_options) {
-  gJdwpConfigured = (jdwp_options != nullptr);
+void Dbg::StartJdwp() {
   if (!gJdwpAllowed || !IsJdwpConfigured()) {
     // No JDWP for you!
     return;
   }
-  DCHECK_NE(jdwp_options->transport, JDWP::kJdwpTransportUnknown);
 
   CHECK(gRegistry == nullptr);
   gRegistry = new ObjectRegistry;
@@ -543,7 +544,7 @@ void Dbg::StartJdwp(const JDWP::JdwpOptions* jdwp_options) {
   // Init JDWP if the debugger is enabled. This may connect out to a
   // debugger, passively listen for a debugger, or block waiting for a
   // debugger.
-  gJdwpState = JDWP::JdwpState::Create(jdwp_options);
+  gJdwpState = JDWP::JdwpState::Create(&gJdwpOptions);
   if (gJdwpState == nullptr) {
     // We probably failed because some other process has the port already, which means that
     // if we don't abort the user is likely to think they're talking to us when they're actually
@@ -720,6 +721,12 @@ bool Dbg::IsDebuggerActive() {
   return gDebuggerActive;
 }
 
+void Dbg::ConfigureJdwp(const JDWP::JdwpOptions& jdwp_options) {
+  CHECK_NE(jdwp_options.transport, JDWP::kJdwpTransportUnknown);
+  gJdwpOptions = jdwp_options;
+  gJdwpConfigured = true;
+}
+
 bool Dbg::IsJdwpConfigured() {
   return gJdwpConfigured;
 }
diff --git a/runtime/debugger.h b/runtime/debugger.h
index 6762608cf2..0c22148c99 100644
--- a/runtime/debugger.h
+++ b/runtime/debugger.h
@@ -228,7 +228,7 @@ class Dbg {
 
   static void SetJdwpAllowed(bool allowed);
 
-  static void StartJdwp(const JDWP::JdwpOptions* jdwp_options);
+  static void StartJdwp();
   static void StopJdwp();
 
   // Invoked by the GC in case we need to keep DDMS informed.
@@ -254,6 +254,9 @@ class Dbg {
   // just DDMS (or nothing at all).
   static bool IsDebuggerActive();
 
+  // Configures JDWP with parsed command-line options.
+  static void ConfigureJdwp(const JDWP::JdwpOptions& jdwp_options);
+
   // Returns true if we had -Xrunjdwp or -agentlib:jdwp= on the command line.
   static bool IsJdwpConfigured();
 
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index 67049630c3..c6e858bdd4 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -173,8 +173,7 @@ Runtime::Runtime()
       implicit_null_checks_(false),
       implicit_so_checks_(false),
       implicit_suspend_checks_(false),
-      is_native_bridge_loaded_(false),
-      jdwp_options_(nullptr) {
+      is_native_bridge_loaded_(false) {
   CheckAsmSupportOffsetsAndSizes();
 }
 
@@ -228,10 +227,6 @@ Runtime::~Runtime() {
 
   // Make sure our internal threads are dead before we start tearing down things they're using.
   Dbg::StopJdwp();
-  if (jdwp_options_ != nullptr) {
-    delete jdwp_options_;
-  }
-
   delete signal_catcher_;
 
   // Make sure all other non-daemon threads have terminated, and all daemon threads are suspended.
@@ -595,7 +590,7 @@ void Runtime::DidForkFromZygote(JNIEnv* env, NativeBridgeAction action, const ch
 
   // Start the JDWP thread. If the command-line debugger flags specified "suspend=y",
   // this will pause the runtime, so we probably want this to come last.
-  Dbg::StartJdwp(jdwp_options_);
+  Dbg::StartJdwp();
 }
 
 void Runtime::StartSignalCatcher() {
@@ -805,8 +800,7 @@ bool Runtime::Init(const RuntimeOptions& raw_options, bool ignore_unrecognized)
   dump_gc_performance_on_shutdown_ = runtime_options.Exists(Opt::DumpGCPerformanceOnShutdown);
 
   if (runtime_options.Exists(Opt::JdwpOptions)) {
-    JDWP::JdwpOptions options = runtime_options.GetOrDefault(Opt::JdwpOptions);
-    jdwp_options_ = new JDWP::JdwpOptions(options);
+    Dbg::ConfigureJdwp(runtime_options.GetOrDefault(Opt::JdwpOptions));
   }
 
   BlockSignals();
diff --git a/runtime/runtime.h b/runtime/runtime.h
index d98eb8d5d2..118c838397 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -47,9 +47,6 @@ namespace gc {
     class GarbageCollector;
   }  // namespace collector
 }  // namespace gc
-namespace JDWP {
-  struct JdwpOptions;
-}  // namespace JDWP
 namespace mirror {
   class ArtMethod;
   class ClassLoader;
@@ -685,9 +682,6 @@ class Runtime {
   // that there's no native bridge.
   bool is_native_bridge_loaded_;
 
-  // JDWP options for debugging.
-  const JDWP::JdwpOptions* jdwp_options_;
-
   DISALLOW_COPY_AND_ASSIGN(Runtime);
 };
 std::ostream& operator<<(std::ostream& os, const Runtime::CalleeSaveType& rhs);