Details
- Reviewers
gkm - Group Reviewers
Restricted Project - Commits
- rGdb7a413e51c5: [lld-macho] Re-root absolute input file paths if -syslibroot is specified
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
200 ms | x64 debian > lld.MachO::reroot-path.s |
Event Timeline
lld/MachO/Driver.cpp | ||
---|---|---|
121 | Since we may return path; from this function, having it return an StringRef instead of an std::string is natural. I therefore modified findPathCombination to return an Optional<StringRef> as well. For uniformity, findFramework should be modified similarly; I'll do it in a separate commit to avoid cluttering this diff. | |
lld/test/MachO/dependency-info.s | ||
23 | we now attempt to search for a bunch of rerooted paths, so there are more not-found entries |
lld/MachO/Driver.cpp | ||
---|---|---|
80 | What does \p mean here in this comment? | |
lld/test/MachO/reroot-path.s | ||
27–28 | These don't test what you want, since they succeed even without -syslibroot:
| |
29–30 | Same behavior here for libbar.dylib vs. bar.a | |
31–33 | Same |
lld/test/MachO/reroot-path.s | ||
---|---|---|
27–28 | I believe the second case is what you want: with -syslibroot %t and s/%t/%:t/bar.a/%t/bar.a/ |
lld/MachO/Driver.cpp | ||
---|---|---|
80 | Parameter. It's used by Doxygen: https://www.doxygen.nl/manual/commands.html#cmdp | |
lld/test/MachO/reroot-path.s | ||
27–28 | yea good catch. I'll check the actual path accessed with -t. |
lld/test/MachO/reroot-path.s | ||
---|---|---|
26–29 | Don't we also want to test with %t/bar.a and %t/libbar.dylib to demonstrate that -syslibroot %t was honored %t prepended to find the file? |
lld/test/MachO/reroot-path.s | ||
---|---|---|
26–29 | oops, yeah. This should just be "%t/libbar.dylib", testing "%t/:%t/libbar.dylib" is unnecessary... I had this right in an earlier version of the diff :p |
lld/test/MachO/reroot-path.s | ||
---|---|---|
29–30 | Almost there! These also need to drop the %:t/ component and become %t/libbar.dylib on the lld command line. |
lld/test/MachO/reroot-path.s | ||
---|---|---|
29–30 | It looks right to me as-is. Are you looking at the right version of the diff? |
lld/test/MachO/reroot-path.s | ||
---|---|---|
29–30 | Line 29 is good now with %t/libbar.dylib, but 28 still contains %t/%:t/libbar.dylib |
lld/test/MachO/reroot-path.s | ||
---|---|---|
29–30 | ugh, yes. Sorry, not sure how I missed that |
Does this do the right thing with --reproduce? (cf createResponseFile() in DriverUtils.cpp)
Does this do the right thing with --reproduce? (cf createResponseFile() in DriverUtils.cpp)
... kinda. It mostly works, except when there's another file at the non-rerooted absolute path, in which case we emit the wrong path. Fix coming up in a bit.
What does \p mean here in this comment?