8345265: Minor improvements for LTO across all compilers #22464

TheShermanTanker · 2024-11-30T00:35:59Z

This is a general cleanup and improvement of LTO, as well as a quick fix to remove a workaround in the Makefiles that disabled LTO for g1ParScanThreadState.cpp due to the old poisoning mechanism causing trouble. The -Wno-attribute-warning change here can be removed once Kim's new poisoning solution is integrated.

-fno-omit-frame-pointer is added to gcc to stop the linker from emitting code without the frame pointer
-flto is set to $(JOBS) instead of auto to better match what the user requested
-Gy is passed to the Microsoft compiler. This does not fully fix LTO under Microsoft, but prevents warnings about -LTCG:INCREMENTAL at least

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-8345265: Minor improvements for LTO across all compilers (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22464

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-30T00:36:58Z

👋 Welcome back jwaters! 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 · 2024-11-30T00:37:40Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-11-30T00:38:12Z

@TheShermanTanker The following labels will be automatically applied to this pull request:

build
hotspot

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

mlbridge · 2024-11-30T00:47:52Z

Webrevs

01: Full - Incremental (9d05cb8e)
00: Full (a357017f)

kimbarrett

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.

kimbarrett · 2024-11-30T06:47:02Z

src/hotspot/share/jvmci/jvmciEnv.cpp

@@ -613,7 +613,7 @@ JVMCIEnv::~JVMCIEnv() {
  if (_init_error_msg != nullptr) {
    // The memory allocated in libjvmci was not allocated with os::malloc
    // so must not be freed with os::free.
-    ALLOW_C_FUNCTION(::free((void*) _init_error_msg));
+    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

kimbarrett · 2024-11-30T06:49:49Z

src/hotspot/share/utilities/compilerWarnings_gcc.hpp

@@ -86,7 +86,7 @@
 // so uses of functions that are both forbidden and fortified won't cause
 // forbidden warnings in such builds.
 #define FORBID_C_FUNCTION(signature, alternative) \
-  extern "C" __attribute__((__warning__(alternative))) signature;
+  [[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.)

I was thinking about the extern "C" and I think it might not be needed. A method that was already declared extern "C" in a C library header will keep that linkage when it is declared again, even without extern "C". There's also the issue that this forbidding macro could declare methods that don't actually exist on the current platform, which I think(?) removing extern "C" helps prevent. There's also the strange case that not all platforms have C library methods that are extern "C" (Windows is a notable example), so this helps declaring things with conflicting linkages and causing an error. The noexcept was just to match the declarations in standard library headers, since they are supposed to be noexcept according to the Standard

kimbarrett · 2024-11-30T09:07:16Z

src/hotspot/share/runtime/os.cpp

@@ -22,6 +22,9 @@
 *
 */

+// Stopgap fix until FORBID_C_FUNCTION can work properly with LTO
+#define DISABLE_POISONING_STOPGAP


This isn't needed if not using LTO.

kimbarrett · 2024-11-30T10:31:26Z

src/hotspot/share/runtime/os.cpp

@@ -22,6 +22,9 @@
 *
 */

+// Stopgap fix until FORBID_C_FUNCTION can work properly with LTO
+#define DISABLE_POISONING_STOPGAP


So far as I can tell, this doesn't work. I still get tons of -Wattribute-warnings when building
with LTO, because of similar problem from other files.

kimbarrett · 2024-11-30T10:51:56Z

src/hotspot/share/runtime/os.cpp

@@ -22,6 +22,9 @@
 *
 */

+// Stopgap fix until FORBID_C_FUNCTION can work properly with LTO
+#define DISABLE_POISONING_STOPGAP


This prevents precompiled headers from being used for this file. -Winvalid-pch will warn if enabled.

TheShermanTanker · 2024-11-30T18:05:05Z

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.

Yeah, I had noticed that it didn't work too, which is why I returned it to draft. It also causes VC to explode when compiling the Windows HotSpot, so that isn't ideal. I guess returning g1ParScanThreadState.cpp to LTO status will have to wait until FORBID_C_FUNCTION is properly fixed up to be LTO proof

TheShermanTanker · 2024-12-08T05:07:58Z

This needs a name and description change, I'll do so later. @MBaesken does this fix LTO on your end? Kim also reports that LTO hangs indefinitely alongside several warning messages, do you have similar issues when you try to enable LTO?

MBaesken · 2024-12-09T10:30:01Z

This needs a name and description change, I'll do so later. @MBaesken does this fix LTO on your end? Kim also reports that LTO hangs indefinitely alongside several warning messages, do you have similar issues when you try to enable LTO?

When I try to build with this change (with and without lto enabled) I run into

/openjdk/tools/devkits/x86_64-linux-gnu-to-x86_64-linux-gnu-fedora27-gcc11.3.0/x86_64-linux-gnu/sysroot/usr/include/unistd.h:520:14: error: conflicting declaration of 'char* get_current_dir_name()' with 'C' linkage
  520 | extern char *get_current_dir_name (void) __THROW;
      |              ^~~~~~~~~~~~~~~~~~~~

(maybe it is related to the devkit, maybe to pch I don't know)

TheShermanTanker · 2024-12-09T11:36:50Z

This needs a name and description change, I'll do so later. @MBaesken does this fix LTO on your end? Kim also reports that LTO hangs indefinitely alongside several warning messages, do you have similar issues when you try to enable LTO?

When I try to build with this change (with and without lto enabled) I run into
/openjdk/tools/devkits/x86_64-linux-gnu-to-x86_64-linux-gnu-fedora27-gcc11.3.0/x86_64-linux-gnu/sysroot/usr/include/unistd.h:520:14: error: conflicting declaration of 'char* get_current_dir_name()' with 'C' linkage
  520 | extern char *get_current_dir_name (void) __THROW;
      |              ^~~~~~~~~~~~~~~~~~~~
(maybe it is related to the devkit, maybe to pch I don't know)

It's related to the subtle change in FORBID_C_FUNCTION, I think unistd.h is being included before compilerWarnings.hpp somewhere. Well, at least I now know the current approach has this issue

TheShermanTanker · 2024-12-17T14:51:15Z

Reverted the change in compilerWarnings_gcc.hpp since it was causing problems like Matthias reported. This still needs a title and description change since it now tries to improve LTO for all compilers, will do that later

TheShermanTanker · 2025-01-09T15:17:21Z

I believe this is now ready

MBaesken · 2025-01-09T16:06:17Z

Hi, the workaround 'disable lto in g1ParScanThreadState because of special inlining/flattening used there' is removed , why this works now ?

kimbarrett · 2025-01-10T03:33:04Z

Hi, the workaround 'disable lto in g1ParScanThreadState because of special inlining/flattening used there' is removed , why this works now ?

The issue there was the -Wattribute-warning warnings that were being generated. But this change is suppressing
those warnings in the LTO link:
https://github.com/openjdk/jdk/blame/9d05cb8eff344fea3c6b9a9686b728ec53963978/make/hotspot/lib/JvmFeatures.gmk#L176C11-L176C11
Note that won't work with the new portable forbidding mechanism based on deprecated attributes.

I'm trying this new version, and I still get a few other warnings and then seem to have a process hang in lto1-ltrans.
The switch from -flto=auto to -flto=$(JOBS) doesn't seem to have helped in that respect.

kimbarrett · 2025-01-10T10:42:50Z

I'm trying this new version, and I still get a few other warnings and then seem to have a process hang in lto1-ltrans. The switch from -flto=auto to -flto=$(JOBS) doesn't seem to have helped in that respect.

Turns out I didn't wait long enough. It does terminate after about 40 minutes, though not successfully. Instead the
build crashes with a failed assert:

#  Internal Error (../../src/hotspot/share/runtime/handles.inline.hpp:76), pid=4017588, tid=4017620
#  assert(_thread->is_in_live_stack((address)this)) failed: not on stack?

I've not tried to debug this. Maybe it's a consequence of one of those problems of bypassing an intentional implicit
noinline in our code (by ensuring a function definition is in a different TU from all callers), with LTO breaking that.
Or maybe LTO is breaking us in some other way (such as taking advantage of no-UB constraints that aren't found
by normal compilation).

magicus · 2025-01-10T13:09:21Z

Or maybe LTO is breaking us in some other way (such as taking advantage of no-UB constraints that aren't found
by normal compilation).

That seems definitely likely. With LTO the linker has a whole lot of room to try many exciting optimizations. I think it is more than likely that this can trigger some UB holes.

The question is: should we decouple "fixing LTO build" from "fixing a working LTO JVM"? I tend to think so; that the problem for the build system is to be able to build the product, and if it then crashes during use, it's the problem of the component owners instead. OTOH, this fix is relatively trivial, and if the JVM is DOA so we can't even finish the build, then maybe it makes no point to integrate it until that is fixed.

TheShermanTanker · 2025-01-17T08:26:17Z

Member

Fixing the JVM under LTO is likely to be a heavy undertaking, much more so than just unbreaking compilation and linking of the JVM (Ignoring that the JVM later crashes when the newly compiled JDK is used to build parts of itself), I'm not sure it would be feasible under the current Pull Request

8345265

d6109a1

TheShermanTanker marked this pull request as draft November 30, 2024 00:36

DISABLE_POISONING_STOPGAP in compilerWarnings_gcc.hpp

b0b06bf

openjdk bot changed the title ~~8345265~~ 8345265: Fix gcc LTO without disabling LTO for g1ParScanThreadState.cpp Nov 30, 2024

Declaration fix in compilerWarnings_gcc.hpp

e725dba

openjdk bot added build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Nov 30, 2024

TheShermanTanker added 2 commits November 30, 2024 08:39

Stopgap fix in os.cpp

856a719

Fix memory leak in jvmciEnv.cpp

a357017

TheShermanTanker marked this pull request as ready for review November 30, 2024 00:43

openjdk bot added the rfr Pull request is ready for review label Nov 30, 2024

TheShermanTanker marked this pull request as draft November 30, 2024 03:09

openjdk bot removed the rfr Pull request is ready for review label Nov 30, 2024

kimbarrett suggested changes Nov 30, 2024

View reviewed changes

TheShermanTanker added 4 commits December 8, 2024 12:49

Revert os.cpp

b8d1c8a

Merge branch 'openjdk:master' into patch-16

55b89a1

Revert DISABLE_POISONING_STOPGAP compilerWarnings_gcc.hpp

5f9c8c8

General LTO fixes JvmFeatures.gmk

b244b86

TheShermanTanker added 2 commits December 17, 2024 22:41

Revert compilerWarnings_gcc.hpp

0c3b5c5

-fno-omit-frame-pointer in JvmFeatures.gmk

89e0cfd

TheShermanTanker marked this pull request as ready for review December 17, 2024 14:47

openjdk bot added the rfr Pull request is ready for review label Dec 17, 2024

Merge branch 'openjdk:master' into patch-16

9d05cb8

TheShermanTanker changed the title ~~8345265: Fix gcc LTO without disabling LTO for g1ParScanThreadState.cpp~~ 8345265: Minor improvements for LTO across all compilers Jan 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8345265: Minor improvements for LTO across all compilers #22464

8345265: Minor improvements for LTO across all compilers #22464

TheShermanTanker commented Nov 30, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 30, 2024

openjdk bot commented Nov 30, 2024

openjdk bot commented Nov 30, 2024

mlbridge bot commented Nov 30, 2024 •

edited

Loading

kimbarrett left a comment

kimbarrett Nov 30, 2024

kimbarrett Nov 30, 2024

TheShermanTanker Dec 8, 2024

kimbarrett Nov 30, 2024

kimbarrett Nov 30, 2024

kimbarrett Nov 30, 2024

TheShermanTanker commented Nov 30, 2024

TheShermanTanker commented Dec 8, 2024

MBaesken commented Dec 9, 2024

TheShermanTanker commented Dec 9, 2024

TheShermanTanker commented Dec 17, 2024

TheShermanTanker commented Jan 9, 2025

MBaesken commented Jan 9, 2025

kimbarrett commented Jan 10, 2025

kimbarrett commented Jan 10, 2025

magicus commented Jan 10, 2025

TheShermanTanker commented Jan 17, 2025

8345265: Minor improvements for LTO across all compilers #22464

Are you sure you want to change the base?

8345265: Minor improvements for LTO across all compilers #22464

Conversation

TheShermanTanker commented Nov 30, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewing

bridgekeeper bot commented Nov 30, 2024

openjdk bot commented Nov 30, 2024

openjdk bot commented Nov 30, 2024

mlbridge bot commented Nov 30, 2024 • edited Loading

Webrevs

kimbarrett left a comment

Choose a reason for hiding this comment

kimbarrett Nov 30, 2024

Choose a reason for hiding this comment

kimbarrett Nov 30, 2024

Choose a reason for hiding this comment

TheShermanTanker Dec 8, 2024

Choose a reason for hiding this comment

kimbarrett Nov 30, 2024

Choose a reason for hiding this comment

kimbarrett Nov 30, 2024

Choose a reason for hiding this comment

kimbarrett Nov 30, 2024

Choose a reason for hiding this comment

TheShermanTanker commented Nov 30, 2024

TheShermanTanker commented Dec 8, 2024

MBaesken commented Dec 9, 2024

TheShermanTanker commented Dec 9, 2024

TheShermanTanker commented Dec 17, 2024

TheShermanTanker commented Jan 9, 2025

MBaesken commented Jan 9, 2025

kimbarrett commented Jan 10, 2025

kimbarrett commented Jan 10, 2025

magicus commented Jan 10, 2025

TheShermanTanker commented Jan 17, 2025

TheShermanTanker commented Nov 30, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 30, 2024 •

edited

Loading