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

[Vladimir Kozlov]

Looks good to me.

thanks,
Vladimir

On 10/29/18 1:24 PM, Ekaterina Pavlova wrote:
> Vladimir,
> I added "-XX:+UseAOTStrictLoading" to "java -version", see make/RunTests.gmk.
> Also added "--compile-with-assertion" to jaotc flags in case "-ea" flag was present in vm options
> (otherwise AOTed library will warn that it does not have java assertions in compiled code).
> 
> I will add -XX:+UseAOTStrictLoading and "-Xlog:aot+class+fingerprint=trace -Xlog:aot+class+load=trace"
> directly to AOT testing tasks in mach5 jobs.
> 
> Magnus,
> I changed
>   "$$(addprefix -vmoption:, $$($1_AOT_OPTIONS))"
> to
> +  ifneq ($$($1_AOT_OPTIONS), )
> +    $1_JTREG_BASIC_OPTIONS += -vmoptions:"$$($1_AOT_OPTIONS)"
> +  endif
> 
> Please review updated webrev:
>   http://cr.openjdk.java.net/~epavlova//8152988/webrev.01/
> 
> thanks,
> -katya
> 
> 
> On 10/23/18 4:40 PM, Vladimir Kozlov wrote:
>> On 10/22/18 9:36 AM, Ekaterina Pavlova wrote:
>>> Vladimir,
>>>
>>> thanks a lot for the review.
>>> "-XX:+PrintAOT" was only passed to "java -version" which is done before the testing to verify that AOTed libraries work.
>>
>> If it is only for -version then using  -XX:+PrintAOT is okay.
>>
>>> It also perhaps could be useful debug info in case some tests starts fail.
>>> But I can change -XX:+PrintAOT to -XX:+UseAOTStrictLoading if you think it is more than enough.
>>
>> -XX:+UseAOTStrictLoading is more important for tests which have different flags specified and may invalidate 
>> AOTLibrary configuration. We would want to catch tests which will not use AOT libraries. At least when we test these 
>> your changes.
>>
>>>
>>> Do you suggest to add "-Xlog:aot+class+fingerprint=trace -Xlog:aot+class+load=trace" to "java -version" only
>>> or to java flags used during tests execution as well?
>>
>> These options are next step after -XX:+UseAOTStrictLoading. We were asked before how we can guarantee that AOTed 
>> methods are used by tests. One way is PrintAOT which should show published AOT methods. An other way is Xlogs flags 
>> which have less output but provide at least some level of information that AOTed classes are used and not skipped.
>>
>> Thanks,
>> Vladimir
>>
>>>
>>>
>>> -katya
>>>
>>> On 10/19/18 10:40 AM, Vladimir Kozlov wrote:
>>>> 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*).
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>