Add `-Z instrument-xray` flag #102963

ilammy · 2022-10-12T12:36:27Z

Implement MCP rust-lang/compiler-team#561, adding -Z instrument-xray flag which enables XRay instrumentation in LLVM.

rust-highfive · 2022-10-12T12:36:30Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @compiler-errors (or someone else) soon.

Please see the contribution instructions for more information.

rust-highfive · 2022-10-12T12:36:31Z

⚠️ Warning ⚠️

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

compiler-errors · 2022-10-13T03:37:06Z

I'm not exactly qualified to review this, re-rolling!

r? compiler

ilammy · 2022-10-14T09:48:39Z

Whew, that's surely a bunch of test that I was not aware of. Now should be all green. I gotta read more about x.py

rustbot · 2022-10-14T09:48:47Z

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

petrochenkov · 2022-10-14T16:04:05Z

I did a preliminary review, this looks fine, but I'll mark this as waiting on compiler team since the MCP is still open.

bors · 2022-10-21T16:16:31Z

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

Nilstrieb · 2022-10-23T11:48:07Z

Hi, I've seen you changed some diagnostic structs in your PR. After #103345, the way we refer to fluent messages changed. They are now in a flat namespace with the same identifier as in the fluent file. For example, parser::cool_thing is now parser_cool_thing and parser::suggestion just suggestion.
You should rebase to the latest master and change your fluent message references as described above. Thanks!

rustbot · 2022-10-29T10:55:42Z

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

bors · 2022-11-10T02:44:35Z

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

apiraino · 2022-12-15T17:19:15Z

Is the PR review status updated? Is it waiting for its MCP to be seconded (as per comment cc @petrochenkov )? Thanks for an update.

ilammy · 2022-12-16T03:21:11Z

@apiraino, MCP is indeed still waiting to be seconded.

Recent updates to PR were only resolving merge conflicts. No substantial changes since @petrochenkov's review.

ilammy · 2023-02-05T02:31:51Z

Rebased once again to resolve conflicts.

MCP has been accepted, unblocking this PR.

cc @petrochenkov, @estebank, @apiraino

bors · 2023-02-05T14:42:54Z

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

estebank · 2023-02-06T12:49:44Z

r=me after rebasing once more

Recognize all bells and whistles that LLVM's XRay pass is capable of. The always/never settings are a bit dumb without attributes but they're still there. The default instruction count is chosen by the compiler, not LLVM pass. We'll do it later.

I'm tired of testing it manually, just codify my expectations in tests. They're pretty low-maintenance.

Four because that's the new reasonable maximum for XRay instrumentation attributes in the following commit.

Add the attributes to functions according to the settings. "xray-always" overrides "xray-never", and they both override "xray-ignore-loops" and "xray-instruction-threshold", but we'll let lints deal with warnings about silly attribute combinations.

Let's add at least some tests to verify that this option is accepted and produces expected LLVM attributes. More tests can be added later with attribute support.

Specify where XRay is supported. I only test ARM64 and x86_64, but hey those others should work too, right? LLVM documentation says that MIPS and PPC are also supported, but I don't have the hardware, so I won't pretend. Naturally, more targets can be added later with more testing.

This is somewhat important because LLVM enables the pass based on target architecture, but support by the target OS also matters. For example, XRay attributes are processed by codegen for macOS targets, but Apple linker fails to process relocations in XRay data sections, so the feature as a whole is not supported there for the time being.

Now that the compiler accepts "-Z instrument-xray" option only when targeting one of the supported targets, make sure to not run the codegen tests where the compiler will fail. Like with other compiletests, we don't have access to internals, so simply hardcode a list of supported architectures here.

ilammy · 2023-02-09T03:55:59Z

Once again, resolved merge conflicts.

pnkfelix · 2023-02-09T15:18:28Z

@bors r=estebank

bors · 2023-02-09T15:18:29Z

📌 Commit 54b26f4 has been approved by estebank

It is now in the queue for this repository.

bors · 2023-02-10T00:02:46Z

⌛ Testing commit 54b26f4 with merge a12d31d...

bors · 2023-02-10T03:21:32Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing a12d31d to master...

rust-timer · 2023-02-10T05:53:13Z

Finished benchmarking commit (a12d31d): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.2%	[-3.2%, -3.2%]	1
Improvements ✅ (secondary)	-2.8%	[-3.2%, -1.5%]	15
All ❌✅ (primary)	-3.2%	[-3.2%, -3.2%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

rust-highfive assigned compiler-errors Oct 12, 2022

rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 12, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2022

ilammy mentioned this pull request Oct 12, 2022

Tracking issue for -Z instrument-xray #102921

Open

3 tasks

This comment has been minimized.

Sign in to view

rust-highfive assigned petrochenkov and unassigned compiler-errors Oct 13, 2022

This comment has been minimized.

Sign in to view

ilammy force-pushed the xray-basic branch from f337600 to 0e332c7 Compare October 14, 2022 01:01

This comment has been minimized.

Sign in to view

ilammy force-pushed the xray-basic branch from 3d4775c to 248f154 Compare October 14, 2022 09:47

ilammy marked this pull request as ready for review October 14, 2022 09:48

petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2022

ilammy force-pushed the xray-basic branch from 248f154 to 07adec6 Compare October 29, 2022 10:55

rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Oct 29, 2022

ilammy force-pushed the xray-basic branch from 07adec6 to bcc7c76 Compare December 4, 2022 10:59

ilammy added 8 commits February 9, 2023 12:25

UI tests for -Z instrument-xray

d748f08

I'm tired of testing it manually, just codify my expectations in tests. They're pretty low-maintenance.

Allow multiple instrumentation attributes

b3cadd2

Four because that's the new reasonable maximum for XRay instrumentation attributes in the following commit.

Codegen tests for -Z instrument-xray

0fef658

Let's add at least some tests to verify that this option is accepted and produces expected LLVM attributes. More tests can be added later with attribute support.

ilammy force-pushed the xray-basic branch from e5eb914 to 54b26f4 Compare February 9, 2023 03:54

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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 9, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 10, 2023

bors merged commit a12d31d into rust-lang:master Feb 10, 2023

rustbot added this to the 1.69.0 milestone Feb 10, 2023

This was referenced Feb 10, 2023

Update the minimum external LLVM to 14 #107573

Merged

Resolve documentation links in rustc and store the results in metadata #94857

Merged

Implement -Zlink-directives=yes/no #107675

Merged

ilammy deleted the xray-basic branch February 12, 2023 08:16

workingjubilee added the A-cli Area: Command line interface to the compiler. label Mar 5, 2023

matthiaskrgr mentioned this pull request Apr 20, 2024

ICE: expected principal trait object #124092

Open

matthiaskrgr mentioned this pull request May 6, 2024

ICE: used byval ABI for unsized layout #124806

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `-Z instrument-xray` flag #102963

Add `-Z instrument-xray` flag #102963

ilammy commented Oct 12, 2022

rust-highfive commented Oct 12, 2022

rust-highfive commented Oct 12, 2022

This comment has been minimized.

compiler-errors commented Oct 13, 2022

This comment has been minimized.

This comment has been minimized.

ilammy commented Oct 14, 2022

rustbot commented Oct 14, 2022

petrochenkov commented Oct 14, 2022

bors commented Oct 21, 2022

Nilstrieb commented Oct 23, 2022

rustbot commented Oct 29, 2022

bors commented Nov 10, 2022

apiraino commented Dec 15, 2022

ilammy commented Dec 16, 2022

ilammy commented Feb 5, 2023

bors commented Feb 5, 2023

estebank commented Feb 6, 2023

ilammy commented Feb 9, 2023

pnkfelix commented Feb 9, 2023

bors commented Feb 9, 2023

bors commented Feb 10, 2023

bors commented Feb 10, 2023

rust-timer commented Feb 10, 2023

Add -Z instrument-xray flag #102963

Add -Z instrument-xray flag #102963

Conversation

ilammy commented Oct 12, 2022

rust-highfive commented Oct 12, 2022

rust-highfive commented Oct 12, 2022

This comment has been minimized.

compiler-errors commented Oct 13, 2022

This comment has been minimized.

This comment has been minimized.

ilammy commented Oct 14, 2022

rustbot commented Oct 14, 2022

petrochenkov commented Oct 14, 2022

bors commented Oct 21, 2022

Nilstrieb commented Oct 23, 2022

rustbot commented Oct 29, 2022

bors commented Nov 10, 2022

apiraino commented Dec 15, 2022

ilammy commented Dec 16, 2022

ilammy commented Feb 5, 2023

bors commented Feb 5, 2023

estebank commented Feb 6, 2023

ilammy commented Feb 9, 2023

pnkfelix commented Feb 9, 2023

bors commented Feb 9, 2023

bors commented Feb 10, 2023

bors commented Feb 10, 2023

rust-timer commented Feb 10, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Add `-Z instrument-xray` flag #102963

Add `-Z instrument-xray` flag #102963