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

Bad MIR spans #99854

Open
Noratrieb opened this issue Jul 28, 2022 · 6 comments · Fixed by #99908
Open

Bad MIR spans #99854

Noratrieb opened this issue Jul 28, 2022 · 6 comments · Fixed by #99908
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Noratrieb
Copy link
Member

In #99780 (comment), I tried to add an assertion in the span formatting function to validate that the span for the mir::Body.span span always contains all spans of MIR statements/terminators.

diff --git a/compiler/rustc_span/src/source_map.rs b/compiler/rustc_span/src/source_map.rs
index 28381157d50..ad44a0366b5 100644
--- a/compiler/rustc_span/src/source_map.rs
+++ b/compiler/rustc_span/src/source_map.rs
@@ -477,6 +477,8 @@ pub fn span_to_relative_line_string(&self, sp: Span, relative_to: Span) -> Strin
             return self.span_to_embeddable_string(sp);
         }

+        assert!(relative_to.contains(sp));
+
         let lo_line = lo.line.saturating_sub(offset.line);
         let hi_line = hi.line.saturating_sub(offset.line);

But this assertion failed for many tests. One such case was for associated constants.

We should investigate all the cases where the MIR body span is not fully correct, and add the assertion back in.

@bjorn3
Copy link
Member

bjorn3 commented Jul 28, 2022

This is trivially false in the face of macros and MIR function inlining, right?

#[inline(always)]
fn inline_me() -> u32 {
    42
}

pub fn root() -> u32 {
    inline_me()
}

when compiled with nightly and optimizations enabled gives the following MIR:

fn root() -> u32 {
    let mut _0: u32;                     // return place in scope 0 at src/lib.rs:6:18: 6:21
    scope 1 (inlined inline_me) {        // at src/lib.rs:7:5: 7:16
    }

    bb0: {
        _0 = const 42_u32;               // scope 1 at src/lib.rs:3:5: 3:7
        return;                          // scope 0 at src/lib.rs:8:2: 8:2
    }
}

_0 = const 42_u32; here is not contained inside root.

@Noratrieb
Copy link
Member Author

Good point, I did not consider this. I guess then the assertion should not be added, and the spans from outside should be better dealt with (I am not sure how that would look like though).

cc @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented Jul 29, 2022

Yea, I think we should not use relative offsets unless the span is within the function's span. Basically fallback to just the original span in that case

@Noratrieb
Copy link
Member Author

That sounds good, I'll improve that.

@Noratrieb
Copy link
Member Author

There are still a few issues that should be improved: #99908 (comment)

@clubby789
Copy link
Contributor

clubby789 commented Mar 31, 2023

@rustbot label +A-mir

@rustbot rustbot added the A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html label Mar 31, 2023
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants