This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Specify unwind lib before other system libraries.
ClosedPublic

Authored by ychen on Feb 3 2019, 1:46 AM.

Details

Reviewers
phosek
EricWF
ldionne
Group Reviewers
Restricted Project
Summary

For the same reason as D57496. This and D57496 combined make local unwind lib
picked up by dyld in a libcxx/libcxxabi/libunwind build on OSX.

before
➜ libcxx otool -L lib/libc++.dylib
lib/libc++.dylib:

@rpath/libc++.1.dylib (compatibility version 1.0.0, current version 1.0.0)
@rpath/libc++abi.1.dylib (compatibility version 1.0.0, current version 1.0.0)
/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1252.200.5)
@rpath/libunwind.1.dylib (compatibility version 1.0.0, current version 1.0.0)

to
➜ libcxx otool -L lib/libc++.dylib
lib/libc++.dylib:

@rpath/libc++.1.dylib (compatibility version 1.0.0, current version 1.0.0)
@rpath/libc++abi.1.dylib (compatibility version 1.0.0, current version 1.0.0)
@rpath/libunwind.1.dylib (compatibility version 1.0.0, current version 1.0.0)
/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1252.200.5)

Event Timeline

ychen created this revision.Feb 3 2019, 1:46 AM
ychen updated this revision to Diff 184938.Feb 3 2019, 1:49 AM
  • update
phosek accepted this revision.Feb 3 2019, 10:04 PM

LGTM

This revision is now accepted and ready to land.Feb 3 2019, 10:04 PM
ychen added a comment.Mar 1 2019, 3:35 PM

Could you please commit this for me? Thanks.

@phosek Has this been committed?

phosek added a comment.May 2 2019, 6:21 PM

Not yet, but it'll probably need to be reworked given all the changes that landed in libc++.

Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2019, 10:54 AM
ldionne accepted this revision.May 3 2019, 11:00 AM

I'll admit that I find it very brittle that ordering matters. We can do this, however it would be better to find another solution that doesn't require depending on the ordering of -l parameters. I don't know what that would be, though.

ychen added a comment.May 3 2019, 11:20 AM

I'll admit that I find it very brittle that ordering matters. We can do this, however it would be better to find another solution that doesn't require depending on the ordering of -l parameters. I don't know what that would be, though.

install_name_tool -change could adjust the order for executable as a post build action. If there are additional dependent libraries, then reorder logic need to be updated. I'm not sure it would be an improvement than just put more comments in the existing code since overriding libSystem.B.dylib is a rare case.

In D57646#1490202, @tabloid.adroit wrote:

I'll admit that I find it very brittle that ordering matters. We can do this, however it would be better to find another solution that doesn't require depending on the ordering of -l parameters. I don't know what that would be, though.

install_name_tool -change could adjust the order for executable as a post build action. If there are additional dependent libraries, then reorder logic need to be updated. I'm not sure it would be an improvement than just put more comments in the existing code since overriding libSystem.B.dylib is a rare case.

innstall_name_tool is only available on Darwin unless I'm mistaken, so I think it's better not to add something that only works on a specific platform. It's an interesting suggestion nonetheless.

ychen updated this revision to Diff 198090.May 3 2019, 2:20 PM
  • add comment
ychen added a comment.May 3 2019, 2:22 PM

Hi ldionne, I definitely agree install_name_tool is not a desired solution. I've also added some comments so people realize the issue when modifying the code.

ldionne closed this revision.Sep 20 2023, 11:48 AM

[Github PR transition cleanup]

Abandoning since this will be solved by https://github.com/llvm/llvm-project/pull/66940.

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 20 2023, 11:48 AM
Herald added a project: Restricted Project. · View Herald Transcript