8352284: EXTRA_CFLAGS incorrectly applied to BUILD_LIBJVM src/hotspot C++ source files

[cnqpzhang]

Building jdk with --with-extra-cflags='-Wno-incompatible-pointer-types' triggers 1000+ warning messages like cc1plus: warning: command-line option ‘-Wno-incompatible-pointer-types’ is valid for C/ObjC but not for C++.

The root cause is that JVM_CFLAGS which contains both EXTRA_CXXFLAGS and EXTRA_CFLAGS when compiling src/hotspot C++ source files and building BUILD_LIBJVM.

This PR does:

1. Not to append EXTRA_CFLAGS or EXTRA_CXXFLAGS into JVM_CFLAGS before calling SetupJdkLibrary, instead let SetupCompilerFlags accept and merge EXTRA_CFLAGS and EXTRA_CXXFLAGS passed from SetupJdkLibrary as parameters, so CPP compilation will only see EXTRA_CXXFLAGS as expected.
2. Correct PCH_COMMAND to use EXTRA_CXXFLAGS as precompiled.hpp.gch should not be compiled with EXTRA_CFLAGS.
3. Fixed STATIC_LIB_CFLAGS in Flags.gmk to -DSTATIC_BUILD=1, which was missed by cbab40bc for the refactor of building static libs.

Tests: Passed jdk building on an AArch64 Linux system and tier1 sanity tests, also passed OpenJDK GHA Sanity Checks.

---

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-8352284: EXTRA_CFLAGS incorrectly applied to BUILD_LIBJVM src/hotspot C++ source files (Enhancement - P4)

Reviewers

• Erik Joelsson (@erikj79 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24115

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

@cnqpzhang 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:

8352284: EXTRA_CFLAGS incorrectly applied to BUILD_LIBJVM src/hotspot C++ source files

Reviewed-by: erikj

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

• a83760a: 8352092: -XX:AOTMode=record crashes with InstanceKlass in allocated state
• 1077265: 8353321: [macos] ErrorTest.testAppContentWarning test case requires signing environment
• 52f56e6: 8353196: [macos] Contents of ".jpackage.xml" file are wrong when building .pkg from unsigned app image
• acd4da4: 8353299: VerifyJarEntryName.java test fails
• 5eee32d: 8352768: CDS test MethodHandleTest.java failed in -Xcomp mode
• 8b0602d: 8319447: Improve performance of delayed task handling
• fe8bd75: 8351290: Clarify integral only for vector operators
• 4d1de46: 8352185: Shenandoah: Invalid logic for remembered set verification
• 3e96f5c: 8351366: Remove the java.security.debug=scl option
• 4247744: 8351435: Change the default Console implementation back to the built-in one in java.base module
• ... and 179 more: https://git.openjdk.org/jdk/compare/b891bfa7e67c21478475642e2bfa2cdc65a3bffe...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@erikj79) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

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

• build

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

• 03: Full - Incremental (46c43921)
• 02: Full - Incremental (5440ddc5)
• 01: Full - Incremental (aa242dd5)
• 00: Full (cf61e94a)

[erikj79]

I think I understand what you are trying to achieve, but this isn't the right solution.

The EXTRA_[C|CXX]FLAGS variables are not meant to be parameters to SetupNativeCompilation. In flags.m4 you can see that the intent is to not even export EXTRA_[C|CXX]FLAGS from spec.gmk:

  # FIXME: For compatibility, export this as EXTRA_CFLAGS for now.
  EXTRA_CFLAGS="$MACHINE_FLAG $USER_CFLAGS"
  EXTRA_CXXFLAGS="$MACHINE_FLAG $USER_CXXFLAGS"
  EXTRA_LDFLAGS="$MACHINE_FLAG $USER_LDFLAGS"
  EXTRA_ASFLAGS="$USER_ASFLAGS"

We don't want library/executable specific makefiles to have to worry about the extra flags supplied by the user. Those should be handled in lower layers. Given that, I agree with the change in JvmFlags.gmk. We shouldn't add $(EXTRA_CFLAGS) there. If we wanted those added, it should be done in configure, where EXTRA_CXXFLAGS are added, but not doing that is your whole goal here. Wouldn't just the change in JvmFlags.gmk be enough to solve your issue?

[cnqpzhang]

The EXTRA_[C|CXX]FLAGS variables are not meant to be parameters to SetupNativeCompilation

Actually, the content of EXTRA_[C|CXX]FLAGS is hidden (and mixed together) inside the JVM_CFLAGS and get passed to as a parameter to SetupNativeCompilation eventually. The diff is whether we want to pass them implicitly (base) or explicitly (patch).
The call sequence is:

1. Set up JVM_CFLAGS and append EXTRA_CXXFLAGS (make/autoconf/flags-cflags.m4#893) ->
2. Update JVM_CFLAGS and append EXTRA_CFLAGS (make/hotspot/lib/JvmFlags.gmk#L89) ->
3. Build libjvm ->
4. SetupJdkLibrary (pass JVM_CFLAGS as a parameter called CFLAGS, make/hotspot/lib/CompileJvm.gmk#L172) ->
5. SetupJdkNativeCompilation (make/common/JdkNativeCompilation.gmk#L477) ->
6. SetupNativeCompilation (make/common/JdkNativeCompilation.gmk#L456) ->
7. SetupCompilerFlags (set CXXFLAGS to JVM_CFLAGS now, make/common/NativeCompilation.gmk#L159, make/common/native/Flags.gmk#L131); CreateCompiledNativeFile (BASE_CXXFLAGS contains JVM_CFLAGS now, make/common/NativeCompilation.gmk#L179) ->
8. Compile C++ with BASE_CXXFLAGS that contains JVM_CFLAGS (make/common/native/CompileFile.gmk#L162)

Wouldn't just the change in JvmFlags.gmk be enough to solve your issue?

No, it seems not doable.
As mentioned above in the step 1 and 2, JVM_CFLAGS contains both EXTRA_CXXFLAGS and EXTRA_CFLAGS at that time, when calling SetupNativeCompilation, there is no information left about which flags were from EXTRA_[C|CXX]FLAGS and we are unable to do filter-out for C vs C++ files using corresponding flags. So, it is a dilemma, if we want to keep the parameters list of SetupNativeCompilation clean (untouched) we would have EXTRA_[C|CXX]FLAGS mixed together for compiling C and C++ files. Any other idea here please?

[cnqpzhang]

Wouldn't just the change in JvmFlags.gmk be enough to solve your issue?

Forgot to mention, one of my initial try-outs (hacking) was: Remove $(EXTRA_CFLAGS) from JVM_CFLAGS, based on an assumption that all source files under $(TOPDIR)/src/hotspot are (according to hotspot/variant-server/libjvm/objs/BUILD_LIBJVM.d) and will be C++ files only which has $(EXTRA_CXXFLAGS) and does not need $(EXTRA_CFLAGS) . This practically solved the issue I observed, however I don't think it was an elegant solution. Is this more acceptable than adding parameters to SetupNativeCompilation (actually the call to SetupJdkLibrary)?

diff --git a/make/hotspot/lib/JvmFlags.gmk b/make/hotspot/lib/JvmFlags.gmk
index 97538da74c7..57b632ee532 100644
--- a/make/hotspot/lib/JvmFlags.gmk
+++ b/make/hotspot/lib/JvmFlags.gmk
@@ -91,7 +91,6 @@ JVM_CFLAGS += \
     $(JVM_CFLAGS_TARGET_DEFINES) \
     $(JVM_CFLAGS_FEATURES) \
     $(JVM_CFLAGS_INCLUDES) \
-    $(EXTRA_CFLAGS) \
     #

[erikj79]

Wouldn't just the change in JvmFlags.gmk be enough to solve your issue?

Forgot to mention, one of my initial try-outs (hacking) was: Remove $(EXTRA_CFLAGS) from JVM_CFLAGS, based on an assumption that all source files under $(TOPDIR)/src/hotspot are (according to hotspot/variant-server/libjvm/objs/BUILD_LIBJVM.d) and will be C++ files only which has $(EXTRA_CXXFLAGS) and does not need $(EXTRA_CFLAGS) . This practically solved the issue I observed, however I don't think it was an elegant solution. Is this more acceptable than adding parameters to SetupNativeCompilation (actually the call to SetupJdkLibrary)?

I can see what you mean. The current state of the build system is that we aren't consistent with the use of CFLAGS vs CXXFLAGS. For libraries that are C++ or use a mix of C and C++, SetupNativeLibrary will accept flags in two ways, either as just CFLAGS, which will then apply to both C and C++, or as separate CFLAGS and CXXFLAGS, which will then be treated separately. To properly solve your issue, we would need to stop this automatic fallback and be explicit with CFLAGS vs CXXFLAGS throughout the build system. That would certainly be cleaner.

Hotspot is currently only C++ and there is little reason to believe that would ever change. The build setup for hotspot is currently only setting CFLAGS, which will then get applied to CXXFLAGS as well. Given that we are currently going with that choice, I think the correct fix is indeed to just remove EXTRA_CFLAGS from JVM_CFLAGS. If we were to ever add C source files to hotspot, then we would need to start handling JVM_CFLAGS and JVM_CXXFLAGS separately, not just for the EXTRA flags, but for all of them.

[cnqpzhang]

Given that we are currently going with that choice, I think the correct fix is indeed to just remove EXTRA_CFLAGS from JVM_CFLAGS.

Thanks for your kindly advise @erikj79, I updated it accordingly.

[cnqpzhang]

Hi @magicus , do you think my above clarifications regarding the unrelated code change and added comments acceptable, or do you still recommend removing those sections before merging? Thank you for confirming, and any advice on further changes.

[magicus]

I still have reservations. Let me re-read the patch to see if I can better understand what part of the code you find hard to understand, so I can be more constructive in my criticism.

[magicus]

I've now re-read it all, and I stand behind my position that the comment should be removed.

I agree that the CFLAGS handling in the build system is obscure, and surprising. There is an old JBS issue from 2012 (!) tracking this: https://bugs.openjdk.org/browse/JDK-8001877 (see also https://bugs.openjdk.org/browse/JDK-8333089). It's a shame that this has not been fixed, but the problem is that the current situation is so complicated that it will take quite an effort to disentangle the mess, which combined with the limited ways to raise abstraction available in make makes this a much larger undertaking than maybe is apparent at first glance.

So in this context, having the proposed comment at that place only just adds to the confusion.

[cnqpzhang]

So in this context, having the proposed comment at that place only just adds to the confusion.

Sure, I removed the comments with a new commit.

[cnqpzhang]

/integrate

[openjdk]

@cnqpzhang
Your change (at version 46c4392) is now ready to be sponsored by a Committer.

[magicus]

Thank you!

/sponsor

[openjdk]

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

• cef5610: 8353272: One instance of STATIC_LIB_CFLAGS was missed in JDK-8345683
• 6801eb8: 8352709: Remove bad timing annotations from WhileOpTest.java
• 85a0baf: 8352719: Add an equals sign to the modules statement
• f25f701: 8353226: JFR: emit old object samples must be transitive closure complete for segment
• aff5aa7: 8350566: NMT: add size parameter to MemTracker::record_virtual_memory_tag
• 196334f: 8352046: Test testEcoFriendly() in jdk tools launcher ExecutionEnvironment.java for AIX and Linux/musl is brittle
• ad48846: 8350386: Test TestCodeCacheFull.java fails with option -XX:-UseCodeCacheFlushing
• 8b4e190: 8353349: ProblemList runtime/cds/appcds/SignedJar.java
• 860a789: 8353219: RISC-V: Fix client builds after JDK-8345298
• a83760a: 8352092: -XX:AOTMode=record crashes with InstanceKlass in allocated state
• ... and 188 more: https://git.openjdk.org/jdk/compare/b891bfa7e67c21478475642e2bfa2cdc65a3bffe...master

Your commit was automatically rebased without conflicts.

[openjdk]

@magicus @cnqpzhang Pushed as commit 1809138.

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