RFR: 8345265: Fix gcc LTO without disabling LTO for g1ParScanThreadState.cpp [v2]

[Kim Barrett]

On Tue, 17 Dec 2024 14:51:16 GMT, Julian Waters <jwaters at 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