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 1 commit 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.

//@ compile-flags: -Z instrument-xray --target x86_64-apple-darwin
//@ compile-flags: -Z instrument-xray --target x86_64-pc-windows-msvc
Copy link
Member

Choose a reason for hiding this comment

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

Question: do we have any smoke test for xray on x86_64-apple-darwin and aarch64-apple-darwin? As in:

  • A smoke test that have some ability to observe xray being enabled or not, for aarch64-apple-darwin and x86_64-apple-darwin targets.
  • A test check that the flag -Z instrument-xray is still unstable, but is no longer rejected on x86_64-apple-darwin and aarch64-apple-darwin respectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think both of these tests don't exist at the moment, I can add tests for this but there are two problems

  1. It seems that only llir is tested in tests/codegen and no asm, which cannot be used to check whether xray is enabled, because xray affects llvm output rather than input. i need some guidance how to write tests for asm output.
  2. Add a test specifically for instrument-xray + x86_64-apple-darwin/aarch64-apple-darwin combination seems a bit strange to me, since this is not a feature we implement, but just an upstream feature. do we need to add such a test for each supported target?

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

5 participants