On Mon, 24 Mar 2025 18:43:42 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
>> Patrick Zhang has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Fixed a typo >> >> Signed-off-by: Patrick Zhang <patr...@os.amperecomputing.com> >> - Added comments to describe why EXTRA_CFLAGS is excluded from JVM_CFLAGS >> >> Signed-off-by: Patrick Zhang <patr...@os.amperecomputing.com> >> - Revert the changes of adding new params to SetupNativeCompilation >> >> Signed-off-by: Patrick Zhang <patr...@os.amperecomputing.com> > > make/hotspot/lib/JvmFlags.gmk line 89: > >> 87: endif >> 88: >> 89: # Hotspot currently supports only C++. To prevent compilation conflicts, > > I don't think this comments serves any purpose. It was probably a mistake to > include EXTRA_CFLAGS here to begin with. After this PR, no one is going to > miss it, so the comment will make no sense. I was initially unfamiliar with the building process and puzzled by why `CFLAGS` unexpectedly encompassed both `EXTRA_CXXFLAGS` and `EXTRA_CFLAGS`, until delving into all details of `JVM_CFLAGS` at a couple of m4 and gmk files. `JVM_CFLAGS` is not set up at a single place, I think necessary comments are required to warn any similar intention to append C and Objective-C only options (https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html). FYI, my notes of where the `JVM_CFLAGS` got configured and used: 1). Initialization: https://github.com/openjdk/jdk/blob/b891bfa7e67c21478475642e2bfa2cdc65a3bffe/make/autoconf/flags-cflags.m4#L893 2). Updated: https://github.com/openjdk/jdk/blob/b891bfa7e67c21478475642e2bfa2cdc65a3bffe/make/hotspot/lib/JvmFlags.gmk#L89-L103 3). Passed as a parameter `CFLAGS` to `SetupJdkLibrary`, `SetupJdkNativeCompilation` , `SetupNativeCompilation` https://github.com/openjdk/jdk/blob/b891bfa7e67c21478475642e2bfa2cdc65a3bffe/make/hotspot/lib/CompileJvm.gmk#L181 4). Will take more steps for other potential changes till its arrival at the final combinations of flags: https://github.com/openjdk/jdk/blob/b891bfa7e67c21478475642e2bfa2cdc65a3bffe/make/common/native/CompileFile.gmk#L159 ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24115#discussion_r2011278491
