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

binary_asm_labels should suggest a change #127821

Closed
tgross35 opened this issue Jul 16, 2024 · 5 comments · Fixed by #127935
Closed

binary_asm_labels should suggest a change #127821

tgross35 opened this issue Jul 16, 2024 · 5 comments · Fixed by #127935
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-inline-assembly Area: inline asm!(..) 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

@tgross35
Copy link
Contributor

tgross35 commented Jul 16, 2024

Code

use std::arch::asm;

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

Current output

error: avoid using labels containing only the digits `0` and `1` in inline assembly
   --> src/aarch64_linux.rs:142:22
    |
142 |                     "0:",
    |                      ^ use a different label that doesn't start with `0` or `1`
...
271 | foreach_cas!(compare_and_swap);
    | ------------------------------ in this macro invocation
    |
    = note: an LLVM bug makes these labels ambiguous with a binary literal number
    = note: `#[deny(binary_asm_labels)]` on by default
    = note: this error originates in the macro `compare_and_swap` which comes from the expansion of the macro `foreach_cas` (in Nightly builds, run with -Z macro-backtrace for more info)

Desired output

Suggest a change that fixes this.

Rationale and extra context

I don't see any documentation about binary_asm_labels, so we should probably suggest what to do instead. I think the correct thing is to start numbering from 2.

The lint was added in #126922 (cc @asquared31415) to fix issue #94426.

Other cases

No response

Rust Version

1.81.0-nightly (24d2ac0b5 2024-07-15)

Anything else?

No response

@tgross35 tgross35 added A-diagnostics Area: Messages for errors, warnings, and lints A-inline-assembly Area: inline asm!(..) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 16, 2024
@tgross35
Copy link
Contributor Author

There also doesn't seem to be an issue filed either here or at LLVM for actually fixing this issue, at least not that I can find.

@tgross35
Copy link
Contributor Author

Also, is this problem x86-specific? The lint is firing on aarch64.

@tgross35
Copy link
Contributor Author

tgross35 commented Jul 16, 2024

It indeed looks like this is an x86-only problem. Looks like compiler explorer won't objdump non-x86 files https://rust.godbolt.org/z/avoE4Gdfa but the sample builds and runs on my aarch64 machine.

tgross35 added a commit to tgross35/rust that referenced this issue Jul 18, 2024
The link pointed to a closed issue. Create a new one and point the link
to it.

Also add a help message to hint what change the user could make.

Fixes: rust-lang#127821
tgross35 added a commit to tgross35/rust that referenced this issue Jul 18, 2024
The link pointed to a closed issue. Create a new one and point the link
to it.

Also add a help message to hint what change the user could make.

Fixes: rust-lang#127821
tgross35 added a commit to tgross35/rust that referenced this issue Jul 18, 2024
The link pointed to a closed issue. Create a new one and point the link
to it.

Also add a help message to hint what change the user could make.

Fixes: rust-lang#127821
@estebank
Copy link
Contributor

@tgross35 can you follow another ticket for making this trigger not when it is compiled in an x86 platform but rather when it could be compiled in an x86 platform (because it is not behind an appropriate cfg flag)? Otherwise an hypothetical person writing generic code on ARM could ship code to their users that will have miscompilations, but because this is a lint, the users (and by extension, the authors) might never find out about the problem.

@tgross35
Copy link
Contributor Author

Sure, #127938

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jul 18, 2024
…y, r=estebank

Change `binary_asm_labels` to only fire on x86 and x86_64

In <rust-lang#126922>, the `binary_asm_labels` lint was added which flags labels such as `0:` and `1:`. Before that change, LLVM was giving a confusing error on x86/x86_64 because of an incorrect interpretation.

However, targets other than x86 and x86_64 never had the error message and have not been a problem. This means that the lint was causing code that previously worked to start failing (e.g. `compiler_builtins`), rather than only providing a more clear messages where there has always been an error.

Adjust the lint to only fire on x86 and x86_64 assembly to avoid this regression.

Also update the help message.

Fixes: rust-lang#127821
tgross35 added a commit to tgross35/rust that referenced this issue Jul 19, 2024
…y, r=estebank,Urgau

Change `binary_asm_labels` to only fire on x86 and x86_64

In <rust-lang#126922>, the `binary_asm_labels` lint was added which flags labels such as `0:` and `1:`. Before that change, LLVM was giving a confusing error on x86/x86_64 because of an incorrect interpretation.

However, targets other than x86 and x86_64 never had the error message and have not been a problem. This means that the lint was causing code that previously worked to start failing (e.g. `compiler_builtins`), rather than only providing a more clear messages where there has always been an error.

Adjust the lint to only fire on x86 and x86_64 assembly to avoid this regression.

Also update the help message.

Fixes: rust-lang#127821
@bors bors closed this as completed in 8410348 Jul 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 19, 2024
Rollup merge of rust-lang#127935 - tgross35:binary_asm_labels-x86-only, r=estebank,Urgau

Change `binary_asm_labels` to only fire on x86 and x86_64

In <rust-lang#126922>, the `binary_asm_labels` lint was added which flags labels such as `0:` and `1:`. Before that change, LLVM was giving a confusing error on x86/x86_64 because of an incorrect interpretation.

However, targets other than x86 and x86_64 never had the error message and have not been a problem. This means that the lint was causing code that previously worked to start failing (e.g. `compiler_builtins`), rather than only providing a more clear messages where there has always been an error.

Adjust the lint to only fire on x86 and x86_64 assembly to avoid this regression.

Also update the help message.

Fixes: rust-lang#127821
@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 asm!(..) 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.

3 participants