8205906: jdk.jfr.jcmd.TestJcmdDumpLimited fails due to erronous processing of -XX:FlightRecorderOptions

Reviewed-by: egahlin

src/hotspot/share/jfr/jfr.cpp

@@ -85,12 +85,12 @@ void Jfr::weak_oops_do(BoolObjectClosure* is_alive, OopClosure* f) {
   LeakProfiler::oops_do(is_alive, f);
 }
 
-bool Jfr::on_start_flight_recording_option(const JavaVMOption** option, char* tail) {
-  return JfrOptionSet::parse_start_flight_recording_option(option, tail);
+bool Jfr::on_flight_recorder_option(const JavaVMOption** option, char* delimiter) {
+  return JfrOptionSet::parse_flight_recorder_option(option, delimiter);
 }
 
-bool Jfr::on_flight_recorder_option(const JavaVMOption** option, char* tail) {
-  return JfrOptionSet::parse_flight_recorder_option(option, tail);
+bool Jfr::on_start_flight_recording_option(const JavaVMOption** option, char* delimiter) {
+  return JfrOptionSet::parse_start_flight_recording_option(option, delimiter);
 }
 
 Thread* Jfr::sampler_thread() {

src/hotspot/share/jfr/jfr.hpp

@@ -49,8 +49,8 @@ class Jfr : AllStatic {
   static void on_thread_exit(JavaThread* thread);
   static void on_thread_destruct(Thread* thread);
   static void on_vm_shutdown(bool exception_handler = false);
-  static bool on_start_flight_recording_option(const JavaVMOption** option, char* tail);
-  static bool on_flight_recorder_option(const JavaVMOption** option, char* tail);
+  static bool on_flight_recorder_option(const JavaVMOption** option, char* delimiter);
+  static bool on_start_flight_recording_option(const JavaVMOption** option, char* delimiter);
   static void weak_oops_do(BoolObjectClosure* is_alive, OopClosure* f);
   static Thread* sampler_thread();
 };

src/hotspot/share/jfr/recorder/jfrRecorder.cpp

@@ -46,6 +46,7 @@
 #include "runtime/handles.inline.hpp"
 #include "runtime/flags/jvmFlag.hpp"
 #include "runtime/globals.hpp"
+#include "utilities/growableArray.hpp"
 
 static bool is_disabled_on_command_line() {
   static const size_t length = strlen("FlightRecorder");
@@ -85,31 +86,30 @@ bool JfrRecorder::on_vm_init() {
   return JfrTime::initialize();
 }
 
-static JfrStartFlightRecordingDCmd* _startup_recording = NULL;
+static GrowableArray<JfrStartFlightRecordingDCmd*>* dcmd_recordings_array = NULL;
 
-static void release_startup_recording() {
-  if (_startup_recording != NULL) {
-    delete _startup_recording;
-    _startup_recording = NULL;
+static void release_recordings() {
+  if (dcmd_recordings_array != NULL) {
+    const int length = dcmd_recordings_array->length();
+    for (int i = 0; i < length; ++i) {
+      delete dcmd_recordings_array->at(i);
+    }
+    delete dcmd_recordings_array;
+    dcmd_recordings_array = NULL;
   }
 }
 
 static void teardown_startup_support() {
-  release_startup_recording();
-  JfrOptionSet::release_startup_recordings();
+  release_recordings();
+  JfrOptionSet::release_startup_recording_options();
 }
 
-
 // Parsing options here to detect errors as soon as possible
-static bool parse_recording_options(const char* options, TRAPS) {
+static bool parse_recording_options(const char* options, JfrStartFlightRecordingDCmd* dcmd_recording, TRAPS) {
   assert(options != NULL, "invariant");
-  if (_startup_recording != NULL) {
-    delete _startup_recording;
-  }
+  assert(dcmd_recording != NULL, "invariant");
   CmdLine cmdline(options, strlen(options), true);
-  _startup_recording = new (ResourceObj::C_HEAP, mtTracing) JfrStartFlightRecordingDCmd(tty, true);
-  assert(_startup_recording != NULL, "invariant");
-  _startup_recording->parse(&cmdline, ',', THREAD);
+  dcmd_recording->parse(&cmdline, ',', THREAD);
   if (HAS_PENDING_EXCEPTION) {
     java_lang_Throwable::print(PENDING_EXCEPTION, tty);
     CLEAR_PENDING_EXCEPTION;
@@ -119,24 +119,30 @@ static bool parse_recording_options(const char* options, TRAPS) {
 }
 
 static bool validate_recording_options(TRAPS) {
-  const GrowableArray<const char*>* startup_options = JfrOptionSet::startup_recordings();
-  if (startup_options == NULL) {
+  const GrowableArray<const char*>* options = JfrOptionSet::startup_recording_options();
+  if (options == NULL) {
     return true;
   }
-  const int length = startup_options->length();
+  const int length = options->length();
   assert(length >= 1, "invariant");
+  assert(dcmd_recordings_array == NULL, "invariant");
+  dcmd_recordings_array = new (ResourceObj::C_HEAP, mtTracing)GrowableArray<JfrStartFlightRecordingDCmd*>(length, true, mtTracing);
+  assert(dcmd_recordings_array != NULL, "invariant");
   for (int i = 0; i < length; ++i) {
-    if (!parse_recording_options(startup_options->at(i), THREAD)) {
+    JfrStartFlightRecordingDCmd* const dcmd_recording = new(ResourceObj::C_HEAP, mtTracing) JfrStartFlightRecordingDCmd(tty, true);
+    assert(dcmd_recording != NULL, "invariant");
+    dcmd_recordings_array->append(dcmd_recording);
+    if (!parse_recording_options(options->at(i), dcmd_recording, THREAD)) {
       return false;
     }
   }
   return true;
 }
 
-static bool launch_recording(TRAPS) {
-  assert(_startup_recording != NULL, "invariant");
+static bool launch_recording(JfrStartFlightRecordingDCmd* dcmd_recording, TRAPS) {
+  assert(dcmd_recording != NULL, "invariant");
   log_trace(jfr, system)("Starting a recording");
-  _startup_recording->execute(DCmd_Source_Internal, Thread::current());
+  dcmd_recording->execute(DCmd_Source_Internal, THREAD);
   if (HAS_PENDING_EXCEPTION) {
     log_debug(jfr, system)("Exception while starting a recording");
     CLEAR_PENDING_EXCEPTION;
@@ -146,31 +152,20 @@ static bool launch_recording(TRAPS) {
   return true;
 }
 
-static bool launch_recordings(const GrowableArray<const char*>* startup_options, TRAPS) {
-  assert(startup_options != NULL, "invariant");
-  const int length = startup_options->length();
-  assert(length >= 1, "invariant");
-  if (length == 1) {
-    // already parsed and ready, launch it
-    return launch_recording(THREAD);
-  }
-  for (int i = 0; i < length; ++i) {
-    parse_recording_options(startup_options->at(i), THREAD);
-    if (!launch_recording(THREAD)) {
-      return false;
+static bool launch_recordings(TRAPS) {
+  bool result = true;
+  if (dcmd_recordings_array != NULL) {
+    const int length = dcmd_recordings_array->length();
+    assert(length >= 1, "invariant");
+    for (int i = 0; i < length; ++i) {
+      if (!launch_recording(dcmd_recordings_array->at(i), THREAD)) {
+        result = false;
+        break;
+      }
     }
   }
-  return true;
-}
-
-static bool startup_recordings(TRAPS) {
-  const GrowableArray<const char*>* startup_options = JfrOptionSet::startup_recordings();
-  if (startup_options == NULL) {
-    return true;
-  }
-  const bool ret = launch_recordings(startup_options, THREAD);
   teardown_startup_support();
-  return ret;
+  return result;
 }
 
 static void log_jdk_jfr_module_resolution_error(TRAPS) {
@@ -180,13 +175,20 @@ static void log_jdk_jfr_module_resolution_error(TRAPS) {
   JfrJavaSupport::is_jdk_jfr_module_available(&stream, THREAD);
 }
 
-bool JfrRecorder::on_vm_start() {
-  if (DumpSharedSpaces && (JfrOptionSet::startup_recordings() != NULL)) {
+static bool is_cds_dump_requested() {
+  // we will not be able to launch recordings if a cds dump is being requested
+  if (DumpSharedSpaces && (JfrOptionSet::startup_recording_options() != NULL)) {
     warning("JFR will be disabled during CDS dumping");
     teardown_startup_support();
     return true;
   }
-  const bool in_graph = JfrJavaSupport::is_jdk_jfr_module_available();
+  return false;
+}
+
+bool JfrRecorder::on_vm_start() {
+  if (is_cds_dump_requested()) {
+    return true;
+  }
   Thread* const thread = Thread::current();
   if (!JfrOptionSet::initialize(thread)) {
     return false;
@@ -194,10 +196,13 @@ bool JfrRecorder::on_vm_start() {
   if (!register_jfr_dcmds()) {
     return false;
   }
-  if (!validate_recording_options(thread)) {
-    return false;
-  }
+
+  const bool in_graph = JfrJavaSupport::is_jdk_jfr_module_available();
+
   if (in_graph) {
+    if (!validate_recording_options(thread)) {
+      return false;
+    }
     if (!JfrJavaEventWriter::initialize()) {
       return false;
     }
@@ -205,14 +210,17 @@ bool JfrRecorder::on_vm_start() {
       return false;
     }
   }
+
   if (!is_enabled()) {
     return true;
   }
+
   if (!in_graph) {
     log_jdk_jfr_module_resolution_error(thread);
     return false;
   }
-  return startup_recordings(thread);
+
+  return launch_recordings(thread);
 }
 
 static bool _created = false;

src/hotspot/share/jfr/recorder/service/jfrOptionSet.cpp

@@ -665,59 +665,69 @@ bool JfrOptionSet::adjust_memory_options() {
   return true;
 }
 
-static GrowableArray<const char*>* startup_recording_array = NULL;
+static const char XXFlightRecorderOptions[] = "-XX:FlightRecorderOptions";
 
-bool JfrOptionSet::parse_start_flight_recording_option(const JavaVMOption** option, char* tail) {
+bool JfrOptionSet::parse_flight_recorder_option(const JavaVMOption** option, char* delimiter) {
   assert(option != NULL, "invariant");
-  assert(tail != NULL, "invariant");
+  assert(delimiter != NULL, "invariant");
   assert((*option)->optionString != NULL, "invariant");
-  assert(strncmp((*option)->optionString, "-XX:StartFlightRecording", 24) == 0, "invariant");
-  const char* param_string = NULL;
-  if (*tail == '\0') {
-    // Add dummy dumponexit=false so -XX:StartFlightRecording can be used without a parameter.
+  assert(strncmp((*option)->optionString, XXFlightRecorderOptions, sizeof XXFlightRecorderOptions - 1) == 0, "invariant");
+  if (*delimiter == '\0') {
+    // -XX:FlightRecorderOptions without any delimiter and values
+  } else {
+    // -XX:FlightRecorderOptions[=|:]
+    // set delimiter to '='
+    *delimiter = '=';
+  }
+  return false;
+}
+
+static GrowableArray<const char*>* startup_recording_options_array = NULL;
+static const char XXStartFlightRecordingOption[] = "-XX:StartFlightRecording";
+
+bool JfrOptionSet::parse_start_flight_recording_option(const JavaVMOption** option, char* delimiter) {
+  assert(option != NULL, "invariant");
+  assert(delimiter != NULL, "invariant");
+  assert((*option)->optionString != NULL, "invariant");
+  assert(strncmp((*option)->optionString, XXStartFlightRecordingOption, sizeof XXStartFlightRecordingOption - 1) == 0, "invariant");
+  const char* value = NULL;
+  if (*delimiter == '\0') {
+    // -XX:StartFlightRecording without any delimiter and values
+    // Add dummy value "dumponexit=false" so -XX:StartFlightRecording can be used without explicit values.
     // The existing option->optionString points to stack memory so no need to deallocate.
     const_cast<JavaVMOption*>(*option)->optionString = (char*)"-XX:StartFlightRecording=dumponexit=false";
-    param_string = (*option)->optionString + 25;
+    value = (*option)->optionString + sizeof XXStartFlightRecordingOption;
   } else {
-    *tail = '='; // ":" -> "="
-    param_string = tail + 1;
+    // -XX:StartFlightRecording[=|:]
+    // set delimiter to '='
+    *delimiter = '=';
+    value = delimiter + 1;
   }
-  assert(param_string != NULL, "invariant");
-  const size_t param_length = strlen(param_string);
+  assert(value != NULL, "invariant");
+  const size_t value_length = strlen(value);
 
-  if (startup_recording_array == NULL) {
-    startup_recording_array = new (ResourceObj::C_HEAP, mtTracing) GrowableArray<const char*>(8, true, mtTracing);
+  if (startup_recording_options_array == NULL) {
+    startup_recording_options_array = new (ResourceObj::C_HEAP, mtTracing) GrowableArray<const char*>(8, true, mtTracing);
   }
-  assert(startup_recording_array != NULL, "invariant");
-  char* startup_options = NEW_C_HEAP_ARRAY(char, param_length + 1, mtTracing);
-  strncpy(startup_options, param_string, strlen(param_string) + 1);
-  assert(strncmp(param_string, startup_options, param_length) == 0, "invariant");
-  startup_recording_array->append(startup_options);
+  assert(startup_recording_options_array != NULL, "invariant");
+  char* const startup_value = NEW_C_HEAP_ARRAY(char, value_length + 1, mtTracing);
+  strncpy(startup_value, value, value_length + 1);
+  assert(strncmp(startup_value, value, value_length) == 0, "invariant");
+  startup_recording_options_array->append(startup_value);
   return false;
 }
 
-const GrowableArray<const char*>* JfrOptionSet::startup_recordings() {
-  return startup_recording_array;
+const GrowableArray<const char*>* JfrOptionSet::startup_recording_options() {
+  return startup_recording_options_array;
 }
 
-void JfrOptionSet::release_startup_recordings() {
-  if (startup_recording_array != NULL) {
-    for (int i = 0; i < startup_recording_array->length(); ++i) {
-      FREE_C_HEAP_ARRAY(char, startup_recording_array->at(i));
+void JfrOptionSet::release_startup_recording_options() {
+  if (startup_recording_options_array != NULL) {
+    const int length = startup_recording_options_array->length();
+    for (int i = 0; i < length; ++i) {
+      FREE_C_HEAP_ARRAY(char, startup_recording_options_array->at(i));
     }
+    delete startup_recording_options_array;
+    startup_recording_options_array = NULL;
   }
-  delete startup_recording_array;
-  startup_recording_array = NULL;
 }
-
-bool JfrOptionSet::parse_flight_recorder_option(const JavaVMOption** option, char* tail) {
-  assert(option != NULL, "invariant");
-  assert(tail != NULL, "invariant");
-  assert((*option)->optionString != NULL, "invariant");
-  assert(strncmp((*option)->optionString, "-XX:FlightRecorderOptions", 25) == 0, "invariant");
-  if (tail != NULL) {
-    *tail = '='; // ":" -> "="
-  }
-  return false;
-}
-

src/hotspot/share/jfr/recorder/service/jfrOptionSet.hpp

@@ -78,11 +78,10 @@ class JfrOptionSet : public AllStatic {
   static bool sample_protection();
   DEBUG_ONLY(static void set_sample_protection(jboolean protection);)
 
-  static bool parse_start_flight_recording_option(const JavaVMOption** option, char* tail);
-  static bool parse_flight_recorder_option(const JavaVMOption** option, char* tail);
-
-  static const GrowableArray<const char*>* startup_recordings();
-  static void release_startup_recordings();
+  static bool parse_flight_recorder_option(const JavaVMOption** option, char* delimiter);
+  static bool parse_start_flight_recording_option(const JavaVMOption** option, char* delimiter);
+  static const GrowableArray<const char*>* startup_recording_options();
+  static void release_startup_recording_options();
 };
 
 #endif // SHARE_VM_JFR_RECORDER_SERVICE_JFROPTIONSET_HPP

test/jdk/jdk/jfr/jcmd/TestJcmdDumpLimited.java

@@ -48,7 +48,7 @@ import jdk.test.lib.process.OutputAnalyzer;
  * @summary The test verifies JFR.dump command
  * @key jfr
  * @library /test/lib /test/jdk
- * @run main/othervm -XX:FlightRecorderOptions jdk.jfr.jcmd.TestJcmdDumpLimited
+ * @run main/othervm jdk.jfr.jcmd.TestJcmdDumpLimited
  */
 public class TestJcmdDumpLimited {