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

Probe and dlopen() the correct libstdc++ #46976

Merged
merged 28 commits into from
Nov 8, 2022

Conversation

apaz-cli
Copy link
Member

@apaz-cli apaz-cli commented Sep 29, 2022

Fixes JuliaCI/julia-buildkite#205
Fixes #34276
Fixes JuliaGL/GLFW.jl#198

Initial implementation by @giordano

To opt-out of the fix, set the JULIA_PROBELIBSTDCXX environment variable to 0; e.g. export JULIA_PROBELIBSTDCXX=0.

TODO:

  • Move the libstdcxx.so.6 that we ship off rpath
  • Load from the correct location when installed with make install
  • Is LIBSTDCXXPROBE a good name for the #define and JULIA_PROBELIBSTDCXX a good name for the environment variable?
  • Parse CSL_NEXT_GLIBCXX_VERSION for the symbol to extract and check for compatibility.

Copy link
Contributor

@giordano giordano left a comment

Choose a reason for hiding this comment

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

Thanks for taking this over!

cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/Makefile Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
@staticfloat
Copy link
Member

Also, to bikeshed the names of things a bit, I tend to prefer "verb-object" names where possible, so PROBE_LIBSTDCXX, rather than LIBSTDCXXPROBE, as the latter is somewhat ambiguous whether there is a library called libstdcxxprobe.so or whatnot. This may be moot if we end up just using _OS_LINUX_ guards, but it's useful nonetheless.

One thing that I'm not 100% happy about here is that we will be introducing a bit of a behavior change on Linux versus everything else. In particular, the path of our bundled libstdc++ is going to be different. I'm of half a mind to just have this probing occur on all platforms, no matter what, and make things consistent across all platforms. What do you all think?

@apaz-cli
Copy link
Member Author

One thing that I'm not 100% happy about here is that we will be introducing a bit of a behavior change on Linux versus everything else. In particular, the path of our bundled libstdc++ is going to be different. I'm of half a mind to just have this probing occur on all platforms, no matter what, and make things consistent across all platforms. What do you all think?

I agree with keeping behavior consistent across all platforms, but I'm also not sure if this is an issue on other platforms. Also obviously if we want to do this on Windows it's going to look completely different.

@apaz-cli
Copy link
Member Author

Om my machine, the path that is loaded is: /home/apaz/git/ejulia/usr/bin/../lib/libstdc++.so. Should we do path normalization?

@vtjnash
Copy link
Member

vtjnash commented Sep 30, 2022

path normalization is likely unnecessary, as the loader will probably capture the realpath later (a function which, notoriously, is the only one that does suffer from PATH_MAX issues, and has no posix replacement 👀)

cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
@apaz-cli
Copy link
Member Author

apaz-cli commented Oct 4, 2022

Currently, buildkite shows that it does fail to dlopen() the fallback libstdc++.so.6 on x86_64-linux-musl. So, perhaps there should be a check for gnu after all?

@giordano
Copy link
Contributor

giordano commented Oct 4, 2022

Do we have an idea of why? I can't easily tell why this should fail to dlopen system libstdc++ (and I'm not even sure this is system libstdc++ at this stage yet as I believe Julia's one still has the priority).

@apaz-cli
Copy link
Member Author

apaz-cli commented Oct 4, 2022

Do we have an idea of why?
https://buildkite.com/julialang/julia-master/builds/16493#01839fe3-7e36-4069-bade-e3b8582c9b6e
dlopen: Invalid argument

@staticfloat
Copy link
Member

staticfloat commented Oct 4, 2022

@apaz-cli you can debug this with the same environment as on CI by using the bughunt tool. Using a Linux machine, just run julia --project bughunt.jl https://buildkite.com/julialang/julia-master/builds/16493#01839fe3-7e36-4069-bade-e3b8582c9b6e and it'll drop you in a shell inside of the same build environment as we use on CI, with the correct Julia checkout. So you can then look at the make flags here and re-create the same make command. Then you can recreate the error locally, eyeball the command that's failing, then run that command inside of gdb (included in the rootfs image) or strace (you can download a static strace):

$ /build/julia.git/usr/bin/julia -C "generic;sandybridge,-xsaveopt,clone_all;haswell,-rdrnd,base(1)" --output-ji /build/julia.git/usr/lib/julia/corecompiler.ji.tmp --startup-file=no --warn-overwrite=yes -g0 -O0 compiler/compiler.jl
libstdc++.so.6dlopen: Invalid argument

For the record, there is a libstdc++.so.6 available within the build rootfs as well:

$ find / -name libstdc++.so.6 2>/dev/null
/usr/lib/libstdc++.so.6
/build/julia.git/usr/lib/libstdc++.so.6

@staticfloat
Copy link
Member

Alright, definite progress! Next thing is to do the moving of libstdc++'s install locations, as I outlined in this comment.

Make.inc Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
cli/loader_lib.c Outdated Show resolved Hide resolved
@apaz-cli
Copy link
Member Author

apaz-cli commented Nov 7, 2022

This PR should be ready to merge, unless there's something else to add. The previous changes were comment typos only.

@vtjnash
Copy link
Member

vtjnash commented Nov 7, 2022

There is a conflict now with CSL_NEXT_GLIBCXX_VERSION, since that was recently bumped, but once you resolve that, it seems good to me.

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Nov 8, 2022
@giordano
Copy link
Contributor

giordano commented Nov 8, 2022

Final benchmarks, f7d4edc (master) vs 7685f15 (#46976):

hot start:

% hyperfine -r 50 "./julia --startup-file=no -e ''" "46976-mg-dlopen-libstdc++/julia --startup-file=no -e ''"
Benchmark 1: ./julia --startup-file=no -e ''
  Time (mean ± σ):     114.4 ms ±   1.0 ms    [User: 87.2 ms, System: 75.9 ms]
  Range (min … max):   113.1 ms … 117.2 ms    50 runs
 
Benchmark 2: 46976-mg-dlopen-libstdc++/julia --startup-file=no -e ''
  Time (mean ± σ):     114.6 ms ±   1.0 ms    [User: 88.6 ms, System: 74.2 ms]
  Range (min … max):   113.2 ms … 117.3 ms    50 runs
 
Summary
  './julia --startup-file=no -e ''' ran
    1.00 ± 0.01 times faster than '46976-mg-dlopen-libstdc++/julia --startup-file=no -e '''

cold start:

% hyperfine --prepare 'sync; echo 3 | sudo tee /proc/sys/vm/drop_caches' -r 50 "./julia --startup-file=no -e ''" "46976-mg-dlopen-libstdc++/julia --startup-file=no -e ''"
Benchmark 1: ./julia --startup-file=no -e ''
  Time (mean ± σ):     253.1 ms ±   3.9 ms    [User: 141.5 ms, System: 175.6 ms]
  Range (min … max):   245.4 ms … 262.2 ms    50 runs
 
Benchmark 2: 46976-mg-dlopen-libstdc++/julia --startup-file=no -e ''
  Time (mean ± σ):     249.6 ms ±   6.7 ms    [User: 134.3 ms, System: 178.0 ms]
  Range (min … max):   238.3 ms … 275.8 ms    50 runs
 
Summary
  '46976-mg-dlopen-libstdc++/julia --startup-file=no -e ''' ran
    1.01 ± 0.03 times faster than './julia --startup-file=no -e '''

Timings are compatible within error bars. Note that in the meantime #45582 has been merged, so the fact timings are consistent is good because now this probe should be no-op.

Thanks a lot @apaz-cli for this work! 🚀

@giordano giordano merged commit eb708d6 into JuliaLang:master Nov 8, 2022
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Nov 8, 2022
@vtjnash vtjnash added backport 1.8 Change should be backported to release-1.8 backport 1.9 Change should be backported to release-1.9 labels Nov 18, 2022
KristofferC pushed a commit that referenced this pull request Nov 28, 2022
* Probe if system libstdc++ is newer than ours

If the system libstdc++ is detected to be newer, load it.
Otherwise, load the one that we ship. This improves compatibility
with external shared libraries that the user might have on their
system.

Fixes #34276

Co-authored-by: Jameson Nash <[email protected]>
Co-authored-by: Elliot Saba <[email protected]>

* Addressed review comments.

* Change error handling in wrapper functions

Co-authored-by: Jameson Nash <[email protected]>

* Call write_wrapper three times instead of snprintf

Co-authored-by: Jameson Nash <[email protected]>

* Apply suggestions from code review

Co-authored-by: Jameson Nash <[email protected]>

* Update cli/loader_lib.c

Co-authored-by: Jameson Nash <[email protected]>

* Reordered reading and waiting to avoid a deadlock.

* Fixed obvious issues.

* Only load libstdc++ preemptively on linux.

* Update cli/loader_lib.c

Co-authored-by: Jameson Nash <[email protected]>

* Update cli/loader_lib.c

Co-authored-by: Jameson Nash <[email protected]>

* Specified path to bundled libstdc++ on the command line.

* Removed whitespace.

* Update cli/Makefile

Co-authored-by: Jameson Nash <[email protected]>

* Handled make install stringreplace.

* Correctly quoted stringreplace.

* Added -Wl,--enable-new-dtags to prevent DT_RPATH for transitive dependencies

* Updated news entry.

* Added comment about environment variable.

* patched rpath for libgfortran and libLLVM.

* Added explaination to Make.inc

* Removed trailing space

* Removed patchelf for libgfortran, now that BB has been fixed.

* Fixed typos and comments

Co-authored-by: Max Horn <[email protected]>

Co-authored-by: Mosè Giordano <[email protected]>
Co-authored-by: Jameson Nash <[email protected]>
Co-authored-by: Elliot Saba <[email protected]>
Co-authored-by: Max Horn <[email protected]>
(cherry picked from commit eb708d6)
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Dec 8, 2022
@KristofferC KristofferC mentioned this pull request Dec 14, 2022
26 tasks
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Dec 16, 2022
@fxcoudert
Copy link
Contributor

This has broken build on Linux with USE_SYSTEM_CSL=1 in Homebrew. It passed for Julia 1.8.3 and fails for Julia 1.8.4 and 1.8.5. I believe the issue is this one: #47987

staticfloat added a commit that referenced this pull request 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
staticfloat added a commit that referenced this pull request 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
staticfloat added a commit that referenced this pull request 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
KristofferC pushed a commit that referenced this pull request Feb 1, 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

(cherry picked from commit fb97c82)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet