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

Improve inline asm error diagnostics #72625

Merged
merged 2 commits into from
May 31, 2020
Merged

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented May 26, 2020

Previously we were just using the raw LLVM error output (with line, caret, etc) as the diagnostic message, which ends up looking rather out of place with our existing diagnostics.

The new diagnostics properly format the diagnostics and also take advantage of LLVM's per-line srcloc attribute to map an error in inline assembly directly to the relevant line of source code.

Incidentally also fixes #71639 by disabling srcloc metadata during LTO builds since we don't know what crate it might have come from. We can only resolve srclocs from the currently crate since it indexes into the source map for the current crate.

Fixes #72664
Fixes #71639

r? @petrochenkov

Old style

#![feature(llvm_asm)]

fn main() {
    unsafe {
        let _x: i32;
        llvm_asm!(
            "mov $0, $1
             invalid_instruction $0, $1
             mov $0, $1"
             : "=&r" (_x)
             : "r" (0)
             :: "intel"
        );
    }
}
error: <inline asm>:3:14: error: invalid instruction mnemonic 'invalid_instruction'
             invalid_instruction ecx, eax
             ^~~~~~~~~~~~~~~~~~~

  --> src/main.rs:6:9
   |
6  | /         llvm_asm!(
7  | |             "mov $0, $1
8  | |              invalid_instruction $0, $1
9  | |              mov $0, $1"
...  |
12 | |              :: "intel"
13 | |         );
   | |__________^

New style

#![feature(asm)]

fn main() {
    unsafe {
        asm!(
            "mov {0}, {1}
             invalid_instruction {0}, {1}
             mov {0}, {1}",
            out(reg) _,
            in(reg) 0i64,
        );
    }
}
error: invalid instruction mnemonic 'invalid_instruction'
 --> test.rs:7:14
  |
7 |              invalid_instruction {0}, {1}
  |              ^
  |
note: instantiated into assembly here
 --> <inline asm>:3:14
  |
3 |              invalid_instruction rax, rcx
  |              ^^^^^^^^^^^^^^^^^^^

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 26, 2020
@Amanieu
Copy link
Member Author

Amanieu commented May 26, 2020

I think we could get even better diagnostics if we could encode more data into the srcloc. LLVM only gives us one u32 per asm source line so we would need to store an index into a table of asm lines. I haven't looked into that yet but it may get quite hairy due to interactions with incremental compilation and codegen units. I would appreciate some mentoring there!

@rust-highfive

This comment has been minimized.

@Amanieu Amanieu force-pushed the asm-srcloc branch 2 times, most recently from 7302a02 to bd7db0f Compare May 26, 2020 22:14
@mominul
Copy link
Contributor

mominul commented May 27, 2020

Would this fix #72664 ?

@Amanieu
Copy link
Member Author

Amanieu commented May 27, 2020

Ah yes, indeed.

@petrochenkov
Copy link
Contributor

This is a great improvement.
I can't help with the FIXME, unfortunately.
@bors r+

@bors
Copy link
Contributor

bors commented May 28, 2020

📌 Commit bd7db0fb1ed741f8ef0342dd69797e4117af0f04 has been approved by petrochenkov

@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 28, 2020
--> $DIR/srcloc.rs:15:13
|
LL | invalid_instruction
| ^
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if this could point at the entire width of invalid_instruction. (I saw the FIXME, which is why this isn't the case.)

Comment on lines +19 to +23
note: instantiated into assembly here
--> <inline asm>:3:13
|
LL | invalid_instruction
| ^^^^^^^^^^^^^^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great if we could avoid the note when it is redundant and the resulting evaluated assembly is the same as what is in the code.

@Amanieu
Copy link
Member Author

Amanieu commented May 29, 2020

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented May 29, 2020

📌 Commit fc497f7 has been approved by petrochenkov

RalfJung added a commit to RalfJung/rust that referenced this pull request May 30, 2020
Improve inline asm error diagnostics

Previously we were just using the raw LLVM error output (with line, caret, etc) as the diagnostic message, which ends up looking rather out of place with our existing diagnostics.

The new diagnostics properly format the diagnostics and also take advantage of LLVM's per-line `srcloc` attribute to map an error in inline assembly directly to the relevant line of source code.

Incidentally also fixes rust-lang#71639 by disabling `srcloc` metadata during LTO builds since we don't know what crate it might have come from. We can only resolve `srcloc`s from the currently crate since it indexes into the source map for the current crate.

Fixes rust-lang#72664
Fixes rust-lang#71639

r? @petrochenkov

### Old style

```rust
#![feature(llvm_asm)]

fn main() {
    unsafe {
        let _x: i32;
        llvm_asm!(
            "mov $0, $1
             invalid_instruction $0, $1
             mov $0, $1"
             : "=&r" (_x)
             : "r" (0)
             :: "intel"
        );
    }
}
```

```
error: <inline asm>:3:14: error: invalid instruction mnemonic 'invalid_instruction'
             invalid_instruction ecx, eax
             ^~~~~~~~~~~~~~~~~~~

  --> src/main.rs:6:9
   |
6  | /         llvm_asm!(
7  | |             "mov $0, $1
8  | |              invalid_instruction $0, $1
9  | |              mov $0, $1"
...  |
12 | |              :: "intel"
13 | |         );
   | |__________^
```

### New style

```rust
#![feature(asm)]

fn main() {
    unsafe {
        asm!(
            "mov {0}, {1}
             invalid_instruction {0}, {1}
             mov {0}, {1}",
            out(reg) _,
            in(reg) 0i64,
        );
    }
}
```

```
error: invalid instruction mnemonic 'invalid_instruction'
 --> test.rs:7:14
  |
7 |              invalid_instruction {0}, {1}
  |              ^
  |
note: instantiated into assembly here
 --> <inline asm>:3:14
  |
3 |              invalid_instruction rax, rcx
  |              ^^^^^^^^^^^^^^^^^^^
```
bors added a commit to rust-lang-ci/rust that referenced this pull request May 30, 2020
Rollup of 13 pull requests

Successful merges:

 - rust-lang#72543 (Account for missing lifetime in opaque and trait object return types)
 - rust-lang#72625 (Improve inline asm error diagnostics)
 - rust-lang#72637 (expand `env!` with def-site context)
 - rust-lang#72650 (Sort sidebar elements)
 - rust-lang#72657 (Allow types (with lifetimes/generics) in impl_lint_pass)
 - rust-lang#72666 (Add -Z profile-emit=<path> for Gcov gcda output.)
 - rust-lang#72668 (Fix missing parentheses Fn notation error)
 - rust-lang#72669 (rustc_session: Cleanup session creation)
 - rust-lang#72728 (Make bootstrap aware of relative libdir in stage0 compiler)
 - rust-lang#72757 (rustc_lexer: Optimize shebang detection slightly)
 - rust-lang#72772 (miri validation: clarify valid values of 'char')
 - rust-lang#72773 (Fix is_char_boundary documentation)
 - rust-lang#72777 (rustdoc: remove calls to `local_def_id_from_node_id`)

Failed merges:

r? @ghost
@bors bors merged commit fadfcb6 into rust-lang:master May 31, 2020
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.
Projects
None yet
6 participants