Skip to content

Enable xray support for Mac #140790

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

quininer
Copy link
Contributor

@quininer quininer commented May 8, 2025

#102921

Upstream has supported Mac for a while, let's enable it.

I've tested it on M4 and it generates nop sled correctly.

@rustbot
Copy link
Collaborator

rustbot commented May 8, 2025

r? @jieyouxu

rustbot has assigned @jieyouxu.
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 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 May 8, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 8, 2025

These commits modify compiler targets.
(See the Target Tier Policy.)

@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

jieyouxu commented May 8, 2025

Since this affects the Tier 1 {x86_64,aarch64}-apple-darwin targets (albeit -Z instrument-xray is still unstable), asking Apple experts for second opinion as I neither have access to apple envs nor am I aware of how well LLVM xray currently works on those targets.

@rustbot ping apple

@rustbot rustbot added the O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) label May 8, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 8, 2025

Hey Apple notification group! This issue or PR could use some Apple-specific
guidance. Could one of you weigh in? Thanks <3

(In case it's useful, here are some instructions for tackling these sorts of
issues).

cc @BlackHoleFox @hkratz @inflation @madsmtm @nvzqz @shepmaster @thomcc

@jieyouxu
Copy link
Member

jieyouxu commented May 9, 2025

@rustbot author (test nits)

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 9, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@quininer quininer requested review from wesleywiser and jieyouxu May 14, 2025 04:08
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 14, 2025
@jieyouxu
Copy link
Member

Responded to your questions in #140790 (comment).
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2025
@quininer
Copy link
Contributor Author

I added tests for assembly output, which should be seen as supplementing the previous missing tests for xray.

I also added flag stable test, but I still don't think it's necessary. I don't see any other flags have this test, I can only get this from https://github.com/rust-lang/rust/blob/1.87.0/tests/ui/bootstrap/rustc_bootstrap.rs#L36 .
and this test has nothing to do with xray, but is a test of the order of -Z flag and target checks.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 19, 2025
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, just a nit on the platform support test

@jieyouxu
Copy link
Member

I also added flag stable test, but I still don't think it's necessary. I don't see any other flags have this test, I can only get this from https://github.com/rust-lang/rust/blob/1.87.0/tests/ui/bootstrap/rustc_bootstrap.rs#L36 .
and this test has nothing to do with xray, but is a test of the order of -Z flag and target checks.

Sorry I shouldn't have said "platform stability", I should've said "platform support".

@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2025
@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 19, 2025
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.

As someone with very little experience with XRay, I tried to test this end-to-end:

$ ./x build --set=build.sanitizers=true
$ echo "fn main() {}" > main.rs
$ rustc +stage1 -Zinstrument-xray=always main.rs
$ objdump -h -j xray_instr_map main # Works
$ XRAY_OPTIONS="patch_premain=true xray_mode=xray-basic verbosity=1" ./main # No-op?

But it doesn't seem to do anything? Probably because I'm not linking the runtime, but it's not clear to me how I'd go about doing that? Does bootstrap need to build it?

@quininer
Copy link
Contributor Author

@madsmtm As with linux, you need to link to compiler-rt/xray. like https://crates.io/crates/clang-rt-xray

Anyway I don't really care about that, because I'll be using my own rt, like uftrace. So just rustc generate the correct nop sled and xray_instr_map section will good.

@jieyouxu
Copy link
Member

jieyouxu commented May 19, 2025

As with linux, you need to link to compiler-rt/xray. like crates.io/crates/clang-rt-xray

Anyway I don't really care about that, because I'll be using my own rt, like uftrace. So just rustc generate the correct nop sled and xray_instr_map section will good.

Could you add that as a remark on the unstable docs for this flag? I imagine Mads won't be the only person asking that question.

@quininer
Copy link
Contributor Author

Could you add that as a remark on the unstable docs for this flag?

@jieyouxu I think it is already mentioned in unstable book. https://github.com/rust-lang/rust/blob/1.87.0/src/doc/unstable-book/src/compiler-flags/instrument-xray.md?plain=1#L35

@rust-log-analyzer

This comment has been minimized.

@quininer
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 20, 2025
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks

@jieyouxu
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented May 22, 2025

📌 Commit 4a619fb has been approved by jieyouxu

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 May 22, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 23, 2025
bors added a commit that referenced this pull request May 24, 2025
Rollup of 8 pull requests

Successful merges:

 - #140790 (Enable xray support for Mac)
 - #141405 (GetUserProfileDirectoryW is now documented to always store the size)
 - #141413 (Make #[cfg(version)] respect RUSTC_OVERRIDE_VERSION_STRING)
 - #141427 (Disable `triagebot`'s `glacier` handler)
 - #141429 (Dont walk into unsafe binders when emiting error for non-structural type match)
 - #141438 (Do not try to confirm non-dyn compatible method)
 - #141444 (Improve CONTRIBUTING.md grammar and clarity)
 - #141446 (Add 2nd Solaris target maintainer)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors r- #141473 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

8 participants