8351958: Some compile commands should be made diagnostic

[marc-chevalier]

Error when using a CompileCommand that is an alias for a diagnostic option when -XX:+UnlockDiagnosticVMOptions is not provided.

The argument processing works this way:

1. Flags are parsed, setting the value accordingly. For CompileCommand, each option is added to a \n-separated string. At this step, if a flag is diagnostic but -XX:+UnlockDiagnosticVMOptions is not provided, then an error message is emitted, argument parsing fails and the VM terminates. Yet, the value of CompileCommand is still an unparsed list of string.
2. Eventually, CompileCommand is parsed. For some of them, the value of regular flag is used as the default value, and as far as I know, it's the only mapping between CompileCommand and the equivalent flag. Moreover, at this point, the order of the various command line arguments is lost: it is not possible to know which CompileCommand comes before or after the -XX:+UnlockDiagnosticVMOptions.

Moreover, CompileCommand are parsed in the same way as compiler directives coming from a file. If we complain about diagnostic CompileCommand, we should also when coming from a directive file, for consistency. But then, while the relative order of CompileCommand and -XX:+UnlockDiagnosticVMOptions is lost, it simply makes no sense to compare the ordering of command line arguments and directives from a file.

So, before the difficulty and the relative lack of sense, I defaulted to ignore the ordering requirement. And by using the CompileCommand error reporting mechanism, we get an error that is consistent with other CompileCommand-parsing related errors, e.g.

CompileCommand: An error occurred during parsing
Error: VM option 'PrintAssembly' is diagnostic and must be enabled via -XX:+UnlockDiagnosticVMOptions.
Line: 'PrintAssembly,*::*'

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.

CompileCommand: compileonly Test.* bool compileonly = true
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

The other problem is how to identify that a CompileCommand is an alias for a diagnostic flag. I refrained from hardcoding the list, but there is no nice mapping stating "this is an alias for...", only the default value for some:

jdk/src/hotspot/share/compiler/compilerDirectives.hpp

Lines 37 to 60 in 9f9e73d

	#define compilerdirectives_common_other_flags(cflags) \
	cflags(Enable, bool, false, Unknown) \
	cflags(Exclude, bool, false, Unknown) \
	cflags(BreakAtExecute, bool, false, BreakAtExecute) \
	cflags(BreakAtCompile, bool, false, BreakAtCompile) \
	cflags(Log, bool, LogCompilation, Unknown) \
	cflags(MemLimit, intx, 0, MemLimit) \
	cflags(MemStat, uintx, 0, MemStat) \
	cflags(PrintAssembly, bool, PrintAssembly, PrintAssembly) \
	cflags(PrintCompilation, bool, PrintCompilation, PrintCompilation) \
	cflags(PrintInlining, bool, PrintInlining, PrintInlining) \
	cflags(PrintNMethods, bool, PrintNMethods, PrintNMethods) \
	cflags(BackgroundCompilation, bool, BackgroundCompilation, BackgroundCompilation) \
	cflags(ReplayInline, bool, false, ReplayInline) \
	cflags(DumpReplay, bool, false, DumpReplay) \
	cflags(DumpInline, bool, false, DumpInline) \
	cflags(CompilerDirectivesIgnoreCompileCommands, bool, CompilerDirectivesIgnoreCompileCommands, Unknown) \
	cflags(RepeatCompilation, intx, RepeatCompilation, RepeatCompilation)
	#define compilerdirectives_common_string_flags(cflags) \
	cflags(DisableIntrinsic, ccstrlist, DisableIntrinsic, DisableIntrinsic) \
	cflags(ControlIntrinsic, ccstrlist, ControlIntrinsic, ControlIntrinsic)
	#define compilerdirectives_common_flags(cflags) \
	compilerdirectives_common_other_flags(cflags) \
	compilerdirectives_common_string_flags(cflags)

But I couldn't recycle compilerdirectives_common_flags (or similar) to define this mapping because the alias is just the default value. It can also be a literal. While PrintAssembly is an alias for PrintAssembly, MemLimit is not an alias for 0, it's just the default value. The best I could find is to assume that if there is a flag, it bears the same name. The only thing that looks like an exception is Log and LogCompilation, but actually, the CompileCommand Log requires the LogCompilation flag (which is diagnostic) to be set. So it is not actually an alias.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8351958: Some compile commands should be made diagnostic (Enhancement - P4)

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer)
• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25150/head:pull/25150
$ git checkout pull/25150

Update a local copy of the PR:
$ git checkout pull/25150
$ git pull https://git.openjdk.org/jdk.git pull/25150/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25150

View PR using the GUI difftool:
$ git pr show -t 25150

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25150.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mchevalier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@marc-chevalier This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8351958: Some compile commands should be made diagnostic

Reviewed-by: thartmann, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 376 new commits pushed to the master branch:

• a919f6d: 8356372: JVMTI heap sampling not working properly with outside TLAB allocations
• 8184ce3: 8357514: Disable AOT caching for runtime stubs
• 061b5cc: 8347050: Console.readLine() drops '' when reading through JLine
• ... and 373 more: https://git.openjdk.org/jdk/compare/482538b100856afe2252395d47e576e6c6d885ce...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@marc-chevalier The following label will be automatically applied to this pull request:

• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 05: Full - Incremental (f28b2076)
• 04: Full - Incremental (ef620280)
• 03: Full - Incremental (159fa771)
• 02: Full - Incremental (670e4019)
• 01: Full - Incremental (f1837f63)
• 00: Full (8e6c9a98)

[TobiHartmann]

Thanks for the detailed description. The fix looks good to me. I just added a few minor suggestions for improvement.

[vnkozlov]

@marc-chevalier I know that you follow existing patter. But may be register_command() should return boolean result if it succeeded or not so you can check it instead of loading value from buffer.

scan_value()and scan_option_and_value() may need to do the same.

Note, print_parse_error() has assert to check that error message is not empty.

[marc-chevalier]

It's fun you suggest that: it was my first version, but it felt like it was a lot more diff than what my PR version has. Happy to use a returned boolean.
I've made sure to rename function with temporary name not to miss any call site where the returned bool would be ignored, and renamed back. I also removed returns at the end of function not to miss any path: reaching end of function without a return is a warning (becoming error) at compile-time.
I've also removed a bit of now-obviously dead code.

[vnkozlov]

Look nice.

[TobiHartmann]

That looks good to me too.

[marc-chevalier]

/integrate

Thanks @vnkozlov & @TobiHartmann!

[openjdk]

Going to push as commit e348aa0.
Since your change was applied there have been 376 commits pushed to the master branch:

• a919f6d: 8356372: JVMTI heap sampling not working properly with outside TLAB allocations
• 8184ce3: 8357514: Disable AOT caching for runtime stubs
• 061b5cc: 8347050: Console.readLine() drops '' when reading through JLine
• ... and 373 more: https://git.openjdk.org/jdk/compare/482538b100856afe2252395d47e576e6c6d885ce...master

Your commit was automatically rebased without conflicts.

[openjdk]

@marc-chevalier Pushed as commit e348aa0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.