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

Make #[used] work when linking with ld64 #133832

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Dec 4, 2024

To make #[used] work in static libraries, we use the symbols.o trick introduced in #95604.

However, the linker shipped with Xcode, ld64, works a bit differently from other linkers; in particular, it completely ignores undefined symbols by themselves, and only consider them if they have relocations (something something atoms something fixups, I don't know the details).

So to make the symbols.o file work on ld64, we need to actually insert a relocation. That's kinda cumbersome to do though, since the relocation must be valid, and hence must point to a valid piece of machine code, and is hence very architecture-specific.

Fixes #133491, see that for investigation.


Another option would be to pass -u _foo to the final linker invocation. This has the problem that -u causes the linker to not be able to dead-strip the symbol, which is undesirable. (If we did this, we would possibly also want to do it by putting the arguments in a file by itself, and passing that file via @, e.g. @undefined_symbols.txt, similar to #52699, though that is only supported since Xcode 12, and I'm not sure we wanna bump that).

Various other options that are probably all undesirable as they affect link time performance:

  • Pass -all_load to the linker.
  • Pass -ObjC to the linker (the Objective-C support in the linker has different code paths that load more of the binary), and instrument the binaries that contain #[used] symbols.
  • Pass -force_load to libraries that contain #[used] symbols.

Failed attempt: Embed -u _foo in the object file with LC_LINKER_OPTION, akin to #121293. Doesn't work, both because ld64 doesn't read that from archive members unless it already has a reason to load the member (which is what this PR is trying to make it do), and because ld64 only support the -l, -needed-l, -framework and -needed_framework flags in there.


TODO:

  • Support all Apple architectures.
  • Ensure that this works regardless of the actual type of the symbol.
  • Write up more docs.
  • Wire up a few proper tests.

@rustbot label O-apple

@rustbot
Copy link
Collaborator

rustbot commented Dec 4, 2024

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 4, 2024
@rust-log-analyzer

This comment has been minimized.

@madsmtm madsmtm changed the title Make symbols.o trick work when linking with ld64 Make #[used] work when linking with ld64 Dec 30, 2024
@rustbot rustbot added the O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) label Dec 30, 2024
@rust-log-analyzer

This comment has been minimized.

@madsmtm
Copy link
Contributor Author

madsmtm commented Dec 31, 2024

@estebank I'm gonna mark this as ready for an initial review, to make sure that you agree that the approach here is the right one (vs. the alternatives I noted in the PR description / another alternative that I haven't considered). If so, then I'll complete the remaining TODO items.

@madsmtm madsmtm marked this pull request as ready for review December 31, 2024 00:13
@rustbot
Copy link
Collaborator

rustbot commented Dec 31, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@estebank
Copy link
Contributor

I'm sorry. I don't think I'm the best person to review this change.

r? compiler

@rustbot rustbot assigned chenyukang and unassigned estebank Dec 31, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Dec 31, 2024

I'm going to ask in T-compiler since this is likely to bounce based on random rerolls. I'll give it a few days, if we still can't find a suitable reviewer by then, I'll compiler-nominate this PR.

Please ping me next Monday if we still haven't found a suitable review (in case this gets lost in my review queue), I'll nominate it for next week's triage meeting.

@jieyouxu jieyouxu assigned jieyouxu and unassigned chenyukang Dec 31, 2024
@DianQK
Copy link
Member

DianQK commented Dec 31, 2024

Edit: I need more time to check it. I'm not the best person here, but fewer people are using macOS here.

I will try to review it tomorrow.
r? DianQK

@rustbot rustbot assigned DianQK and unassigned jieyouxu Dec 31, 2024
@DianQK
Copy link
Member

DianQK commented Jan 2, 2025

IIUC, I can reproduce this with C:

// foo.c
__attribute__((constructor))
void foo() {}

// main.c
int main() {
    return 0;
}

When I build the foo.c to the libfoo.a, foo.o that in libfoo.a never be loaded by any linker. For fix this, we added a symbols.o to help the linker load such object files.

I'm concerned about the current implementation. It seems we're building our logic around ld64's internal implementation details, and ld64 isn't truly open source. (I haven't looked into ld64's implementation details yet. I'm not sure if this approach is fragile or will be difficult to maintain in the future.)

IIUC, the key here is to have the linker load object files as intended, just like when we directly use ld foo.o. So why don't we just add these object files directly?

If we don't want to do this, how about adding the right value of #[used] to symbols.o? For static PUSH: extern "C" fn() = push;, we add "push" to symbols.o. That looks workable, as long as push always be a global symbol and both PUSH and push stay in the same object file.

@madsmtm
Copy link
Contributor Author

madsmtm commented Jan 2, 2025

[...]
When I build the foo.c to the libfoo.a, foo.o that in libfoo.a never be loaded by any linker. For fix this, we added a symbols.o to help the linker load such object files.

That's correct.

I'm concerned about the current implementation. It seems we're building our logic around ld64's internal implementation details, and ld64 isn't truly open source. (I haven't looked into ld64's implementation details yet. I'm not sure if this approach is fragile or will be difficult to maintain in the future.)

I agree that it is fragile, but I don't think it is as bad as it looks; consider that linkers are mostly backwards compatible, and that it works now (whereas before it just didn't work at all, in no versions of ld64). I honestly don't think Apple is gonna change how this works, and if they do, we'll have the Xcode betas to fix it.

The only thing I can conceivably think of that might break is:

  • If symbols were treated differently depending on their type (function vs. static).
  • If the linker figures out that the data that the relocation refers to is never actually used.

I guess if we wanted to make it even more robust, we'd do something like:

// symbols.rs
extern crate crate1;
extern crate crate2;
extern crate crate3;

fn _used_symbols() {
    let _static1 = crate1::STATIC1;
    let _static2 = crate2::STATIC2;
    let _fn3 = crate3::fn3;
}

// rustc symbols.rs --emit=obj

That is, emit a label that refers to the block of code that touches all symbols, such that the linker cannot assume the section to be unused. The roughly equivalent could be done with object-rs like so:

file.add_symbol(write::Symbol {
    name: "_used_symbols".into(),
    value: 0,
    size: 0,
    kind: SymbolKind::Text,
    scope: SymbolScope::Dynamic,
    weak: false,
    section: write::SymbolSection::Section(section_id),
    flags: SymbolFlags::None,
});

Was that clear? I can try to go in more detail here if you want? Or try to conjure up some contrived assembly code, and consider how ld64 would have to link that now and in the future?

IIUC, the key here is to have the linker load object files as intended, just like when we directly use ld foo.o. So why don't we just add these object files directly?

I can see two ways to do that:

  • Avoid archives altogether, just pass each object file in every crate to the linker (or maybe combine the different object files generated by each codegen unit into a "libfoo.o").
  • Link just the object files that use #[used]. Requires somehow extracting the relevant object files from the archive at link time, or maybe doing it while building the crate itself? E.g. create libfoo.a and foo_used_symbols.o.

The former is bad for link-time performance (the linker can skip a lot of work for archives that if can't for object files), and the latter is either also bad for perf, or makes the integration between rustc and Cargo even more complex than it already is.

If we don't want to do this, how about adding the right value of #[used] to symbols.o? For static PUSH: extern "C" fn() = push;, we add "push" to symbols.o. That looks workable, as long as push always be a global symbol and both PUSH and push stay in the same object file.

Hmm, not sure I understand?

Note that we'd still want e.g. #[used] #[link_section = "..."] static FOO: i32 = 2; to be seen by the linker, and here there's no "inner" symbol to refer to?

@bjorn3
Copy link
Member

bjorn3 commented Jan 2, 2025

If the linker figures out that the data that the relocation refers to is never actually used.

The linker would have still pulled in the object file by that time. And #[used] doesn't actually have to prevent linker GC of the section. Only make sure that it isn't discarded before the linker has a chance of see the symbol and make it's own decision about whether it is considered to be a root for the linker GC. The unstable #[used(linker)] also forces the linker to keep the symbol, but that emits a separate flag on the symbol to prevent it from being deleted by the linker when the linker actually sees it.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 18, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2025
Make `#[used]` work when linking with `ld64`

To make `#[used]` work in static libraries, we use the `symbols.o` trick introduced in rust-lang#95604.

However, the linker shipped with Xcode, ld64, works a bit differently from other linkers; in particular, [it completely ignores undefined symbols by themselves](https://github.com/apple-oss-distributions/ld64/blob/ld64-954.16/src/ld/parsers/macho_relocatable_file.cpp#L2455-L2468), and only consider them if they have relocations (something something atoms something fixups, I don't know the details).

So to make the `symbols.o` file work on ld64, we need to actually insert a relocation. That's kinda cumbersome to do though, since the relocation must be valid, and hence must point to a valid piece of machine code, and is hence very architecture-specific.

Fixes rust-lang#133491, see that for investigation.

---

Another option would be to pass `-u _foo` to the final linker invocation. This has the problem that `-u` causes the linker to not be able to dead-strip the symbol, which is undesirable. (If we did this, we would possibly also want to do it by putting the arguments in a file by itself, and passing that file via ``@`,` e.g. ``@undefined_symbols.txt`,` similar to rust-lang#52699, though that [is only supported since Xcode 12](https://developer.apple.com/documentation/xcode-release-notes/xcode-12-release-notes#Linking), and I'm not sure we wanna bump that).

Various other options that are probably all undesirable as they affect link time performance:
- Pass `-all_load` to the linker.
- Pass `-ObjC` to the linker (the Objective-C support in the linker has different code paths that load more of the binary), and instrument the binaries that contain `#[used]` symbols.
- Pass `-force_load` to libraries that contain `#[used]` symbols.

Failed attempt: Embed `-u _foo` in the object file with `LC_LINKER_OPTION`, akin to rust-lang#121293. Doesn't work, both because `ld64` doesn't read that from archive members unless it already has a reason to load the member (which is what this PR is trying to make it do), and because `ld64` only support the `-l`, `-needed-l`, `-framework` and `-needed_framework` flags in there.

---

TODO:
- [x] Support all Apple architectures.
- [x] Ensure that this works regardless of the actual type of the symbol.
- [x] Write up more docs.
- [x] Wire up a few proper tests.

`@rustbot` label O-apple
@bors
Copy link
Contributor

bors commented Feb 18, 2025

⌛ Testing commit 5605039 with merge 3b73000...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 18, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 18, 2025
@DianQK
Copy link
Member

DianQK commented Feb 22, 2025

Note that we'd still want e.g. #[used] #[link_section = "..."] static FOO: i32 = 2; to be seen by the linker, and here there's no "inner" symbol to refer to?

Ah, yeah! This also reminder me of something. Consider the following C code:

__attribute__((constructor)) static void push() {}

or

static void push() {}

#if defined(__linux__)
__attribute__((section(".init_array")))
#elif defined(__APPLE__)
__attribute__((section("__DATA,__mod_init_func,mod_init_funcs")))
#endif
static void (*const PUSH)(void) = push;

There are no global symbols here. I think rustc should not modify local symbols into global symbols through #[used] - in this case, it's the same situation as the C code above, where an object file has no global symbols.

IMO, loading object files through a "symbols.o" doesn't seem like a good idea. Looking at the longer term, I don't think we should continue to go this way.

Looking at this PR itself, I don't think we need to focus on ld64's internal implementation, since we're trying to simulate a standard object file. Based on my newly added comments, I think this approach is still quite complicated, and we need more workarounds to resolve it. What do you think?

I've put a pull request that describes my proposal. #137426

@madsmtm
Copy link
Contributor Author

madsmtm commented Feb 24, 2025

It's unclear to me why the test failed, but I've skipped it on Windows for now, it's not the platform whose problems I'm trying to resolve here.

EDIT: Nvmd, I figured it out, it was because I was using printf directly. Doing a test run of x86_64-msvc-1 in https://github.com/rust-lang/rust/actions/runs/13489994880/job/37686636198.

@madsmtm madsmtm requested a review from DianQK February 24, 2025 02:47
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Feb 24, 2025
@DianQK DianQK removed A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Feb 24, 2025
@madsmtm
Copy link
Contributor Author

madsmtm commented Feb 24, 2025

The test run for x86_64-msvc-1 completed successfully.

@DianQK
Copy link
Member

DianQK commented Feb 24, 2025

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Feb 24, 2025

📌 Commit b202430 has been approved by DianQK

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 24, 2025
@bors
Copy link
Contributor

bors commented Feb 24, 2025

⌛ Testing commit b202430 with merge 2ad1dfb...

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 24, 2025
Make `#[used]` work when linking with `ld64`

To make `#[used]` work in static libraries, we use the `symbols.o` trick introduced in rust-lang#95604.

However, the linker shipped with Xcode, ld64, works a bit differently from other linkers; in particular, [it completely ignores undefined symbols by themselves](https://github.com/apple-oss-distributions/ld64/blob/ld64-954.16/src/ld/parsers/macho_relocatable_file.cpp#L2455-L2468), and only consider them if they have relocations (something something atoms something fixups, I don't know the details).

So to make the `symbols.o` file work on ld64, we need to actually insert a relocation. That's kinda cumbersome to do though, since the relocation must be valid, and hence must point to a valid piece of machine code, and is hence very architecture-specific.

Fixes rust-lang#133491, see that for investigation.

---

Another option would be to pass `-u _foo` to the final linker invocation. This has the problem that `-u` causes the linker to not be able to dead-strip the symbol, which is undesirable. (If we did this, we would possibly also want to do it by putting the arguments in a file by itself, and passing that file via ``@`,` e.g. ``@undefined_symbols.txt`,` similar to rust-lang#52699, though that [is only supported since Xcode 12](https://developer.apple.com/documentation/xcode-release-notes/xcode-12-release-notes#Linking), and I'm not sure we wanna bump that).

Various other options that are probably all undesirable as they affect link time performance:
- Pass `-all_load` to the linker.
- Pass `-ObjC` to the linker (the Objective-C support in the linker has different code paths that load more of the binary), and instrument the binaries that contain `#[used]` symbols.
- Pass `-force_load` to libraries that contain `#[used]` symbols.

Failed attempt: Embed `-u _foo` in the object file with `LC_LINKER_OPTION`, akin to rust-lang#121293. Doesn't work, both because `ld64` doesn't read that from archive members unless it already has a reason to load the member (which is what this PR is trying to make it do), and because `ld64` only support the `-l`, `-needed-l`, `-framework` and `-needed_framework` flags in there.

---

TODO:
- [x] Support all Apple architectures.
- [x] Ensure that this works regardless of the actual type of the symbol.
- [x] Write up more docs.
- [x] Wire up a few proper tests.

`@rustbot` label O-apple
@bors
Copy link
Contributor

bors commented Feb 24, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 24, 2025
@rust-log-analyzer
Copy link
Collaborator

The job dist-x86_64-mingw failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] unicode_ident test:false 0.245
error: could not compile `unicode-ident` (lib)

Caused by:
  process didn't exit successfully: `D:\a\rust\rust\build\bootstrap\debug\rustc D:\a\rust\rust\build\bootstrap\debug\rustc --crate-name unicode_ident --edition=2018 C:\Users\runneradmin\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\unicode-ident-1.0.16\src\lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no -C debug-assertions=off --check-cfg cfg(docsrs,test) --check-cfg "cfg(feature, values())" -C metadata=a163a2dc94512ad5 -C extra-filename=-9e92e9f68ebc7032 --out-dir D:\a\rust\rust\build\x86_64-pc-windows-gnu\stage0-bootstrap-tools\release\deps -L dependency=D:\a\rust\rust\build\x86_64-pc-windows-gnu\stage0-bootstrap-tools\release\deps --cap-lints allow -Z binary-dep-depinfo` (exit code: 0xc00000fd, STATUS_STACK_OVERFLOW)
[RUSTC-TIMING] byteorder test:false 0.247
[RUSTC-TIMING] once_cell test:false 0.310
[RUSTC-TIMING] windows_sys test:false 2.850
Build completed unsuccessfully in 0:01:25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries A-run-make Area: port run-make Makefiles to rmake.rs O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macOS: Crate symbols get discarded when crate appears unused
10 participants