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
diff --git a/cmdline/cmdline_types.h b/cmdline/cmdline_types.h
index 45f9b56..180baec 100644
--- a/cmdline/cmdline_types.h
+++ b/cmdline/cmdline_types.h
@@ -88,7 +88,6 @@
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 @@
"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 @@
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 @@
} 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 @@
} 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 @@
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 @@
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 678b29a..a3d3b47 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -297,6 +297,9 @@
// 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 @@
}
}
-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 @@
// 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 @@
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 6762608..0c22148 100644
--- a/runtime/debugger.h
+++ b/runtime/debugger.h
@@ -228,7 +228,7 @@
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 @@
// 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 6704963..c6e858b 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -173,8 +173,7 @@
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 @@
// 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 @@
// 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 @@
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 d98eb8d..118c838 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -47,9 +47,6 @@
class GarbageCollector;
} // namespace collector
} // namespace gc
-namespace JDWP {
- struct JdwpOptions;
-} // namespace JDWP
namespace mirror {
class ArtMethod;
class ClassLoader;
@@ -685,9 +682,6 @@
// 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);
