-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8371893: [macOS aarch64] use dead_strip linker option to reduce binary size #28319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
|
Is there is a particular reason to not apply this on x64 as well? |
On macOS x86_64 I saw in our CI those tests failing when enabling the dead_strip feature (and macOS on Intel is not that interesting any more these days) For macOS aarch64, the tests in our CI were fine but I see some failures in the GHA in the serviceability area. Have to check why I do not see those issues in our test infra. |
That information should at least be stated in the bug for future reference on why this decision was made.
Adding serviceability label as this is affecting their tests. I have a vague memory of some seemingly dead code being necessary for certain sa functionality. /label serviceability |
|
@erikj79 |
Yeah I remember too, Is there a way to attribute such 'dead' functions with e.g. an attribute to pragma or something similar to avoid removal of those ? |
|
There was a bit of discussion on serviceability-dev some years ago about that lto on libjvm issue and the serviceability errors and Magnus commented 'We used to have LTO enabled on the old, closed-source Oracle arm-32 builds. There is still a "link-time-opt" JVM feature present; afaik it still works and adds the -flto flag. The main drawback of this is the extremely long link times of libjvm.so. Probably the dead-strip feature runs into similar issues. I commented back then |
|
Surely you're going to use features that are provided for debugging. They aren't called from anywhere, but they are used when debugging HotSpot. And you still need them even if you're not using a debug build. |
|
SA is throwing an exception on the following:
This looks to be the first thing it is doing that involves looking up a hotspot symbol. Possibly the issue is with symbols in general being stripped, or maybe just this one was. The fact that only 3 tests failed makes me think it is some very selective dead stripping of symbols that is the problem. Most SA functionality does not use SystemDictionary().getClassLoaderKlass(), so if just this one symbol was missing, that would explain why for the most part SA is working. On the assumption that it is just this one symbol, Hotspot declares an array of InstanceKlass* for some critical classes: In SA, the failed SystemDictionary.initialize() part of the stacktrace shows an attempt to access this array for the ClassLoader InstanceKlass*
And in vmStructs we have:
I think this vmStructs static field is getting dead stripped. |
|
Sorry, I think I went somewhat astray in my comments above. I think public static InstanceKlass getClassLoaderKlass() { instantiateWrapperFor() is failing. If you look at the stack trace (and the accompanying code), it appears that it has failed to lookup the base type, which is Metadata. There probably are no instances of Metadata allocated, just instances of subclasses, so possibly the Metadata vtbl has been dead stripped, and SA needs it. However, I would think this would result in mass failures since we call Metadata.instantiateWrapperFor() all over the place. If you want to attempt a fix, have hotspot allocate a Metadata instance somewhere and keep a reference to it. That should prevent the Metadata vtbl from being dead stripped. |
|
As usual, maybe it is possible to split this patch between hotspot and the libraries? |
|
I reproduced the SA issues locally. Not surprisingly almost every SA test failed (I'm not sure why only 3 were listed above). They seemed to all fail trying to get a vtable for a hotspot type. The tests that didn't failed were doing mundane things like testing attaching, hotspot flags, and clhsdb history, but not doing anything with hotspot objects, or at least not with hotspot metadata. I was able to confirm the problem is with the Metadata vtable missing. I fixed it by allocating a Metadata instance and assigning it to a global. However, in order to get this to compile I had make Metadata no longer be abstract. All tests passed after I did this. I'm not sure how viable a solution this is. The hotspot team will probably object to the Metadata changes. Maybe there is some other way to cause a reference to the Metadata vtable so it is not dead stripped. |
Would be good to have a fix people agree on, because we already saw the issue with lto enabled for Hotspot (see my comment above). So it is not only an issue related to the dead_strip flag. |
For sure we can do this. |
Okay I checked why this is the case. |
|
I remember s390 used to strip unneeded stuff, and it was very hard to debug anything. Please think about whether you need to do this. |
What exactly was hard to debug? |
|
Btw on Linux s390x it is the jdk/make/autoconf/jdk-options.m4 Line 112 in 0bff5f3
jdk/make/autoconf/flags-ldflags.m4 Line 55 in 0bff5f3
It is just enabled by default on Linux s390x because there we have no serviceability agent support. |
There seem to be attributes for this kind of use case in clang, e.g. retain |
|
On 19/11/2025 13:22, Matthias Bäsken wrote:
It was hard to debug hotspot. Helpers such as pp(), back_trace(), and pfl() were missing. I think it was a release build, but of course you sometimes have to debug release builds. |
Yes, it can. Thankfully I don't usually need to debug hotspot in production s390 systems. I do, however, need to debug AArch64 on MacOS. This is a Very Bad Idea, and should not be dome. |
|
Seems like we should fix the elimination of those debug helpers ; but those are only in the jvm lib. The situation in other libs is different. |
I tried the following but it didn't work. Perhaps I don't have the syntax right: Note |
|
The following is how you declare a reference to the vtable: extern "C" void* _ZTV8Metadata[]; If you then reference _ZTV8Metadata from somewhere in a way that does not get dead stripped, that seems to fix the problem. The latter part is definitely very hacky. It would be nice to get a pragma working to keep the symbol from being deadstripped. |
|
This seems to work: You can put this anywhere since it does not reference any hotspot types. It just needs to be linked in with libjvm. Note I tried putting the "used" attribute on _ZTV8Metadata and getting rid of the foo() part, but I got the following warning and the vtable was dead stripped:
|
Maybe, but it's not just the specific debug helpers. There are many functions in hotspot, and the maintenance programmer (in the case of Mac/AArch64, that's often me) will frequently type stuff like Linker options that strip "unused" code in binaries have been available for decades, and we haven't much used such options because they're a pain. Debugging on MacOS/AArch64 is a pain as it is, without making it worse. |
|
On further consideration, I remembered that many of the helpers are debug-only, so that shouldn't be a problem. Maybe if we only do this on non-PRODUCT builds it's be OK, but it would require very careful thought and analysis. |
Thanks for looking into it. I can disable the dead_strip switch for the debug-builds, so it would not eliminate these helpers. |
Thanks for the testing and providing the workaround. Too bad that the 'retain' and 'used' are only function attributes, this makes things a bit harder. |
|
@MBaesken |
Right, but we'll need to make sure we don't remove the non-debug ones. We don't want to exclude stuff in |
|
Interestingly, we seem to rely already on the 'used' attribute in the OpenJDK codebase at some places, see |
|
For the Apple linker there seem to be also a way to mark some sections with |
Most of the debug.cpp helpers are in product builds now. I think only pns() and pns2() are left out of product builds. |
Maybe we should for now limit the dead_strip to the JDK native libs ? This would avoid the trouble with the HS debug helpers coding and the serviceability agent (but in the long run we have this trouble also with linktime-gc and probably LTO which can be enabled by OpenJDK configure, not only with this macOS-linker related flag this PR is about ). |
The dead_strip linker option on macOS removes functions and data that are unreachable by the entry point or exported symbols.
Setting it can reduce the size of some binaries we generate quite a lot, for example (product build, Xcode 15 is used) :
(before -> after setting the option)
and libjvm :
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28319/head:pull/28319$ git checkout pull/28319Update a local copy of the PR:
$ git checkout pull/28319$ git pull https://git.openjdk.org/jdk.git pull/28319/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28319View PR using the GUI difftool:
$ git pr show -t 28319Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28319.diff
Using Webrev
Link to Webrev Comment