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.

Sign up for GitHub

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
Merged

Fix spans in LLVM-generated inline asm errors #110985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions compiler/rustc_codegen_ssa/src/back/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1821,9 +1821,15 @@ impl SharedEmitterMain {
let source = sess
.source_map()
.new_source_file(FileName::inline_asm_source_code(&buffer), buffer);
let source_span = Span::with_root_ctxt(source.start_pos, source.end_pos);
let spans: Vec<_> =
spans.iter().map(|sp| source_span.from_inner(*sp)).collect();
let spans: Vec<_> = spans
.iter()
.map(|sp| {
Span::with_root_ctxt(
source.normalized_byte_pos(sp.start as u32),
source.normalized_byte_pos(sp.end as u32),
)
})
.collect();
err.span_note(spans, "instantiated into assembly here");
}

Expand Down
22 changes: 22 additions & 0 deletions compiler/rustc_span/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1745,6 +1745,28 @@ 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.
///
/// 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.
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.

let diff = match self
.normalized_pos
.binary_search_by(|np| (np.pos.0 + np.diff).cmp(&(self.start_pos.0 + offset)))
{
Ok(i) => self.normalized_pos[i].diff,
Err(i) if i == 0 => 0,
Err(i) => self.normalized_pos[i - 1].diff,
};

BytePos::from_u32(self.start_pos.0 + offset - diff)
}

/// Converts an absolute `BytePos` to a `CharPos` relative to the `SourceFile`.
pub fn bytepos_to_file_charpos(&self, bpos: BytePos) -> CharPos {
// The number of extra bytes due to multibyte chars in the `SourceFile`.
Expand Down