[jdk11u-dev] RFR: 8209611: use C++ compiler for hotspot tests

[Thomas Stuefe]

On Wed, 17 Nov 2021 08:09:41 GMT, Goetz Lindenmaier <goetz at openjdk.org> wrote:

> I want to downport this for parity with jdk11.0.x-oracle.
> 
> I had to resolve /make/common/TestFilesCompilation.gmk
> One chunk is pointless, as it was changed again by "8210920: Native C++ tests are not using CXXFLAGS"
> which, in head, came after this change, but was already downported to 11u.
> I resolved the other chunk that did not apply to the current syntax in head.
> 
> I had to conserve some changes we backported from the .cpp files
> to the .c files and move them over to the new .cpp files:
> 
> test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassMethods/getclmthd007/getclmthd007.c
> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/extension/EX03/ex03t001/ex03t001.c
> test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/extmech/extmech.c
> 
> In this file, we disable some warnings in 11 with "8232084: HotSpot build failed with GCC 9.2.1"
> I can not check whether the warnings are a problem in cpp, too, so
> I moved the change to the new file:
> test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.c

Marked as reviewed by stuefe (Reviewer).

Sorry, my comment was aimed more at the original JBS issue (I like to understand what I review). Meanwhile, I found Igors original review thread in build-dev from 2018, which offers more explanations than the JBS issue:

https://mail.openjdk.java.net/pipermail/build-dev/2018-August/022880.html

It seems that converting everything to C++ files was a workaround for shortcomings in the build system for tests, where .c files include other .c files, so all of them have to be either C or C++.

"Currently our build system supports only 1-1 relation b/w tests lib/exec and source file; most of native libraries in vmTestbase have more than one source file. to work that around, we introduced a .c file for each library and theses .c files include all other required .c files."

I wondered about the implications since C->C++ conversion has pitfalls (http://david.tribble.com/text/cdiffs.htm), and the patch size makes earnest reviews challenging.

After applying your patch, I noticed that there are 78 files left in test/hotspot/jtreg that remain C. None of these were removed for JDK12, so looks like they were not touched by the original patch either. I did not find an explanation for that, but I assume that's ok.

Note that one disadvantage of moving wholesale to C++ is longer build times. But I guess that ship is sailed. Since if oracle did backport this we should follow suit.

To review, I tried to clone the original jdk12u sources, but both mercurial repo and github readonly mirror are that slow that I gave up after an hour. I diffed the two diffs by hand and did not find anything overly suspicious. I also looked at your patch in general, the places you pointed out, and it seems fine. Due to the patch size this was a cursory look only. I guess if tests start failing, we will see follow up issues.

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

PR: https://git.openjdk.java.net/jdk11u-dev/pull/634