RFR: JDK-8282797: CompileCommand parsing errors should exit VM

[Christian Hagedorn]

On Tue, 2 May 2023 11:35:54 GMT, Tobias Holenstein <tholenstein at openjdk.org> wrote:

> Currently, errors during compile command parsing just print an error but don't exit the VM. As a result, issues go unnoticed. 
> 
> With this PR the behavior is changed to exit the VM when an error occurs.
> 
> E.g. `java -XX:CompileCommand=compileonly,HashMap:: -version` will exit the VM after a parsing occurred.  
> 
> CompileCommand: An error occurred during parsing
> Error: Could not parse method pattern
> Line: 'compileonly,HashMap::'
> 
> Usage: '-XX:CompileCommand=<option>,<method pattern>' - to set boolean option to true
> Usage: '-XX:CompileCommand=<option>,<method pattern>,<value>'
> Use:   '-XX:CompileCommand=help' for more information and to list all option.
> 
> Error: Could not create the Java Virtual Machine.
> Error: A fatal exception has occurred. Program will exit.
> 
> 
> ### Updated Tests
> Updated all tests to now expect an error code `1` for wrong `CompileCommand`

That's a nice feature to avoid accidental compile command typos in tests!

I have some comments.

That's a nice feature to avoid accidental compile command typos in tests!

I have some comments.

src/hotspot/share/compiler/compilerOracle.cpp line 815:

> 813: bool CompilerOracle::parse_from_line(char* line) {
> 814:   if (line[0] == '\0') return true;
> 815:   if (line[0] == '#')  return true;

While at it, you could unify these.

src/hotspot/share/compiler/compilerOracle.cpp line 955:

> 953:   assert(has_command_file(), "command file must be specified");
> 954:   FILE* stream = os::fopen(cc_file(), "rt");
> 955:   if (stream == nullptr) return true;

Whilte at it, you could add braces:

Suggestion:

  if (stream == nullptr) {
    return true;
  }

src/hotspot/share/compiler/compilerOracle.cpp line 1062:

> 1060:       char* newName = NEW_RESOURCE_ARRAY( char, i + 1);
> 1061:       if (newName == nullptr)
> 1062:         return true;

Should be `false`.

You could also add braces here.

test/hotspot/jtreg/compiler/compilercontrol/share/IntrinsicCommand.java line 79:

> 77:                 md, type, intrinsic_ids);
> 78: 
> 79:         System.out.println("XXXXXXXXXXXXXXXX compileCommand.isValid " + compileCommand.isValid());

Leftover from debugging?

test/hotspot/jtreg/compiler/compilercontrol/share/scenario/Scenario.java line 127:

> 125:             Asserts.assertNE(mainOutput.getExitValue(), 0, "VM should exit with "
> 126:                     + "error for incorrect directives");
> 127:             //if (isJcmdValid) {

Intentionally commented out? Then it can be removed.

test/hotspot/jtreg/compiler/compilercontrol/share/scenario/Scenario.java line 129:

> 127:             //if (isJcmdValid) {
> 128:             boolean parse_error_found = false;
> 129:             for(OutputAnalyzer out : outputList) {

Suggestion:

            for (OutputAnalyzer out : outputList) {

-------------

Changes requested by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13753#pullrequestreview-1441216621
PR Review: https://git.openjdk.org/jdk/pull/13753#pullrequestreview-1441377274
PR Review Comment: https://git.openjdk.org/jdk/pull/13753#discussion_r1203672003
PR Review Comment: https://git.openjdk.org/jdk/pull/13753#discussion_r1203673827
PR Review Comment: https://git.openjdk.org/jdk/pull/13753#discussion_r1203676771
PR Review Comment: https://git.openjdk.org/jdk/pull/13753#discussion_r1203786219
PR Review Comment: https://git.openjdk.org/jdk/pull/13753#discussion_r1203788061
PR Review Comment: https://git.openjdk.org/jdk/pull/13753#discussion_r1203788762