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.

Sign up for GitHub

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

Jump to bottom

Fix santizers on MacOS that require dylibs in the LLVM toolchain. #250

Merged
merged 3 commits into from
Mar 13, 2024
Merged

Fix santizers on MacOS that require dylibs in the LLVM toolchain. #250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4010aff
Add dylibs to the `lib` target so that santizers work properly on macos.
timbess Feb 3, 2024
5146050
Merge remote-tracking branch 'origin/master' into timbess/master
siddharthab Mar 13, 2024
c3bce0d
fix comments
siddharthab Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions toolchain/BUILD.llvm_repo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ filegroup(
[
"lib/**/lib*.a",
"lib/clang/*/lib/**/*.a",
"lib/clang/*/lib/**/*.dylib",
# clang_rt.*.o supply crtbegin and crtend sections.
"lib/**/clang_rt.*.o",
],
Expand All @@ -72,9 +73,9 @@ filegroup(
"lib/liblld*.a",
],
),
# Do not include the .dylib files in the linker sandbox because they will
# not be available at runtime. Any library linked from the toolchain should
# be linked statically.
# Include the .dylib files in the linker sandbox even though they will
# not be available at runtime to allow sanitizers to work locally.
# Any library linked from the toolchain to be released should be linked statically.
)

filegroup(
Expand Down
30 changes: 10 additions & 20 deletions toolchain/cc_toolchain_config.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,32 +206,22 @@ def cc_toolchain_config(
"-ldl",
])
else:
# The only known mechanism to static link libraries in ld64 is to
# not have the corresponding .dylib files in the library search
# path. The link time sandbox does not include the .dylib files, so
# anything we pick up from the toolchain should be statically
# linked. However, several system libraries on macOS dynamically
# link libc++ and libc++abi, so static linking them becomes a problem.
# We need to ensure that they are dynamic linked from the system
# sysroot and not static linked from the toolchain, so explicitly
# have the sysroot directory on the search path and then add the
# toolchain directory back after we are done.
# Several system libraries on macOS dynamically link libc++ and
# libc++abi, so static linking them becomes a problem. We need to
# ensure that they are dynamic linked from the system sysroot and
# not static linked from the toolchain, so explicitly have the
# sysroot directory on the search path and then add the toolchain
# directory back after we are done.
link_flags.extend([
"-L{}/usr/lib".format(sysroot_path),
"-lc++",
"-lc++abi",
])

# Let's provide the path to the toolchain library directory
# explicitly as part of the search path to make it easy for a user
# to pick up something. This also makes the behavior consistent with
# targets when a user explicitly depends on something like
# libomp.dylib, which adds this directory to the search path, and would
# (unintentionally) lead to static linking of libraries from the
# toolchain.
link_flags.extend([
"-Bstatic",
"-lunwind",
"-Bdynamic",
Copy link
Member

Choose a reason for hiding this comment

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

This seems to contradict the comment, wouldn't we want to link libraries from the toolchain (not the sysroot) statically instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment says that we should dynamically link libc++ and libc++abi from the sysroot, instead of static linking from the toolchain. I don't have a good memory of why I wanted to do that beyond what the comment says.

@timbess Can you comment a little on why you needed to statically link libunwind here?

Copy link
Member

Choose a reason for hiding this comment

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

note that in general this is wrong for macOS since -Bstatic and -Bdynamic are not supported, but since clang is the linker they are accepted and just ignored. This results in libunwind being linked dynamically but no rpaths being set on the binary and a runtime crash similar to #224

Copy link
Member

Choose a reason for hiding this comment

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

@keith Should we revert or do you see a simple way to fix this?

Copy link
Member

Choose a reason for hiding this comment

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

maybe a better solution for libunwind would be to just exclude it from the glob above on macOS so that the system one has to be used?

Copy link
Member

Choose a reason for hiding this comment

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

I think this wasn't spotted because you actually have to use something from it for it to be an issue, which is the case we hit

Copy link
Contributor Author

@timbess timbess Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry didn't see this until now somehow... If I remember right, I was trying to get all the sanitizers working which it wouldn't find without the dylibs in the glob, but then it would end up dynamically linking libunwind and fail with an RPATH error. There was a static archive in the same directory, but ld64 would prefer the dylib unless I set -Bstatic after which it didn't show as being dynamically linked when I ran otool -L, but maybe @keith is right and I just effectively hid the error until any libunwind symbols are actually used?

Ah ok so I just replicated it. If I comment out the -Bstatic I get this:
image

But with the static linking I get:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I had some comments about this behaviour in the code before this change. Reverting to some of that may solve the issues here.

Copy link
Contributor Author

@timbess timbess Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Seems like I'm getting libunwind from the actual LLVM toolchain, not the sysroot.
image

I think the issue was that it was dynamically linking to a library that was symlinked into the sandbox, but the Bazel output dir isn't in the RPATH of the final binary, so it'd fail to find libunwind at boot. But it seems to be respecting the static linking flags from what I can tell.

"-L{}lib".format(toolchain_path_prefix),
])

elif stdlib == "libc++":
cxx_flags = [
"-std=" + cxx_standard,
Expand Down
Loading