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

in asm!() using a local numeric label made of all 0's and 1's gives a confusing error #94426

Closed
oconnor663 opened this issue Feb 27, 2022 · 4 comments · Fixed by #126922
Closed
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-inline-assembly Area: Inline assembly (`asm!(…)`) D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. L-binary_asm_labels Lint: LLVM parses labels like 0, 1, 10, 11, etc. oddly T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@oconnor663
Copy link
Contributor

oconnor663 commented Feb 27, 2022

rustc 1.59.0 (9d1b2106e 2022-02-23)

Here's a function that jumps to a local numberic label inside of asm!:

use std::arch::asm;

#[cfg(target_arch = "x86_64")]
fn main() {
    let mut counter: u64 = 0;
    unsafe {
        asm!(
            "2:",
            "inc {counter}",
            "cmp {counter}, 10",
            "jnz 2b",
            counter = inout(reg) counter,
        );
    }
    dbg!(counter);  // prints 10
}

As per Rust By Example, we use 2: as a local label there to work around an LLVM bug that mistakes labels like 0:, 1:, or 10: for binary. This example works. However, if we don't know about this rule, and we try to use 0: instead, the error we get is confusing:

error: invalid operand for instruction
  --> src/main.rs:11:14
   |
11 |             "jnz 0b",
   |              ^
   |
note: instantiated into assembly here
  --> <inline asm>:5:1
   |
5  | jnz 0b
   | ^

error: could not compile `scratch` due to previous error

Using 1: is the same:

error: invalid operand for instruction
  --> src/main.rs:11:14
   |
11 |             "jnz 1b",
   |              ^
   |
note: instantiated into assembly here
  --> <inline asm>:5:1
   |
5  | jnz 1b
   | ^

error: could not compile `scratch` due to previous error

It would be pretty great to have a hint here like "0: and 1: aren't valid local labels unfortunately, so just start with 2:".

@oconnor663 oconnor663 added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 27, 2022
@estebank estebank added A-inline-assembly Area: Inline assembly (`asm!(…)`) D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. labels Apr 24, 2022
@joshtriplett joshtriplett changed the title in asm!() using a local numberic label made of all 0's and 1's gives a confusing error in asm!() using a local numeric label made of all 0's and 1's gives a confusing error Jun 19, 2024
@joshtriplett joshtriplett added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Jun 19, 2024
@asquared31415
Copy link
Contributor

@rustbot claim

@joshtriplett
Copy link
Member

Documenting something that isn't explicitly stated here: this is an issue specifically related to the use of Intel syntax, because (AFAICT) LLVM doesn't have this issue for AT&T syntax.

This seems like something that, eventually, we could try to get a fix for into LLVM. We would need an assembly dialect option that either handles binary numbers differently or handles backwards labels differently.

@apiraino apiraino removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Jul 3, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 12, 2024
…estebank

add lint for inline asm labels that look like binary

fixes rust-lang#94426

Due to a bug/feature in LLVM, labels composed of only the digits `0` and `1` can sometimes be confused with binary literals, even if a binary literal would not be valid in that position.

This PR adds detection for such labels and also as a drive-by change, adds a note to cases such as `asm!(include_str!("file"))` that the label that it found came from an expansion of a macro, it wasn't found in the source code.

I expect this PR to upset some people that were using labels `0:` or `1:` without issue because they never hit the case where LLVM got it wrong, but adding a heuristic to the lint to prevent this is not feasible - it would involve writing a whole assembly parser for every target that we have assembly support for.

[zulip discussion](https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202024-06-20/near/445870628)

r? `@estebank`
@bors bors closed this as completed in fc0136e Jul 13, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 13, 2024
Rollup merge of rust-lang#126922 - asquared31415:asm_binary_label, r=estebank

add lint for inline asm labels that look like binary

fixes rust-lang#94426

Due to a bug/feature in LLVM, labels composed of only the digits `0` and `1` can sometimes be confused with binary literals, even if a binary literal would not be valid in that position.

This PR adds detection for such labels and also as a drive-by change, adds a note to cases such as `asm!(include_str!("file"))` that the label that it found came from an expansion of a macro, it wasn't found in the source code.

I expect this PR to upset some people that were using labels `0:` or `1:` without issue because they never hit the case where LLVM got it wrong, but adding a heuristic to the lint to prevent this is not feasible - it would involve writing a whole assembly parser for every target that we have assembly support for.

[zulip discussion](https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202024-06-20/near/445870628)

r? ``@estebank``
@asquared31415
Copy link
Contributor

asquared31415 commented Jul 13, 2024

While the diagnostics side of this is fixed and so this issue should remain closed, I think that the underlying bug that prevents these labels from being usable should be tracked somewhere, does anyone know if we already have an issue for that underlying bug?

@joshtriplett
Copy link
Member

@asquared31415 I don't think we have an issue for that. We probably should, but ultimately I think we can't fix it without some upstream support in LLVM. (Assuming we continue to not attempt to parse all assembly ourselves.)

@workingjubilee workingjubilee added the L-binary_asm_labels Lint: LLVM parses labels like 0, 1, 10, 11, etc. oddly label Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-inline-assembly Area: Inline assembly (`asm!(…)`) D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. L-binary_asm_labels Lint: LLVM parses labels like 0, 1, 10, 11, etc. oddly 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