Skip to content

Conversation

jbatez
Copy link

@jbatez jbatez commented Aug 20, 2025

Tracking issue: #145496

This feature makes it possible to reference Objective-C classes and selectors using the same ABI used by native Objective-C on Apple/Darwin platforms. Without it, Rust code interacting with Objective-C must resort to loading classes and selectors using costly string-based lookups at runtime. With it, these references can be loaded efficiently at dynamic load time.

r? @tmandry

@rustbot
Copy link
Collaborator

rustbot commented Aug 20, 2025

r? @lcnr

rustbot has assigned @lcnr.
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-attributes Area: Attributes (`#[…]`, `#![…]`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-tidy Area: The tidy tool 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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 20, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 20, 2025

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann

@rustbot rustbot assigned tmandry and unassigned lcnr Aug 20, 2025
@jbatez jbatez mentioned this pull request Aug 16, 2025
5 tasks
@rust-log-analyzer

This comment has been minimized.

@jdonszelmann
Copy link
Contributor

I looked through the attribute changes and they look good to me. I can't judge the rest. @tmandry feel free to approve in both our names once you agree with the rest :)

Copy link
Contributor

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Looked through it, it seems to match what I tried to do with the objc2 macros those years back (which is not to say that it is correct, I had no idea what I was doing back then).

Also, it would be helpful with more comments, and a few codegen tests - I get why you haven't done that yet, it's bothersome while we're still somewhat discussing what the best approach is.


Note: I'm still a bit unsure that this is actually the correct approach going forward, I suspect there might be more value in the future from something like the define_in_every_cgu_used you first proposed?
At the very least we'll also need some way to get protocol references, we might also need metaclass references, and possibly more if we're to support static NSString and support fully statically declared classes.

But I think it's fine to move forwards with this in the current state, then we can always figure that sort of stuff out well before stabilisation.

@jbatez
Copy link
Author

jbatez commented Aug 21, 2025

... At the very least we'll also need some way to get protocol references, we might also need metaclass references, and possibly more if we're to support static NSString and support fully statically declared classes.

But I think it's fine to move forwards with this in the current state, then we can always figure that sort of stuff out well before stabilisation.

NSString constants were next on my list and should be pretty straightforward. I haven't looked into protocols or metaclasses yet, but I did take a quick peek at static class definitions and, wow, that's a lot more complicated; I'm not personally planning to tackle that any time soon.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot

This comment has been minimized.

@jbatez
Copy link
Author

jbatez commented Aug 21, 2025

I added tests/codegen-llvm/darwin-objc.rs. In the process, I discovered, at some point, OBJC_CLASSLIST_REFERENCES_$_ and OBJC_SELECTOR_REFERENCES_ were getting optimized away and their initial values were getting used directly. I'm pretty sure this wasn't the case when I originally wrote this months ago. Adding them to llvm.compiler.used (which Clang does as well, fwiw) solved the problem. In the process, I had to convert compiler_used_statics to a RefCell, but I think that's fine.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Aug 22, 2025

☔ The latest upstream changes (presumably #145728) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Aug 23, 2025

☔ The latest upstream changes (presumably #145773) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot

This comment has been minimized.

@jbatez
Copy link
Author

jbatez commented Aug 23, 2025

@tmandry @madsmtm There aren't any more outstanding issues that I'm aware of.

@rustbot

This comment has been minimized.

Copy link
Contributor

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Gave it another review after I got around to actually building and playing with it.

The RefCell on compiler_used_statics is fine.

Also, if you could update the PR description to motivate the feature that'd be nice as well (it'll be recorded in Git history).

View changes since this review

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Aug 29, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@jbatez
Copy link
Author

jbatez commented Aug 30, 2025

I added a lot of tests. Let me know if there's any other tests you'd like to see.

I took a peek at adding invariant.load at the load sites, but it looked like to me that the places where we generate the load instruction are pretty far abstracted from having any metadata about the static that we're loading from. And there are many different code paths that lead to these, so plumbing that through looks like more trouble than it's worth at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-tidy Area: The tidy tool 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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants