On Tue, 17 Dec 2024 14:51:16 GMT, Julian Waters <jwat...@openjdk.org> wrote:
>> [JDK-8343698](https://bugs.openjdk.org/browse/JDK-8343698) fixed LTO for gcc >> when compiling for platforms where the FORBID_C_FUNCTION mechanism is >> active, however the fix does so by inhibiting LTO for a specific file. This >> can hinder optimization, which is the end goal if one is indeed doing an LTO >> build. Fix the issue in a different way by disabling FORBID_C_FUNCTION >> entirely for os.cpp, which is where the error originates. This has a wide >> downstream effect, as os.cpp contains a call to os::malloc which contains >> the forbidden malloc that causes errors that cannot be suppressed by >> ALLOW_C_FUNCTION in an LTO build. This is a stopgap fix until >> FORBID_C_FUNCTION is fixed to work properly with LTO on all platforms. While >> here, also fix a memory leak in jvmciEnv.cpp > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 12 additional > commits since the last revision: > > - Merge branch 'openjdk:master' into patch-16 > - -fno-omit-frame-pointer in JvmFeatures.gmk > - Revert compilerWarnings_gcc.hpp > - General LTO fixes JvmFeatures.gmk > - Revert DISABLE_POISONING_STOPGAP compilerWarnings_gcc.hpp > - Merge branch 'openjdk:master' into patch-16 > - Revert os.cpp > - Fix memory leak in jvmciEnv.cpp > - Stopgap fix in os.cpp > - Declaration fix in compilerWarnings_gcc.hpp > - ... and 2 more: https://git.openjdk.org/jdk/compare/00943376...9d05cb8e I only noticed this had been changed back to Draft after I was mostly done looking at it. But I don't think this should be done this way, esp. since it didn't seem to work (as in suppressing warnings from LTO) for me. src/hotspot/share/jvmci/jvmciEnv.cpp line 616: > 614: // The memory allocated in libjvmci was not allocated with os::malloc > 615: // so must not be freed with os::free. > 616: ALLOW_C_FUNCTION(::free, ::free((void*) _init_error_msg);) Please do this bug fix change under a separate JBS issue & PR. I've created a JBS issue for it: https://bugs.openjdk.org/browse/JDK-8345267 Fix memory leak in JVMCIEnv dtor src/hotspot/share/runtime/os.cpp line 26: > 24: > 25: // Stopgap fix until FORBID_C_FUNCTION can work properly with LTO > 26: #define DISABLE_POISONING_STOPGAP This isn't needed if not using LTO. src/hotspot/share/runtime/os.cpp line 26: > 24: > 25: // Stopgap fix until FORBID_C_FUNCTION can work properly with LTO > 26: #define DISABLE_POISONING_STOPGAP So far as I can tell, this doesn't work. I still get tons of `-Wattribute-warning`s when building with LTO, because of similar problem from other files. src/hotspot/share/runtime/os.cpp line 26: > 24: > 25: // Stopgap fix until FORBID_C_FUNCTION can work properly with LTO > 26: #define DISABLE_POISONING_STOPGAP This prevents precompiled headers from being used for this file. `-Winvalid-pch` will warn if enabled. src/hotspot/share/utilities/compilerWarnings_gcc.hpp line 89: > 87: // forbidden warnings in such builds. > 88: #define FORBID_C_FUNCTION(signature, alternative) \ > 89: [[gnu::warning(alternative)]] signature noexcept; Why are you making this change at all, let alone under this JBS issue? Among other problems, `noexcept` is mostly irrelevant in HotSpot, since we build with exceptions disabled. (There are a few places where `noexcept` affects semantics, like for `operator new`, but otherwise there is no point.) ------------- Changes requested by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22464#pullrequestreview-2470757627 PR Review Comment: https://git.openjdk.org/jdk/pull/22464#discussion_r1864123170 PR Review Comment: https://git.openjdk.org/jdk/pull/22464#discussion_r1864170998 PR Review Comment: https://git.openjdk.org/jdk/pull/22464#discussion_r1864210214 PR Review Comment: https://git.openjdk.org/jdk/pull/22464#discussion_r1864215385 PR Review Comment: https://git.openjdk.org/jdk/pull/22464#discussion_r1864125153
