On Tue, 2 May 2023 11:35:54 GMT, Tobias Holenstein <tholenst...@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
