Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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
# for free to join this conversation on GitHub. Already have an account? # 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