8311064: Windows builds fail without precompiled headers after JDK-8310728

[djelinski]

Please review this change to fix Windows build without precompiled headers.

After JDK-8310728:

the compiler enforces the C++11 requirement that all functions declared inline must have a definition available in the same translation unit if they're used

When precompiled headers are used, more headers are automatically included, which allowed the build to pass without this change.

This PR adds 2 includes and un-inlines PlatformMutex destructor, which was needed by 3 files and not easily exposed.

Windows build passes with this change, both with and without precompiled headers. Full tier1-5 build in progress.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8311064: Windows builds fail without precompiled headers after JDK-8310728 (Bug - P3)

Reviewers

• David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14701/head:pull/14701
$ git checkout pull/14701

Update a local copy of the PR:
$ git checkout pull/14701
$ git pull https://git.openjdk.org/jdk.git pull/14701/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14701

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14701.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back djelinski! 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]

@djelinski The following label will be automatically applied to this pull request:

• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (1b23cbc8)

[dholmes-ora]

Why couldn't you include os_windows.inline.hpp where the missing decl was needed?

[djelinski]

Mostly because the same destructor is not inline in os_posix.cpp.
I could include "runtime/os.inline.hpp" in xPhysicalMemory.cpp, xVirtualMemory.cpp and zVirtualMemory.cpp, but that would add weight to non-Windows builds for no good reason.

[dholmes-ora]

Okay. The changes seem harmless in that regard. We should have more consistency with the os specific inline definitions.

Thanks.

[openjdk]

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

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

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

8311064: Windows builds fail without precompiled headers after JDK-8310728

Reviewed-by: dholmes

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 23 new commits pushed to the master branch:

• cf8d706: 8308463: Refactor regenerated class handling in lambdaFormInvokers.cpp
• 6f58ab2: 8301569: jmod list option and jimage list --help not interpreted correctly on turkish locale
• 8f5a384: 8311032: Empty value for java.protocol.handler.pkgs system property can lead to unnecessary classloading attempts of protocol handlers
• ded1370: 8309811: BytecodePrinter cannot handle unlinked classes
• 02b17d7: 8310264: In PhaseChaitin::Split defs and phis are leaked
• a63afa4: 8294427: Check boxes and radio buttons have rendering issues on Windows in High DPI env
• 3df36c4: 8310061: Note if implicit annotation processing is being used
• da0f832: 8310606: Fix signed integer overflow, part 3
• f0c2f09: 8296972: [macos13] java/awt/Frame/MaximizedToIconified/MaximizedToIconified.java: getExtendedState() != 6 as expected.
• 9f46fc2: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
• ... and 13 more: https://git.openjdk.org/jdk/compare/48e61c1df53c11ed49603abd70a3dd62a25f7be5...master

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.

[djelinski]

/integrate

[openjdk]

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

• cbf418a: 8311020: Typo cleanup in Classfile API
• f4b900b: 8310902: (fc) FileChannel.transferXXX async close and interrupt issues
• cf8d706: 8308463: Refactor regenerated class handling in lambdaFormInvokers.cpp
• 6f58ab2: 8301569: jmod list option and jimage list --help not interpreted correctly on turkish locale
• 8f5a384: 8311032: Empty value for java.protocol.handler.pkgs system property can lead to unnecessary classloading attempts of protocol handlers
• ded1370: 8309811: BytecodePrinter cannot handle unlinked classes
• 02b17d7: 8310264: In PhaseChaitin::Split defs and phis are leaked
• a63afa4: 8294427: Check boxes and radio buttons have rendering issues on Windows in High DPI env
• 3df36c4: 8310061: Note if implicit annotation processing is being used
• da0f832: 8310606: Fix signed integer overflow, part 3
• ... and 15 more: https://git.openjdk.org/jdk/compare/48e61c1df53c11ed49603abd70a3dd62a25f7be5...master

Your commit was automatically rebased without conflicts.

[openjdk]

@djelinski Pushed as commit af319d9.

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

[stefank]

@djelinski @dholmes-ora It's not obvious to me that this was the right fix for ZGC. Could you explain where in zPhysicalMemory.cpp we uses a ZListNode? If it is not and it is instead used in one of the included files the added include line should go there instead.

I also wonder where in ZVirtualMemory_windows.cpp we use zVirtualMemory.inline.hpp? The error message states that we miss ZMemory::end etc. So, it seems like the appropriate fix would have been to include zMemory.inline.hpp instead.

[dholmes-ora]

@djelinski hotspot changes require two reviews before integration.

@stefank I took it on faith that the ZGC changes were appropriate, or that a ZGC reviewer would report otherwise. If a hpp file is trying to use an inline function that is a problem in itself!

[dholmes-ora]

This error:

zPhysicalMemory.obj : error LNK2001: unresolved external symbol "public: __cdecl ZListNode<class ZMemory>::~ZListNode<class ZMemory>(void)" (??1?$[ZListNode@VZMemory](mailto:ZListNode@VZMemory)@@@@[QEAA@XZ](mailto:QEAA@XZ))

certainly suggests to me that zPhysicalMemory.cpp needs the ZListNode header file.

[stefank]

If a hpp file is trying to use an inline function that is a problem in itself!

I didn't say that it was used in an hpp file. I said "included files" and implicitly meant an .inline.hpp file.

[stefank]

certainly suggests to me that zPhysicalMemory.cpp needs the ZListNode header file.

How do you know that it isn't one of the include .inline.hpp files that needs the ZListNode destructor?

[djelinski]

Apologies for integrating this early, I wanted to get the CI off my back as quickly as possible, and the changes seemed innocuous enough.

The compiler/linker output does not make it easy to figure out where the offending ZListNode destructor was called. I would guess that it could be called from the implicit ZMemory destructor, will try to confirm that.

Good point about including zMemory instead of zVirtualMemory, I'll look into that.

On a side note, I assume that the split into hpp and inline.hpp was done to speed up compilation; has anyone checked recently if the split still makes sense? If not, I could do some experiments here.

[stefank]

Apologies for integrating this early, I wanted to get the CI off my back as quickly as possible, and the changes seemed innocuous enough.

Understood. Thanks. We will take care of updating the ZGC code.

The compiler/linker output does not make it easy to figure out where the offending ZListNode destructor was called. I would guess that it could be called from the implicit ZMemory destructor, will try to confirm that.

That's exactly what happens. There is an implicit usage of the inline function from our hpp files. This propagates from ZList -> ZMemory -> ZMemoryManager -> ZPhysicalMemoryManager. This is problematic and we will think about what we think about a solution that fits the ZGC code.

Good point about including zMemory instead of zVirtualMemory, I'll look into that.

I tested it and it works.

On a side note, I assume that the split into hpp and inline.hpp was done to speed up compilation; has anyone checked recently if the split still makes sense? If not, I could do some experiments here.

The compile times are one reason. Another is to minimize issues with circular dependencies.

[djelinski]

Thanks. In case that helps, I verified that creating explicit inline destructors for ZMemory and ZMemoryManager in zMemory.inline.hpp also fixes the zPhysicalMemory linking problem.

[dholmes-ora]

certainly suggests to me that zPhysicalMemory.cpp needs the ZListNode header file.

How do you know that it isn't one of the include .inline.hpp files that needs the ZListNode destructor?

I don't know for certain but in the context of reviewing these changes the code change was not obviously incorrect in relation to the error message.

[stefank]

certainly suggests to me that zPhysicalMemory.cpp needs the ZListNode header file.

How do you know that it isn't one of the include .inline.hpp files that needs the ZListNode destructor?

I don't know for certain but in the context of reviewing these changes the code change was not obviously incorrect in relation to the error message.

Right, the change works, but it adds an include in a file that doesn't explicitly use that included functions. That shows that there is an issues somewhere else that we will likely show up to be a problem in the future. We've now identified that the issues is that zMemory.hpp has a dependency against the destructor in zList.inline.hpp. We'll deal with that a new RFE.