RFR(M) 8152988: [AOT] Update test batch definitions to include aot-ed java.base module mode into hs-comp testing

[Vladimir Kozlov]

I share concern about output of -XX:+PrintAOT - it prints all AOT classes and methods. For AOTed java.base the output 
will be almost the same for all tests (depends which are java.base classes are used).

I would suggest instead to use -XX:+UseAOTStrictLoading which exit(1) VM if it can't load AOT library for some reason 
(mismatched configuration - different GCs).

Also -Xlog:aot+class+fingerprint=trace  to trace cases of mismatching class fingerprint.
Also -Xlog:aot+class+load=trace -to trace AOT class loading or skipping them if they are not matching.

And I agree to redirect this into file.

Thanks,
Vladimir

On 10/19/18 9:04 AM, Erik Joelsson wrote:
> Hello,
> 
> I will answer the parts I'm responsible for.
> 
> On 2018-10-19 00:21, Magnus Ihse Bursie wrote:
>>
>> In RunTests.gmk, the following line is repeated:
>> # Note, this could not be done during JDK build time.
>>
>> In the options:
>> $1_JAOTC_OPTS := \
>> -J-Xmx4g --info \
>> -Xmx4g is huge, much larger than what we typically use. Is this intentional? This will risk crashing at machines with 
>> too little free memory. Will AOT fail to work with less memory?
>>
>>
>> There's an extra space before the comma here:
>> $$(addprefix --compile-commands , $$($1_AOT_CCLIST)) \
>> Question (to Erik, I presume): the idea of addprefix here is that AOT_CCLIST can be empty, and this was a convenient 
>> way to just add "--compile-commands <the list file>" if it exists? I was a bit confounded at first since normally we 
>> only use it for distributing a prefix over multiple items, and I could see no way for AOT_CCLIST to ever be more than 
>> one item.
>>
> Yes, that was the intention. I often use addprefix that way and find it convenient that it seamlessly handles 0-N number 
> of elements in the argument without the need for cumbersome conditionals. The extra space is needed in this case but 
> should perhaps be explicit using $(SPACE).
>>
>> About this:
>> # Note, ExecuteWithLog doesn't play well with with two calls in the same recipe.
>> $$(info $$(JDK_IMAGE_DIR)/bin/java $$($1_JVM_OPTIONS) -XX:+PrintAOT -XX:AOTLibrary=$$@ -version)
>> $$(FIXPATH) $$(JDK_IMAGE_DIR)/bin/java \
>> $$($1_JVM_OPTIONS) \
>> -XX:+PrintAOT -XX:AOTLibrary=$$@ -version
>> First, what is the purpose of this? Is it to verify that the generated AOT library works? This will result in a lot of 
>> "java -version" being printed at the time when running tests. Perhaps the output should be redirected? (Assuming that 
>> java exists with a non-zero exit value if things fail, but if it does not, I'm not sure how this would help otherwise.)
>>
> My guess is that they want diagnostics saved in case tests fail.
>> Second, there's no problem to use multiple ExecuteWithLog in the same recipe, however, the base variable needs to be 
>> different. E.g:
>> $$(call ExecuteWithLog, $$@_verify, \
>> $$(FIXPATH) $$(JDK_IMAGE_DIR)/bin/java \
>> 	    ...
>>
> The issue I'm referring to is that the $(info) line in ExecuteWithLog will print the commands first, then execute the 
> first line of the recipe. This makes the output confusing since both of those commands print (what I'm assuming is) 
> relevant diagnostics information. This is simply a consequence of using $(info) for printing the command line. Make will 
> evaluate all the recipe lines first, then execute them one by one. To be clear, it ends up like this:
> 
> Executing [jaotc ...]
> Executing [java ...]
> <output from jaotc>
> <output from java>
>>
>> Further,
>> SetupAot = $(NamedParamsMacroTemplate)
>> define SetupAotBody
>> ifneq ($$($1_MODULES), )
>> ...
>> If think it would be clearer if the if-test is on the call site for SetupAot instead. Now it's unconditionally called, 
>> but does nothing if AOT is not used. I think that looks odd.
>>
> My goal was to minimize the repeated code at each call site. While developing this, I had a lot more duplication at 
> first, and it was a hassle to update each location every time I needed to change something. Other than that I don't feel 
> strongly for either construct.
>>
>> And here,
>> - run-test-$1: clean-workdir-$1 $(TEST_PREREQS)
>> + run-test-$1: clean-workdir-$1 $$($1_AOT_TARGETS)
>> I don't think you can just remove TEST_PREREQS like that..? (The same goes for the other run-test-$1 instance.)
>>
> I added the TEST_PREREQS variable to support the CDS archive generation, which was recently removed. With the way 
> SetupAot turned out to need separate calls from each SetupTestRun, the global TEST_PREREQS wasn't suitable. I don't know 
> of any current need for a general TEST_PREREQS.
>>
>> Finally:
>> + $$(addprefix -vmoption:, $$($1_AOT_OPTIONS)) \
>> I'm not really comfortable with this use of addprefix. I think a construct like:
>> ifneq ($$($1_AOT_OPTIONS), )
>>    $1_JTREG_BASIC_OPTIONS += -vmoption:$$($1_AOT_OPTIONS)
>> endif
>> would be much clearar. Ideally, I'd like to see the same construct in the SetupAotModule function as well, but that's 
>> not as important.
>>
> I agree.
> 
> /Erik
>>
>> The rest of the build changes look good. (I can't say anything about the test/**/*-list.txt files.)
>>
>> /Magnus
>>
>>> testing: Tested by running subset of hotspot, jdk and jck tests with TEST_OPTS_AOT_MODULES=java.base 
>>> jdk.internal.vm.compiler
>>>          with "make run-test" and in Mach5.
>>>
>>> thanks,
>>> -katya
>>>
>>> p.s.
>>>  Thanks a lot Erik for huge help with porting original patch done in old testing framework (test/TestCommon.gmk)
>>>  to new one (make/RunTests*).
>>>
>>>
>>
>