Fix santizers on MacOS that require dylibs in the LLVM toolchain. #250
Conversation
timbess
requested review from
siddharthab,
rrbutani and
jsharpe
as code owners
|
Thanks for the contribution. I am not knowledgeable enough to be confident if this is the right approach, but let's land this and see what happens. |
| link_flags.extend([ | ||
| "-Bstatic", | ||
| "-lunwind", | ||
| "-Bdynamic", |
There was a problem hiding this comment.
This seems to contradict the comment, wouldn't we want to link libraries from the toolchain (not the sysroot) statically instead?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@keith Should we revert or do you see a simple way to fix this?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
But with the static linking I get:
There was a problem hiding this comment.
I had some comments about this behaviour in the code before this change. Reverting to some of that may solve the issues here.
There was a problem hiding this comment.
Seems like I'm getting libunwind from the actual LLVM toolchain, not the sysroot.
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.
…zel-contrib#250) Add dylibs to the `lib` target so that santizers work properly on MacOS. Resolves bazel-contrib#192. --------- Co-authored-by: Siddhartha Bagaria <starsid@gmail.com>
Add dylibs to the
libtarget so that santizers work properly on MacOS.Should address #192.