Skip to content

[compiler-rt] Don't explicitly ad-hoc code sign dylibs if using Apple's new linker #88323

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 11, 2024
Merged

[compiler-rt] Don't explicitly ad-hoc code sign dylibs if using Apple's new linker #88323

merged 1 commit into from
Apr 11, 2024

Conversation

bdash
Copy link
Contributor

@bdash bdash commented Apr 10, 2024

Apple's new linker reports itself as ld rather than ld64 and does not match the version detection regex.

Invert the logic to look only for older versions of ld64. This ensures the runtime dylibs are left with a linker-generated code signature that tools such as strip will preserve.

[compiler-rt] Don't explicitly ad-hoc code sign dylibs if using Apple…
4a240fc 
…'s new linker

Apple's new linker reports itself as ld rather than ld64 and does not
match the version detection regex. Invert the logic to look only for
older versions of ld64.

This ensures the runtime dylibs are left with a linker-generated code
signature that tools such as `strip` will preserve.
@github-actions GitHub Actions
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@vitalybuka vitalybuka requested review from nico and yln April 10, 2024 21:49
vitalybuka
vitalybuka approved these changes Apr 10, 2024
View reviewed changes
Copy link
Collaborator

@vitalybuka vitalybuka left a comment

Choose a reason for hiding this comment

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

LGTM, but lets wait for @yln

yln
yln approved these changes Apr 11, 2024
View reviewed changes
Copy link
Collaborator

@yln yln left a comment

Choose a reason for hiding this comment

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

Change makes sense to me, thanks!

TIL: linker ad-hoc codesigns by default

man ld
...
     -adhoc_codesign
             Directs the linker to add an ad-hoc codesignature to the output file. The default for Apple Silicon binaries is to be ad-hoc codesigned.

@vitalybuka vitalybuka merged commit 2f55056 into llvm:main Apr 11, 2024
@github-actions GitHub Actions
Copy link

@bdash Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

@cjappl
Copy link
Contributor

cjappl commented May 2, 2024

Just a heads up @bdash , this causes a bug on my system where I can't run check-asan (or any of the other sanitizer checks), because it complains about the dylibs being unsigned.

Reverting the change in this PR fixes the issue.

Showing the bug:

> ninja check-asan
[1/2] Running the AddressSanitizer tests
...
dyld[50801]: Library not loaded: @rpath/libclang_rt.asan_osx_dynamic.dylib
  Referenced from: <10CB825E-7352-3AE1-9B7C-95BE6D9891A1> /Users/topher/code/radsan_cjappl/build/projects/compiler-rt/lib/asan/tests/ARM64DarwinConfig/Asan-arm64-inline-Test
  Reason: tried: '/Users/topher/code/radsan_cjappl/build/./lib/../lib/clang/19/lib/darwin/libclang_rt.asan_osx_dynamic.dylib' (code signature in <683ECD6C-FC26-3C6D-89D4-ADA8558EDFB
0> '/Users/topher/code/radsan_cjappl/build/lib/clang/19/lib/darwin/libclang_rt.asan_osx_dynamic.dylib' not valid for use in process: Trying to load an unsigned library), '/System/Vo
lumes/Preboot/Cryptexes/OS/Users/topher/code/radsan_cjappl/build/./lib/../lib/clang/19/lib/darwin/libclang_rt.asan_osx_dynamic.dylib' (no such file), '/Users/topher/code/radsan_cjap
pl/build/projects/compiler-rt/lib/asan/tests/ARM64DarwinConfig/libclang_rt.asan_osx_dynamic.dylib' (no such file), '/Users/topher/code/radsan_cjappl/build/./lib/../lib/clang/19/lib/
darwin/libclang_rt.asan_osx_dynamic.dylib' (code signature in <683ECD6C-FC26-3C6D-89D4-ADA8558EDFB0> '/Users/topher/code/radsan_cjappl/build/lib/clang/19/lib/darwin/libclang_rt.asan
_osx_dynamic.dylib' not valid for use in process: Trying to load an unsigned library), '/System/Volumes/Preboot/Cryptexes/OS/Users/topher/code/radsan_cjappl/build/./lib/../lib/clang
/19/lib/darwin/libclang_rt.asan_osx_dynamic.dylib' (no such file), '/Users/topher/code/radsan_cjappl/build/projects/compiler-rt/lib/asan/tests/ARM64DarwinConfig/libclang_rt.asan_osx
_dynamic.dylib' (no such file)

Replicating the important line:

'/Users/topher/code/radsan_cjappl/build/lib/clang/19/lib/darwin/libclang_rt.asan
_osx_dynamic.dylib' not valid for use in process: Trying to load an unsigned library)

This file exists, but is unsigned.

Let me know what diagnostic information I can get for you, happy to help track it down :)

Some things to possibly help get things started, logging from cmake:

-- CMAKE_LINKER=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ld
-- LD_V_OUTPUT=@(#)PROGRAM:ld PROJECT:ld-1053.12

Other info:

> clang --version
Apple clang version 15.0.0 (clang-1500.3.9.4)
Target: arm64-apple-darwin23.4.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

Let me know what else you need.

@cjappl cjappl mentioned this pull request May 2, 2024
Closed
@bdash
Copy link
Contributor Author

bdash commented May 3, 2024

I will look into it.

@cjappl
Copy link
Contributor

cjappl commented May 3, 2024 via email

bdash pushed a commit to bdash/llvm-project that referenced this pull request May 9, 2024
[compiler-rt] Don't rely on automatic codesigning with Apple's linker
d72e4f7 
In llvm#88323, I changed the logic
within `add_compiler_rt_runtime` to only explicitly code sign the
resulting library if an older version of Apple's ld64 was in use. This
was based on the assumption that newer versions of ld64 and the new
Apple linker always ad-hoc sign their output binaries. This is true in
most cases, but not when using Apple's new linker with the
'-darwin-target-variant' flag to build Mac binaries that are compatible
with Catalyst.

Rather than adding increasingly complicated logic to detect the exact
scenarios that require explicit code signing, I've opted to always
explicitly code sign when using any Apple linker. We instead detect and
use the 'linker-signed' codesigning option when possible to match the
signatures that the linker would otherwise create. This avoids having
non-'linker-signed' ad-hoc signatures which was the underlying problem that
llvm#88323 intended to address.
@bdash bdash mentioned this pull request May 9, 2024
Merged
yln pushed a commit that referenced this pull request May 30, 2024
@bdash
[compiler-rt] Don't rely on automatic codesigning with Apple's linker (…
815250b 
…#91681)

In #88323, I changed the logic
within `add_compiler_rt_runtime` to only explicitly code sign the
resulting library if an older version of Apple's ld64 was in use. This
was based on the assumption that newer versions of ld64 and the new
Apple linker always ad-hoc sign their output binaries. This is true in
most cases, but not when using Apple's new linker with the
`-darwin-target-variant` flag to build Mac binaries that are compatible
with Catalyst.

Rather than adding increasingly complicated logic to detect the exact
scenarios that require explicit code signing, I've opted to always
explicitly code sign when using any Apple linker. We instead detect and
use the 'linker-signed' codesigning option when possible to match the
signatures that the linker would otherwise create. This avoids having
non-'linker-signed' ad-hoc signatures which was the underlying problem
that #88323 was intended to
address.

Co-authored-by: Mark Rowe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yln yln yln approved these changes

@vitalybuka vitalybuka vitalybuka approved these changes

@nico nico Awaiting requested review from nico

Assignees
No one assigned
Labels
compiler-rt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bdash @cjappl @yln @vitalybuka @llvmbot