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

Comment on lines 4 to 5
//@ rustc-env:RUSTC_BOOTSTRAP=-1
//@ compile-flags: -Z instrument-xray --target x86_64-unknown-linux-gnu
Copy link
Member

@jieyouxu jieyouxu May 19, 2025

Choose a reason for hiding this comment

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

Suggestion: What I meant was you can gate this test behind nightly-channel only (//@ only-nightly), so the unstable flag is accepted by an unstable compiler (i.e. -Z instrument-xray is accepted) but rejected because it's not supported for the given platform. You can then check for the target-platform-gate message:

//~? ERROR XRay instrumentation is not supported for this target

You can also introduce a differential revision to check that this is accepted, however, on x86_64-apple-darwin and aarch64-apple-darwin: e.g.

//@ only-nightly (flag is still unstable)

//@ revisions: unsupported
//@[unsupported] compile-flags: -Z instrument-xray --target=x86_64-pc-windows-msvc

//@ revisions: x86_64_darwin
//@[x86_64_darwin] compile-flags: -Z instrument-xray --target=x86_64-apple-darwin
//@[x86_64_darwin] check-pass

//@ revisions: aarch64_darwin
//@[aarch64_darwin] compile-flags: -Z instrument-xray --target=aarch64-apple-darwin
//@[aarch64_darwin] check-pass

So i.e. this test would be platform-support.rs.

Comment on lines 7 to 9
#![feature(no_core)]
#![no_core]
#![no_main]
Copy link
Member

Choose a reason for hiding this comment

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

Question: can this just be fn main() {}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I just copy this from tests/ui/instrument-xray/target-not-supported.rs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not seem to work, which would cause the test to fail on linux. (because darwin std cannot be built).

If we are just test flag, using no_main seems reasonable to me.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, yeah that's fine.

@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
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-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.

6 participants