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

Fix spans in LLVM-generated inline asm errors #110985

Merged
merged 1 commit into from
May 6, 2023

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Apr 29, 2023

Previously, incorrect spans were reported if inline assembly contained CRLF (Windows) line endings.

Fixes #110885

@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@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 Apr 29, 2023
@Amanieu
Copy link
Member Author

Amanieu commented Apr 29, 2023

I'm having trouble making a test for this since git doesn't preserve CRLF line endings.

@@ -1745,6 +1745,21 @@ impl SourceFile {
BytePos::from_u32(pos.0 - self.start_pos.0 + diff)
}

/// Calculates a normalized byte position from a byte offset relative to the
/// start of the file.
pub fn normalized_byte_pos(&self, offset: u32) -> BytePos {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really familiar with this code. I don't know what normalization in this context is, so I don't really understand what this function does. I also find it kind of surprising that functionality like this hasn't existed before and you need to implement it yourself to adjust some spans.

Feel free to assign to somebody else, if you don't have the time to explain what normalization is in this context (I don't have the time to read up on it unfortunately).

Copy link
Member Author

Choose a reason for hiding this comment

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

Normalization refers to this function which removes the unicode BOM and converts \n\r newlines to \r when a new SourceFile is added.

When we get an inline assembler error from LLVM during codegen, we import the expanded assembly code as a new SourceFile, which can then be used for error reporting with spans. However the byte offsets given to us by LLVM are relative to the start of the original buffer, not the normalized one. Hence we need to convert those offsets to the normalized form when constructing spans.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, makes sense to me now. Can you maybe add the second part of your comment to the docs for that function?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added the comment.

Previously, incorrect spans were reported if inline assembly contained
CRLF (Windows) line endings.

Fixes rust-lang#110885
@Amanieu Amanieu force-pushed the normalize_asm_spans branch from 07b33b6 to bba2a1e Compare May 6, 2023 08:32
@b-naber
Copy link
Contributor

b-naber commented May 6, 2023

Thanks. @bors r+ rollup

@bors
Copy link
Contributor

bors commented May 6, 2023

📌 Commit bba2a1e has been approved by b-naber

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 6, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 6, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#110577 (Use fulfillment to check `Drop` impl compatibility)
 - rust-lang#110610 (Add Terminator conversion from MIR to SMIR, part #1)
 - rust-lang#110985 (Fix spans in LLVM-generated inline asm errors)
 - rust-lang#110989 (Make the BUG_REPORT_URL configurable by tools )
 - rust-lang#111167 (debuginfo: split method declaration and definition)
 - rust-lang#111230 (add hint for =< as <=)
 - rust-lang#111279 (More robust debug assertions for `Instance::resolve` on built-in traits with non-standard trait items)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8172ada into rust-lang:master May 6, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

ICE: assertion failed: bpos.to_u32() >= mbc.pos.to_u32() + mbc.bytes as u32
4 participants