Skip to content
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

Pass end position of span through inline ASM cookie #129181

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

beetrees
Copy link
Contributor

@beetrees beetrees commented Aug 17, 2024

Before this PR, only the start position of the span was passed though the inline ASM cookie to diagnostics. LLVM 19 has full support for 64-bit inline ASM cookies; this PR uses that to pass the end position of the span in the upper 32 bits, meaning inline ASM diagnostics now point at the entire line the error occurred on, not just the first character of it.

@rustbot
Copy link
Collaborator

rustbot commented Aug 17, 2024

r? @pnkfelix

rustbot has assigned @pnkfelix.
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 Aug 17, 2024
@rust-log-analyzer

This comment has been minimized.

@beetrees beetrees marked this pull request as ready for review August 17, 2024 12:56
@beetrees
Copy link
Contributor Author

Should now be ready. I've set a try-job for LLVM 18 since it's not tested in PR CI if someone could do a @bors try.

@bors
Copy link
Contributor

bors commented Sep 19, 2024

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

@pnkfelix
Copy link
Member

pnkfelix commented Oct 4, 2024

Sorry for the delay in review.

I posted a suggestion for the comments, but its not needed to pass review.

so r=me once this is rebased.

@pnkfelix
Copy link
Member

pnkfelix commented Oct 4, 2024

@bors try

@pnkfelix
Copy link
Member

pnkfelix commented Oct 4, 2024

(or wait, does is a try build done against a merge, which will obviously fail here? Sorry again.)

@bors
Copy link
Contributor

bors commented Oct 4, 2024

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout asm-spans (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self asm-spans --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Auto-merging tests/ui/asm/inline-syntax.x86_64.stderr
CONFLICT (content): Merge conflict in tests/ui/asm/inline-syntax.x86_64.stderr
Auto-merging tests/ui/asm/inline-syntax.rs
CONFLICT (content): Merge conflict in tests/ui/asm/inline-syntax.rs
CONFLICT (modify/delete): tests/ui/asm/inline-syntax.arm_llvm_18.stderr deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of tests/ui/asm/inline-syntax.arm_llvm_18.stderr left in tree.
Auto-merging tests/ui/asm/inline-syntax.arm.stderr
CONFLICT (content): Merge conflict in tests/ui/asm/inline-syntax.arm.stderr
Auto-merging compiler/rustc_span/src/lib.rs
Auto-merging compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp
Auto-merging compiler/rustc_codegen_ssa/src/back/write.rs
Auto-merging compiler/rustc_codegen_llvm/src/llvm/ffi.rs
Auto-merging compiler/rustc_codegen_llvm/src/back/write.rs
CONFLICT (content): Merge conflict in compiler/rustc_codegen_llvm/src/back/write.rs
Auto-merging compiler/rustc_codegen_llvm/src/asm.rs
CONFLICT (content): Merge conflict in compiler/rustc_codegen_llvm/src/asm.rs
Automatic merge failed; fix conflicts and then commit the result.

@pnkfelix
Copy link
Member

pnkfelix commented Oct 4, 2024

@rustbot author

just switching to waiting-on-author for the rebase of the PR.

@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 Oct 4, 2024
@beetrees
Copy link
Contributor Author

Rebased

@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 Oct 12, 2024
@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 16, 2024

📌 Commit 29ae617 has been approved by pnkfelix

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 Oct 16, 2024
Urgau added a commit to Urgau/rust that referenced this pull request Oct 16, 2024
Pass end position of span through inline ASM cookie

Before this PR, only the start position of the span was passed though the inline ASM cookie to diagnostics. LLVM 19 has full support for 64-bit inline ASM cookies; this PR uses that to pass the end position of the span in the upper 32 bits, meaning inline ASM diagnostics now point at the entire line the error occurred on, not just the first character of it.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 16, 2024
Rollup of 10 pull requests

Successful merges:

 - rust-lang#129181 (Pass end position of span through inline ASM cookie)
 - rust-lang#130989 (Don't check unsize goal in MIR validation when opaques remain)
 - rust-lang#131657 (Rustfmt `for<'a> async` correctly)
 - rust-lang#131691 (Delay ambiguous intra-doc link resolution after `Cache` has been populated)
 - rust-lang#131730 (Refactor some `core::fmt` macros)
 - rust-lang#131751 (Rename `can_coerce` to `may_coerce`, and then structurally resolve correctly in the probe)
 - rust-lang#131753 (Unify `secondary_span` and `swap_secondary_and_primary` args in `note_type_err`)
 - rust-lang#131776 (Emscripten: Xfail backtrace ui tests)
 - rust-lang#131777 (Fix trivially_copy_pass_by_ref in stable_mir)
 - rust-lang#131778 (Fix needless_lifetimes in stable_mir)

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

@bors r- rollup=iffy
failed in #131785 (comment) I guess

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 16, 2024
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 16, 2024
@Urgau
Copy link
Member

Urgau commented Oct 16, 2024

Failed in rollup #131785 (comment)
@bors r-

@beetrees
Copy link
Contributor Author

Should be fixed now.

@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 Oct 16, 2024
@bors
Copy link
Contributor

bors commented Nov 4, 2024

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

@compiler-errors
Copy link
Member

@bors r=pnkfelix,compiler-errors

@bors
Copy link
Contributor

bors commented Dec 11, 2024

📌 Commit 68227a3 has been approved by pnkfelix,compiler-errors

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 Dec 11, 2024
@bors
Copy link
Contributor

bors commented Dec 12, 2024

⌛ Testing commit 68227a3 with merge 903d297...

@bors
Copy link
Contributor

bors commented Dec 12, 2024

☀️ Test successful - checks-actions
Approved by: pnkfelix,compiler-errors
Pushing 903d297 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 12, 2024
@bors bors merged commit 903d297 into rust-lang:master Dec 12, 2024
7 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 12, 2024
@beetrees beetrees deleted the asm-spans branch December 12, 2024 05:19
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (903d297): 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 (primary -0.7%, secondary -0.2%)

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)
2.6% [2.6%, 2.6%] 1
Regressions ❌
(secondary)
4.4% [4.4%, 4.4%] 1
Improvements ✅
(primary)
-2.3% [-3.3%, -1.3%] 2
Improvements ✅
(secondary)
-1.8% [-2.1%, -1.5%] 3
All ❌✅ (primary) -0.7% [-3.3%, 2.6%] 3

Cycles

Results (secondary -3.4%)

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)
- - 0
Improvements ✅
(secondary)
-3.4% [-3.9%, -2.9%] 5
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 769.999s -> 769.893s (-0.01%)
Artifact size: 331.03 MiB -> 331.00 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

9 participants