7178703: Fix handling of quoted arguments and better error messages in dcmd

Reviewed-by: coleenp, mgronlun, rbackman

hotspot/src/share/vm/prims/whitebox.cpp

@@ -113,6 +113,9 @@ const char* WhiteBox::lookup_jstring(const char* field_name, oop object) {
   int offset = offset_for_field(field_name, object,
       vmSymbols::string_signature());
   oop string = object->obj_field(offset);
+  if (string == NULL) {
+    return NULL;
+  }
   const char* ret = java_lang_String::as_utf8_string(string);
   return ret;
 }

hotspot/src/share/vm/services/diagnosticCommand.hpp

@@ -48,7 +48,7 @@ public:
            "With no argument this will show a list of available commands. "
            "'help all' will show help for all commands.";
   }
-  static const char* impact() { return "Low: "; }
+  static const char* impact() { return "Low"; }
   static int num_arguments();
   virtual void execute(TRAPS);
 };
@@ -60,7 +60,7 @@ public:
   static const char* description() {
     return "Print JVM version information.";
   }
-  static const char* impact() { return "Low: "; }
+  static const char* impact() { return "Low"; }
   static int num_arguments() { return 0; }
   virtual void execute(TRAPS);
 };
@@ -72,7 +72,7 @@ public:
   static const char* description() {
     return "Print the command line used to start this VM instance.";
   }
-  static const char* impact() { return "Low: "; }
+  static const char* impact() { return "Low"; }
   static int num_arguments() { return 0; }
   virtual void execute(TRAPS) {
     Arguments::print_on(_output);
@@ -88,7 +88,7 @@ public:
       return "Print system properties.";
     }
     static const char* impact() {
-      return "Low: ";
+      return "Low";
     }
     static int num_arguments() { return 0; }
     virtual void execute(TRAPS);
@@ -105,7 +105,7 @@ public:
     return "Print VM flag options and their current values.";
   }
   static const char* impact() {
-    return "Low: ";
+    return "Low";
   }
   static int num_arguments();
   virtual void execute(TRAPS);
@@ -121,7 +121,7 @@ public:
     return "Print VM uptime.";
   }
   static const char* impact() {
-    return "Low: ";
+    return "Low";
   }
   static int num_arguments();
   virtual void execute(TRAPS);

hotspot/src/share/vm/services/diagnosticFramework.cpp

@@ -75,11 +75,13 @@ bool DCmdArgIter::next(TRAPS) {
   }
   // extracting first item, argument or option name
   _key_addr = &_buffer[_cursor];
+  bool arg_had_quotes = false;
   while (_cursor <= _len - 1 && _buffer[_cursor] != '=' && _buffer[_cursor] != _delim) {
     // argument can be surrounded by single or double quotes
     if (_buffer[_cursor] == '\"' || _buffer[_cursor] == '\'') {
       _key_addr++;
       char quote = _buffer[_cursor];
+      arg_had_quotes = true;
       while (_cursor < _len - 1) {
         _cursor++;
         if (_buffer[_cursor] == quote && _buffer[_cursor - 1] != '\\') {
@@ -95,16 +97,22 @@ bool DCmdArgIter::next(TRAPS) {
     _cursor++;
   }
   _key_len = &_buffer[_cursor] - _key_addr;
+  if (arg_had_quotes) {
+    // if the argument was quoted, we need to step past the last quote here
+    _cursor++;
+  }
   // check if the argument has the <key>=<value> format
   if (_cursor <= _len -1 && _buffer[_cursor] == '=') {
     _cursor++;
     _value_addr = &_buffer[_cursor];
+    bool value_had_quotes = false;
     // extract the value
     while (_cursor <= _len - 1 && _buffer[_cursor] != _delim) {
       // value can be surrounded by simple or double quotes
       if (_buffer[_cursor] == '\"' || _buffer[_cursor] == '\'') {
         _value_addr++;
         char quote = _buffer[_cursor];
+        value_had_quotes = true;
         while (_cursor < _len - 1) {
           _cursor++;
           if (_buffer[_cursor] == quote && _buffer[_cursor - 1] != '\\') {
@@ -120,6 +128,10 @@ bool DCmdArgIter::next(TRAPS) {
       _cursor++;
     }
     _value_len = &_buffer[_cursor] - _value_addr;
+    if (value_had_quotes) {
+      // if the value was quoted, we need to step past the last quote here
+      _cursor++;
+    }
   } else {
     _value_addr = NULL;
     _value_len = 0;
@@ -185,8 +197,17 @@ void DCmdParser::parse(CmdLine* line, char delim, TRAPS) {
         arg->read_value(iter.key_addr(), iter.key_length(), CHECK);
         next_argument = next_argument->next();
       } else {
-        THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
-          "Unknown argument in diagnostic command");
+        const size_t buflen    = 120;
+        const size_t argbuflen = 30;
+        char buf[buflen];
+        char argbuf[argbuflen];
+        size_t len = MIN2<size_t>(iter.key_length(), argbuflen - 1);
+
+        strncpy(argbuf, iter.key_addr(), len);
+        argbuf[len] = '\0';
+        jio_snprintf(buf, buflen - 1, "Unknown argument '%s' in diagnostic command.", argbuf);
+
+        THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), buf);
       }
     }
     cont = iter.next(CHECK);
@@ -207,19 +228,21 @@ GenDCmdArgument* DCmdParser::lookup_dcmd_option(const char* name, size_t len) {
 }
 
 void DCmdParser::check(TRAPS) {
+  const size_t buflen = 256;
+  char buf[buflen];
   GenDCmdArgument* arg = _arguments_list;
   while (arg != NULL) {
     if (arg->is_mandatory() && !arg->has_value()) {
-      THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
-              "Missing argument for diagnostic command");
+      jio_snprintf(buf, buflen - 1, "The argument '%s' is mandatory.", arg->name());
+      THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), buf);
     }
     arg = arg->next();
   }
   arg = _options;
   while (arg != NULL) {
     if (arg->is_mandatory() && !arg->has_value()) {
-      THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
-              "Missing option for diagnostic command");
+      jio_snprintf(buf, buflen - 1, "The option '%s' is mandatory.", arg->name());
+      THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), buf);
     }
     arg = arg->next();
   }

hotspot/src/share/vm/services/diagnosticFramework.hpp

@@ -238,6 +238,16 @@ public:
   static const char* name() { return "No Name";}
   static const char* description() { return "No Help";}
   static const char* disabled_message() { return "Diagnostic command currently disabled"; }
+  // The impact() method returns a description of the intrusiveness of the diagnostic
+  // command on the Java Virtual Machine behavior. The rational for this method is that some
+  // diagnostic commands can seriously disrupt the behavior of the Java Virtual Machine
+  // (for instance a Thread Dump for an application with several tens of thousands of threads,
+  // or a Head Dump with a 40GB+ heap size) and other diagnostic commands have no serious
+  // impact on the JVM (for instance, getting the command line arguments or the JVM version).
+  // The recommended format for the description is <impact level>: [longer description],
+  // where the impact level is selected among this list: {Low, Medium, High}. The optional
+  // longer description can provide more specific details like the fact that Thread Dump
+  // impact depends on the heap size.
   static const char* impact() { return "Low: No impact"; }
   static int num_arguments() { return 0; }
   outputStream* output() { return _output; }
@@ -250,7 +260,7 @@ public:
     bool has_arg = iter.next(CHECK);
     if (has_arg) {
       THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
-                "Unknown argument in diagnostic command");
+                "The argument list of this diagnostic command should be empty.");
     }
   }
   virtual void execute(TRAPS) { }

hotspot/test/serviceability/ParserTest.java

@@ -20,6 +20,7 @@ public class ParserTest {
         testNanoTime();
         testJLong();
         testBool();
+        testQuotes();
         testMemorySize();
     }
 
@@ -95,6 +96,33 @@ public class ParserTest {
         parse(name, "false", "", args);
     }
 
+    public void testQuotes() throws Exception {
+        String name = "name";
+        DiagnosticCommand arg1 = new DiagnosticCommand(name,
+                "desc", DiagnosticArgumentType.STRING,
+                false, null);
+        DiagnosticCommand arg2 = new DiagnosticCommand("arg",
+                "desc", DiagnosticArgumentType.STRING,
+                false, null);
+        DiagnosticCommand[] args = {arg1, arg2};
+
+        // try with a quoted value
+        parse(name, "Recording 1", name + "=\"Recording 1\"", args);
+        // try with a quoted argument
+        parse(name, "myrec", "\"" + name + "\"" + "=myrec", args);
+        // try with both a quoted value and a quoted argument
+        parse(name, "Recording 1", "\"" + name + "\"" + "=\"Recording 1\"", args);
+
+        // now the same thing but with other arguments after
+
+        // try with a quoted value
+        parse(name, "Recording 1", name + "=\"Recording 1\",arg=value", args);
+        // try with a quoted argument
+        parse(name, "myrec", "\"" + name + "\"" + "=myrec,arg=value", args);
+        // try with both a quoted value and a quoted argument
+        parse(name, "Recording 1", "\"" + name + "\"" + "=\"Recording 1\",arg=value", args);
+    }
+
     public void testMemorySize() throws Exception {
         String name = "name";
         String defaultValue = "1024";