RFR: 8320750: Allow a testcase to run with multiple -Xlog [v2]

[Stefan Karlsson]

On Wed, 7 Feb 2024 20:37:15 GMT, Leo Korinth <lkorinth at openjdk.org> wrote:

>> Running a testcase with muliple -Xlog crashes JTREG test cases. This is because `Collector.toMap` is not given a merge strategy.
>> 
>> When the same argument is passed multiple times, I have added a merge strategy to use the latter value. This is similar to how it is implemented for `vm.opt.*` in JTREG.
>> 
>> If the flag tested is `-Xlog`, replace the value part with a dummy value "NONEMPTY_TEST_SENTINEL". This is because in the case of multiple `-Xlog` all values are used, and JTREG does not give a satisfactory way to represent them. This dummy value should make it hard to try to `@require` on specific values by mistake.
>> 
>> Tested with:
>> 
>>  @requires vm.opt.x.Xlog == "NONEMPTY_TEST_SENTINEL"
>>  @requires vm.opt.x.Xlog == "NONEMPTY_TEST_SENTINELXXX"
>>  @requires vm.opt.x.Xms == "3g"
>> 
>> and
>> 
>> JAVA_OPTIONS=-Xms3g -Xms4g
>> JAVA_OPTIONS=-Xms4g -Xms3g
>> JAVA_OPTIONS=-Xlog:gc* -Xlog:gc*
>> ``` 
>> 
>> Running tier1
>
> Leo Korinth has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Added test, reworked code structure after suggestions from Stefan and Johan

I find this version much easier to quickly read and understand. I've added a few suggestions that you consider, but only the comments around -Xbootclasspath/a is a blocker for integration.

test/jtreg-ext/requires/VMProps.java line 143:

> 141:         map.put("vm.flagless", this::isFlagless);
> 142:         map.put("jdk.foreign.linker", this::jdkForeignLinker);
> 143:         map.putAll(xFlags()); // -Xmx4g -> @requires vm.opt.x.Xmx == "4g" )

This has a slightly different style than the vmOptFinalFlags:

map.putAll(xFlags()); // -Xmx4g -> @requires vm.opt.x.Xmx == "4g" )

vs

vmOptFinalFlags(map);


It could be nice to unify the style so that we have:

vmOptXFlags(map);
vmOptFinalFlags(map);

test/jtreg-ext/requires/VMProps.java line 752:

> 750:      */
> 751:     private static boolean isXFlag(String flag) {
> 752:         return flag.startsWith("-X") && !flag.startsWith("-XX:") && !flag.equals("-X");

`-XX:` is not wrong, but I wonder if this could/should be `-XX` instead.

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

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16824#pullrequestreview-1869568027
PR Review Comment: https://git.openjdk.org/jdk/pull/16824#discussion_r1482622913
PR Review Comment: https://git.openjdk.org/jdk/pull/16824#discussion_r1482624108