RFR: 8351958: Some compile commands should be made diagnostic

[Tobias Hartmann]

On Fri, 9 May 2025 15:05:11 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

> 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:
> https://github.com/openjdk/jdk/blob/9f9e73d5f9fcb5e926a2674c54cbbc920...

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

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

> 341:     JVMFlag* flag = JVMFlag::find_declared_flag(name);
> 342:     if (flag != nullptr && flag->is_diagnostic()) {
> 343:       jio_snprintf(errorbuf, buf_size, "VM option '%s' is diagnostic and must be enabled via -XX:+UnlockDiagnosticVMOptions.", name);

Just noticed that code above in line 333 uses `warning(...)`. I think that code should also use `errorbuf` now that we have it.

test/hotspot/jtreg/runtime/CommandLine/VMOptionWarning.java line 48:

> 46: 
> 47: /* @test VMCompileCommandWarningDiagnostic
> 48:  * @bug 8027314

Suggestion:

 * @bug 8351958

test/hotspot/jtreg/runtime/CommandLine/VMOptionWarning.java line 51:

> 49:  * @summary Warn if compile command that is an alias for a diagnostic vm option is used and -XX:+UnlockDiagnosticVMOptions isn't specified.
> 50:  * @requires vm.flagless
> 51:  * @requires ! vm.debug

Suggestion:

 * @requires !vm.debug

test/hotspot/jtreg/runtime/CommandLine/VMOptionWarning.java line 97:

> 95:             }
> 96:             case "DiagnosticCompileCommand": {
> 97:                 pb = ProcessTools.createLimitedTestJavaProcessBuilder("-XX:CompileCommand=PrintAssembly,*::*", "-version");

We don't really need to print assembly here, right? It would just unnecessarily slow down test execution. I would suggest to use something like `-XX:CompileCommand=PrintAssembly,MyClass::myMethod`

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25150#pullrequestreview-2846246047
PR Review Comment: https://git.openjdk.org/jdk/pull/25150#discussion_r2092804150
PR Review Comment: https://git.openjdk.org/jdk/pull/25150#discussion_r2092807838
PR Review Comment: https://git.openjdk.org/jdk/pull/25150#discussion_r2092808174
PR Review Comment: https://git.openjdk.org/jdk/pull/25150#discussion_r2092810114