Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8209611: use C++ compiler for hotspot tests #634

Closed
wants to merge 1 commit into from

Conversation

GoeLin
Copy link
Member

@GoeLin GoeLin commented Nov 17, 2021

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/634/head:pull/634
$ git checkout pull/634

Update a local copy of the PR:
$ git checkout pull/634
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/634/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 634

View PR using the GUI difftool:
$ git pr show -t 634

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/634.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 17, 2021

👋 Welcome back goetz! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title Backport 8c47dc4a946723de202c943bc577765cefa8d24a 8209611: use C++ compiler for hotspot tests Nov 17, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 17, 2021

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr labels Nov 17, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 17, 2021

Webrevs

@GoeLin
Copy link
Member Author

@GoeLin GoeLin commented Nov 17, 2021

I need to backport '8226943: compile error in libfollowref003.cpp with XCode 10.2 on macosx" to fix the build error in the CI. See #636

@openjdk
Copy link

@openjdk openjdk bot commented Nov 18, 2021

@GoeLin This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8209611: use C++ compiler for hotspot tests

Reviewed-by: mbaesken, stuefe

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 5 new commits pushed to the master branch:

  • ed9a41b: 8276536: Update TimeZoneNames files to follow the changes made by JDK-8275766
  • ccd7a6f: 8275766: (tz) Update Timezone Data to 2021e
  • d9b2c4c: 8256956: RegisterImpl::max_slots_per_register is incorrect on AMD64
  • 1b4eb43: 8276157: C2: Compiler stack overflow during escape analysis on Linux x86_32
  • 0d0aa7c: 8263059: security/infra/java/security/cert/CertPathValidator/certification/ComodoCA.java fails due to revoked cert

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Nov 18, 2021
@tstuefe
Copy link
Member

@tstuefe tstuefe commented Nov 18, 2021

Wow. The original bug must be one of the most taciturn JBS issues I have ever seen, especially considering the patch size. How is one supposed to seriously review this?

@GoeLin
Copy link
Member Author

@GoeLin GoeLin commented Nov 18, 2021

To verify what I did, I dumped the original and my change, and diffed the changes.
I did it twice, once so that moved files are recognized, and once without. This can be achieved by setting git config merge.renamelimit .
Luckily the change is from 12, so not that far away from 11...
Thanks for reviewing, Matthias!

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Nov 18, 2021

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.

@GoeLin
Copy link
Member Author

@GoeLin GoeLin commented Nov 20, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Nov 20, 2021

Going to push as commit 4b591db.
Since your change was applied there have been 7 commits pushed to the master branch:

  • 5e08548: 8274860: gcc 10.2.1 produces an uninitialized warning in sharedRuntimeTrig.cpp
  • 9b94409: 8277029: JMM GetDiagnosticXXXInfo APIs should verify output array sizes
  • ed9a41b: 8276536: Update TimeZoneNames files to follow the changes made by JDK-8275766
  • ccd7a6f: 8275766: (tz) Update Timezone Data to 2021e
  • d9b2c4c: 8256956: RegisterImpl::max_slots_per_register is incorrect on AMD64
  • 1b4eb43: 8276157: C2: Compiler stack overflow during escape analysis on Linux x86_32
  • 0d0aa7c: 8263059: security/infra/java/security/cert/CertPathValidator/certification/ComodoCA.java fails due to revoked cert

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 20, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 20, 2021

@GoeLin Pushed as commit 4b591db.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@GoeLin GoeLin deleted the goetz_backport_8209611 branch Nov 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated
3 participants