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

Propagate sanitizer arguments to the clang-linker-driver invocations for dynamic libraries #1325

Merged
merged 1 commit into from Apr 7, 2023

Conversation

artemcm
Copy link
Contributor

@artemcm artemcm commented Apr 6, 2023

Unless we are building a static library, we need to propagate -fsanitizer= flag to the clang linker driver, otherwise sanitizers libraries will not be found at link-time.

Resolves rdar://107733898

…for dynamic libraries

Unless we are building a static library, we need to propagate `-fsanitizer=` flag to the clang linker driver, otherwise sanitizers libraries will not be found at link-time.

Resolves rdar://107733898
@@ -199,7 +199,7 @@ extension DarwinToolchain {
// Linking sanitizers will add rpaths, which might negatively interact when
// other rpaths are involved, so we should make sure we add the rpaths after
// all user-specified rpaths.
if linkerOutputType == .executable && !sanitizers.isEmpty {
if linkerOutputType != .staticLibrary && !sanitizers.isEmpty {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@keith do you, by any chance, remember why we had this restriction for .executable?

!= .staticLibrary seems like the right thing to do here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hrm im not sure, maybe I got it from this

if linkerOutputType == .executable && !sanitizers.isEmpty {
, I think you're right though

Copy link
Collaborator

Choose a reason for hiding this comment

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

besides previously there was a check for excluding the fuzzer sanitizer for executables, so you might want that back a557d59#diff-d44ec7a906797a39be2e42ecb334167ae217537ad09a726d1f905c4ba9130200L365

@artemcm
Copy link
Contributor Author

artemcm commented Apr 6, 2023

@swift-ci test

Copy link
Member

@etcwilde etcwilde left a comment

Choose a reason for hiding this comment

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

LGTM

@artemcm artemcm merged commit 2810937 into apple:main Apr 7, 2023
3 checks passed
@artemcm artemcm deleted the SanitizersForAll_dylibs branch April 7, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants