-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries #14064
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
Conversation
… first. Then create the static library using the output object file produced by partial linking.
…LSA_CommonUtils.c change.
There is a linking failure with linux-x86 build: /usr/bin/ld: relocatable linking with relocations from format elf32-i386 (/home/runner/work/jdk/jdk/build/linux-x86/hotspot/variant-server/libjvm/libgtest/objs/gmock-all.o) to format elf64-x86-64 (/home/runner/work/jdk/jdk/build/linux-x86/hotspot/variant-server/libjvm/libgtest/objs/libgtest_relocatable.o) is not supported
|
👋 Welcome back jiangli! A progress list of the required criteria for merging this PR into |
|
@jianglizhou The following labels will be automatically applied to this pull request:
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. |
erikj79
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran this patch in our internal build and test system and got failures on macos and windows. I think I know the cause for the mac failure, but the Windows failure I don't know. It looks like the same error I saw before after the original patch went in.
[2023-05-19T20:51:31,466Z] c:\sb\prod\1684529071\workspace\open\src\hotspot\share\compiler\disassembler.cpp(792): error C2220: the following warning is treated as an error
[2023-05-19T20:51:31,466Z] c:\sb\prod\1684529071\workspace\open\src\hotspot\share\compiler\disassembler.cpp(792): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data
[2023-05-19T20:51:31,466Z] lib/CompileJvm.gmk:152: recipe for target '/cygdrive/c/sb/prod/1684529071/workspace/build/windows-x64-open/hotspot/variant-server/libjvm/objs/static/disassembler.obj' failed
[2023-05-19T20:51:31,466Z] make[3]: *** [/cygdrive/c/sb/prod/1684529071/workspace/build/windows-x64-open/hotspot/variant-server/libjvm/objs/static/disassembler.obj] Error 1
make/common/NativeCompilation.gmk
Outdated
| $$(call ExecuteWithLog, $$($1_OBJECT_DIR)/$$($1_SAFE_NAME)_partial_link, \ | ||
| $$($1_LD) $(LDFLAGS_CXX_PARTIAL_LINKING) $(LD_OUT_OPTION)$$($1_TARGET_RELOCATABLE) \ | ||
| $$($1_LD_OBJ_ARG)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command line is missing $$($1_SYSROOT_LDFLAGS) which is causing it to fail in our builds with:
ld: library not found for -lSystem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the mac failure, sorry if that wasn't clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @erikj79. Could you please help provide some more info on the build failure:
Which macOs version ran into the build issue? My mac is on Ventura 13.3.1 (a). It builds successfully for the static-libs-image target.
Does it fail when partially linking libjvm_relocatable.o only, or it fails when partially linking other native libraries as well on macosx? Could you please share the partial linking command, e.g. hotspot/variant-server/libjvm/gtest/objs/static/BUILD_GTEST_LIBJVM_partial_link.cmdline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @erikj79. Could you please help provide some more info on the build failure:
Which macOs version ran into the build issue? My mac is on Ventura 13.3.1 (a). It builds successfully for the
static-libs-imagetarget.Does it fail when partially linking
libjvm_relocatable.oonly, or it fails when partially linking other native libraries as well on macosx? Could you please share the partial linking command, e.g. hotspot/variant-server/libjvm/gtest/objs/static/BUILD_GTEST_LIBJVM_partial_link.cmdline?
Additional info, following is the partial linking command from my macosx-x86_64 build:
/usr/bin/g++ -m64 -r -o /.../github/JDK-8307858/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/gtest/objs/static/libjvm_relocatable.o @/.../github/JDK-8307858/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/gtest/objs/static/_BUILD_GTEST_LIBJVM_objectfilenames.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fails because we are using a "devkit" and not an installed Xcode. The SYSROOT flags must always be added to any compiler or link command lines.
Thanks a lot, @erikj79! I was going to ask your help for testing the patch.
The warning error looks the same as https://bugs.openjdk.org/browse/JDK-8307858?focusedCommentId=14581027&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14581027. Sorry I didn't look closely earlier. I just took a look of
#ifdef STATIC_BUILD The code is only enabled for the static build, which is why the issue doesn't show up in regular JDK build. Could you please help fix the mac failure as you have already looked into it. Thanks! |
|
/contributor add @erikj79 Thanks for running tests!! |
|
@jianglizhou |
| endif | ||
|
|
||
| ifeq ($$($1_TYPE), STATIC_LIBRARY) | ||
| $1_VARDEPS := $$($1_AR) $$(ARFLAGS) $$($1_ARFLAGS) $$($1_LIBS) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to add some things to VARDEPS. Looks like at least $$($1_LD) and $$($1_SYSROOT_LDFLAGS) are needed. Maybe conditionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix, thanks.
Co-authored-by: Erik Joelsson <37597443+erikj79@users.noreply.github.com>
…YPE) is gcc or clang, as suggested by @erikj79.
|
Hi @erikj79 Please let me know if you have any additional comments for the PR. Could you please also help run through testing for the latest version? Thanks |
I'm running a full set of builds now, but in my initial local build on mac, I see a lot of warnings like this: Any idea what that could be caused by or if it's possible to eliminate? |
erikj79
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My build job is still running, but it has failed in two distinct ways already. See below for mac fix. Our cross build of arm32 fails with this message:
[2023-05-24T19:25:15,310Z] /opt/mach5/mesos/work_dir/jib-master/install/jpg/infra/builddeps/devkit-linux_x64-to-linux_arm/gcc8.2.0-Fedora27+1.0/devkit-linux_x64-to-linux_arm-gcc8.2.0-Fedora27+1.0.tar.gz/x86_64-linux-gnu-to-arm-linux-gnueabihf/bin/../lib/gcc/arm-linux-gnueabihf/8.2.0/../../../../arm-linux-gnueabihf/bin/ld: fatal error: cannot mix -r with dynamic object /opt/mach5/mesos/work_dir/jib-master/install/jpg/infra/builddeps/devkit-linux_x64-to-linux_arm/gcc8.2.0-Fedora27+1.0/devkit-linux_x64-to-linux_arm-gcc8.2.0-Fedora27+1.0.tar.gz/x86_64-linux-gnu-to-arm-linux-gnueabihf/bin/../lib/gcc/arm-linux-gnueabihf/8.2.0/../../../../arm-linux-gnueabihf/lib/libstdc++.so
I observed the same warnings on my macosx-x86_64 local build. It's from the partial linking step. Not much information yielded when I searched for I tried linking a Haven't found any information on how to suppress the warning either. |
Thanks you! Co-authored-by: Erik Joelsson <37597443+erikj79@users.noreply.github.com>
The partial linking was originally suggested by C++/Clang toolchain folks to mitigate linker overhead that was observed during final executable link time. For a static library containing any object file ( Is it possible |
Another possibility might be the user provided |
I tried building @erikj79 Could you please help provide additional insight for the build failure you found for arm32? For the |
|
Fix typo: |
There are no extra options. I suspect it's an issue with the older GCC version. Here is one failing command line:
This seems to be Clang specific, so maybe only apply this to Clang and not GCC? Have you measured the difference in link time and concluded that this workaround is needed? If so, how big were the differences? Given that it prints a lot of warnings on mac, I would suggest leaving the partial linking out of this patch to get something in that is actually working. It was included in this patch because of the side effect it had with handling long path names. However, since looking closer at that issue, we were using a different workaround for Clang already and that same workaround should work fine for AR on mac as well. |
Thanks a lot for looking into the issue.
It was noticeably slower (which prompted me looked into it at that time) when linking the executable using the static libraries created without partial linking workaround. In my experiments, it took >2m when linking final executable. With the mitigation the final linking took a few seconds.
Sounds good for solving the macosx |
I have no concerns regarding clang on Linux. |
- Only do partial linking step for building static libraries with clang on linux. - On macosx, workaround the long argument issue for 'AR' with relative path. Tested building jdk-image and static-libs-image on linux-x64 (for both gcc and clang) and macosx-x64 (clang) manually.
…R command if relative path is used.
I've updated this PR to include partial linking for building static libraries with clang on Linux only. On macosx builds, relative path is used when needed for long path and many objects. @erikj79, could you please help run another iteration of your tests to see if there are any remaining missed cases. Hopefully we'll be able to wrap up this change. Thanks! |
erikj79
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All our builds succeeded, so this is looking pretty good now. Just a minor suggestion left.
| ifeq ($(call isTargetOs, linux), true) | ||
| ifneq ($(findstring $(TOOLCHAIN_TYPE), clang), ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This combination of conditions is repeated 3 times. Maybe we could assign the result to a variable (e.g. $1_ENABLE_PARTIAL_LINKING) and check for that instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This combination of conditions is repeated 3 times. Maybe we could assign the result to a variable (e.g.
$1_ENABLE_PARTIAL_LINKING) and check for that instead?
Done, thanks.
…d be enabled, as suggested by @erikj79.
Thanks a lot! |
|
@jianglizhou This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
|
Going to push as commit 45414fc.
Your commit was automatically rebased without conflicts. |
|
@jianglizhou Pushed as commit 45414fc. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Original description for JDK-8307194 change:
This PR is branched from the makefile changes for https://bugs.openjdk.org/browse/JDK-8303796 and contains the following for handling the JDK/hotspot static libraries:
Build hotspot libjvm.a and JDK static libraries for static-libs-image/static-libs-bundles targets; This change does not affect the graal-builder-image target
For libjvm.a specifically, exclude operator_new.o
Filter out "external" .o files (those are the .o files included from a different JDK library and needed when creating the .so shared library only) from JDK .a libraries; That's to avoid linker failures caused by duplicate symbols
Handle long arguments case for static build in make/common/NativeCompilation.gmk
Address @erikj79's comment in 8303796: Optionally build fully statically linked JDK image #13709 (comment) for LIBJLI_STATIC_EXCLUDE_OBJS
Updates to address build failures reported on macosx- platforms:
-rlinking option) all object files into one object. The output object file from the partial linking is then passed toarto create the static library.The original change for JDK-8307194 used @argument_file for all platforms when dealing with long arguments to
ar, which caused failures on macosx- builds. On darwin (https://www.unix.com/man-page/osx/1/ar/),ardoes not support @argument_file. The updated change avoids using @argument_file forar.The partial linking change is done in make/common/NativeCompilation.gmk. The flag related change is done in make/autoconf/flags-ldflags.m4 mainly.
Progress
Issue
Reviewers
Contributors
<erikj@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14064/head:pull/14064$ git checkout pull/14064Update a local copy of the PR:
$ git checkout pull/14064$ git pull https://git.openjdk.org/jdk.git pull/14064/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14064View PR using the GUI difftool:
$ git pr show -t 14064Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14064.diff
Webrev
Link to Webrev Comment