RFR: 8351958: Some compile commands should be made diagnostic [v2]

[Marc Chevalier]

On Fri, 16 May 2025 10:38:51 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Address comments
>
> 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.

Done (despite being out of the window so it won't show as outdated).

> 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

I think it's nicer indeed. I've kept it this way for consistency, but I don't mind making it nicer. Should I also fix the lines 29, 40 and 62 that have such a space?

> 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`

No indeed, that's a good point. Replaced.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25150#discussion_r2092922046
PR Review Comment: https://git.openjdk.org/jdk/pull/25150#discussion_r2092923286
PR Review Comment: https://git.openjdk.org/jdk/pull/25150#discussion_r2092920997