Skip to content
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

Move libstdc++ path into LOADER_*_DEP_LIBS #48342

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

staticfloat
Copy link
Member

@staticfloat staticfloat commented Jan 19, 2023

After adding libstdc++ probing into the Julia loader [0], we originally made the assumption that the libstdc++ that is shipped with julia would always be co-located with libjulia.so [1]. This is not the case when building with USE_SYSTEM_CSL=1, however, where we sequester system libraries in usr/lib/julia, even at build-time.

The path to libstdc++.so has already been getting altered when moving from build-time to install time via stringreplace [2], but after further thought, I decided that it would be better to just use the pre-existing LOADER_*_DEP_LIBS mechanism to communicate to the loader what the correct relative path to libstdc++.so is. This also allows the single stringreplace to update all of our "special" library paths.

[0] #46976
[1] https://github.com/JuliaLang/julia/pull/46976/files#diff-8c5c98f26f3f7aac8905a1074c5bec11a57e9b9c7c556791deac5a3b27cc096fR379 [2] https://github.com/JuliaLang/julia/blob/master/Makefile#L430

Fix #47987.

@staticfloat staticfloat requested a review from vtjnash January 19, 2023 06:21
@staticfloat staticfloat added backport 1.9 Change should be backported to release-1.9 backport 1.8 Change should be backported to release-1.8 labels Jan 19, 2023
@staticfloat
Copy link
Member Author

This should fix #47987

@fxcoudert
Copy link
Contributor

Fixes the build on Linux for Homebrew. Thanks!

cli/loader_lib.c Outdated Show resolved Hide resolved
After adding `libstdc++` probing into the Julia loader [0], we
originally made the assumption that the `libstdc++` that is shipped with
`julia` would always be co-located with `libjulia.so` [1].  This is not
the case when building with `USE_SYSTEM_CSL=1`, however, where we
sequester system libraries in `usr/lib/julia`, even at build-time.

The path to `libstdc++.so` has already been getting altered when moving
from build-time to install time via `stringreplace` [2], but after
further thought, I decided that it would be better to just use the
pre-existing `LOADER_*_DEP_LIBS` mechanism to communicate to the loader
what the correct relative path to `libstdc++.so` is.  This also allows
the single `stringreplace` to update all of our "special" library paths.

[0] #46976
[1] https://github.com/JuliaLang/julia/pull/46976/files#diff-8c5c98f26f3f7aac8905a1074c5bec11a57e9b9c7c556791deac5a3b27cc096fR379
[2] https://github.com/JuliaLang/julia/blob/master/Makefile#L430
The `DEPS_LIBS` RPATH-substitute mechanism contains a list of paths to
load, and some of these paths are "special", in that they require more
involved loading than simply `load_library()`.  These libraries are
thereby denoted by a `@` prefixing them.

Previously, we made note of these libraries, then loaded them at the end
of the loading loop, but with the addition of `libstdc++` it is now
important to have the order of the libraries (including special
libraries) to be obeyed by the loading loop, so I have inlined special
library handling into the loading loop.  In the future, we may wish to
denote special libraries more explicitly than simply relying on there
being exactly three libraries, with the ordering being mapped to
`libstdc++`, `libjulia-internal`, and `libjulia-codegen`.
@staticfloat staticfloat force-pushed the sf/libstdcxx_probe_use_system_csl branch from f7176c0 to 4e99860 Compare January 19, 2023 18:17
@staticfloat
Copy link
Member Author

staticfloat commented Jan 19, 2023

I've now pushed a commit that causes the library loading (including special libraries) to obey the order as specified in DEP_LIBS. The current ordering is now:

  • libgcc_s
  • libm (usually OpenLibm, but can be system libm if USE_SYSTEM_LIBM=1)
  • libstdc++
  • libjulia-internal
  • libjulia-codegen

This ordering is important because libstdc++ depends on libgcc_s and libm, so if we are using USE_SYSTEM_LIBM=1, we technically should load libm first. Previously, we would probe and load libstdc++ first, which would bring in libgcc_s and libm from the system.

As an aside, we now basically have a little switch statement in our library loading that does different things based on which "special library" we're processing; if we get much fancier than this (e.g. if we end up doing more probes for system libraries than just libstdc++), I'm going to start implementing different tokens so that we're not so positionally-dependent with our "special libraries".

@@ -1465,7 +1465,7 @@ JULIA_SYSIMG_release := $(build_private_libdir)/sys.$(SHLIB_EXT)
JULIA_SYSIMG := $(JULIA_SYSIMG_$(JULIA_BUILD_MODE))

define dep_lib_path
$$($(PYTHON) $(call python_cygpath,$(JULIAHOME)/contrib/relative_path.py) $(1) $(2))
$(shell $(PYTHON) $(call python_cygpath,$(JULIAHOME)/contrib/relative_path.py) $(1) $(2))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This slows things down a bit (adds ~60ms to every make invocation on my linux desktop) but I think it's worth it for the extra clarity.

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Jan 19, 2023
@staticfloat staticfloat merged commit 7d2499d into master Jan 19, 2023
@staticfloat staticfloat deleted the sf/libstdcxx_probe_use_system_csl branch January 19, 2023 21:36
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jan 26, 2023
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.8 Change should be backported to release-1.8
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build fails with USE_SYSTEM_CSL=1
5 participants