This is an archive of the discontinued LLVM Phabricator instance.

Differential D111268

[lld][test] Fix darwin REQUIRES (NFC)
ClosedPublic

Authored by keith on Oct 6 2021, 1:42 PM.

Details

Reviewers
gkm
thevinster
Group Reviewers
Restricted Project
Commits
rG0885afb8b058: [lld][test] Fix darwin REQUIRES (NFC)
Summary

Some subprojects like compiler-rt define the darwin feature in their
lit config, but lld does not do that, so we need to use the global
system-darwin here instead. This test seems to have drifted from the
actual behavior so I also had to add /usr/local/lib here to make it
pass.

Diff Detail

Event Timeline

keith created this revision.Oct 6 2021, 1:42 PM
Herald added a reviewer: gkm. · View Herald TranscriptOct 6 2021, 1:42 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
keith requested review of this revision.Oct 6 2021, 1:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2021, 1:42 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
Harbormaster completed remote builds in B127387: Diff 377670.Oct 6 2021, 2:51 PM
thevinster added a subscriber: thevinster.Oct 6 2021, 11:48 PM
thevinster added inline comments.
lld/test/MachO/search-paths-darwin.test
2

What's the reason for not supporting darwin in LLD's lit config instead? If compiler-rt defines the feature, then it makes sense for LLD to follow a similar pattern.

keith added inline comments.Oct 7 2021, 7:56 AM
lld/test/MachO/search-paths-darwin.test
2

Since there are more uses of system-darwin across the whole project, and that configuration is centralized, that feels to me like a better one to standardize on. I think if we want to make this more clear for folks we should probably remove the custom ones from compiler-rt instead

thevinster accepted this revision.Oct 7 2021, 11:28 AM
thevinster added inline comments.
lld/test/MachO/search-paths-darwin.test
2

That's fair. Fwiw, it's not just compiler-rt. A quick search showed libcxx defining the darwin feature as well, but it does look like the majority continues to use system-darwin.

This revision is now accepted and ready to land.Oct 7 2021, 11:28 AM
keith added inline comments.Oct 7 2021, 12:01 PM
lld/test/MachO/search-paths-darwin.test
2

Yea, I audited libcxx one too and the weird part about that is it uses an entirely different heuristic to enable it, and I _think_ that means we'd need to leave it separate even if we could change compiler-rt

Closed by commit rG0885afb8b058: [lld][test] Fix darwin REQUIRES (NFC) (authored by keith). · Explain WhyOct 7 2021, 12:38 PM
This revision was automatically updated to reflect the committed changes.
keith added a commit: rG0885afb8b058: [lld][test] Fix darwin REQUIRES (NFC).
thakis added a subscriber: thakis.Oct 7 2021, 2:59 PM

This breaks lld tests: http://45.33.8.238/mac/36585/step_10.txt

Please take a look and revert for now if it takes a while to fix.

keith added a comment.Oct 7 2021, 3:09 PM

Thanks for the ping, I submitted https://reviews.llvm.org/D111361, was I supposed to get an email from that failure?

thakis added a comment.Oct 7 2021, 5:36 PM

No, those bots don't email.

Revision Contents

PathSize
lld/
test/
MachO/
3 lines

Diff 377965

lld/test/MachO/search-paths-darwin.test

REQUIRES: x86 REQUIRES: x86
REQUIRES: darwin REQUIRES: system-darwin
thevinsterUnsubmitted
Not Done
ReplyInline Actions

What's the reason for not supporting darwin in LLD's lit config instead? If compiler-rt defines the feature, then it makes sense for LLD to follow a similar pattern.

thevinster: What's the reason for not supporting `darwin` in LLD's lit config instead? If compiler-rt…
keithAuthorUnsubmitted
Done
ReplyInline Actions

Since there are more uses of system-darwin across the whole project, and that configuration is centralized, that feels to me like a better one to standardize on. I think if we want to make this more clear for folks we should probably remove the custom ones from compiler-rt instead

keith: Since there are more uses of `system-darwin` across the whole project, and that configuration…
thevinsterUnsubmitted
Not Done
ReplyInline Actions

That's fair. Fwiw, it's not just compiler-rt. A quick search showed libcxx defining the darwin feature as well, but it does look like the majority continues to use system-darwin.

thevinster: That's fair. Fwiw, it's not just compiler-rt. A quick search showed libcxx defining the…
keithAuthorUnsubmitted
Done
ReplyInline Actions

Yea, I audited libcxx one too and the weird part about that is it uses an entirely different heuristic to enable it, and I _think_ that means we'd need to leave it separate even if we could change compiler-rt

keith: Yea, I audited libcxx one too and the weird part about that is it uses an entirely different…
RUN: mkdir -p %t1 %t2 RUN: mkdir -p %t1 %t2
RUN: not ld64.lld -arch x86_64 -v -L%t1 -F%t2 2>&1 | FileCheck -DLDIR=%t1 -DFDIR=%t2 %s RUN: not ld64.lld -arch x86_64 -v -L%t1 -F%t2 2>&1 | FileCheck -DLDIR=%t1 -DFDIR=%t2 %s
CHECK: Library search paths: CHECK: Library search paths:
CHECK-NEXT: [[LDIR]] CHECK-NEXT: [[LDIR]]
CHECK-NEXT: /usr/lib CHECK-NEXT: /usr/lib
CHECK-NEXT: /usr/local/lib
CHECK-NEXT: Framework search paths: CHECK-NEXT: Framework search paths:
CHECK-NEXT: [[FDIR]] CHECK-NEXT: [[FDIR]]
CHECK-NEXT: /Library/Frameworks CHECK-NEXT: /Library/Frameworks
CHECK-NEXT: /System/Library/Frameworks CHECK-NEXT: /System/Library/Frameworks
RUN: not ld64.lld -arch x86_64 -v -L%t1 -F%t2 -Z 2>&1 | FileCheck -DLDIR=%t1 -DFDIR=%t2 --check-prefix=CHECK_Z %s RUN: not ld64.lld -arch x86_64 -v -L%t1 -F%t2 -Z 2>&1 | FileCheck -DLDIR=%t1 -DFDIR=%t2 --check-prefix=CHECK_Z %s
CHECK_Z: Library search paths: CHECK_Z: Library search paths:
CHECK_Z-NEXT: [[LDIR]] CHECK_Z-NEXT: [[LDIR]]
CHECK_Z-NEXT: Framework search paths: CHECK_Z-NEXT: Framework search paths:
CHECK_Z-NEXT: [[FDIR]] CHECK_Z-NEXT: [[FDIR]]