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

add lint for inline asm labels that look like binary #126922

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

asquared31415
Copy link
Contributor

@asquared31415 asquared31415 commented Jun 24, 2024

fixes #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

r? @estebank

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 24, 2024
@asquared31415
Copy link
Contributor Author

cc @Amanieu

@joshtriplett
Copy link
Member

This seems like a good starting point!

Eventually, I hope we can get LLVM to have a mode without this "feature", and then rust assembly should use that mode by default. But in the interim, we should warn people.

compiler/rustc_lint/src/builtin.rs Show resolved Hide resolved
compiler/rustc_lint/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
Comment on lines +420 to +424
error: avoid using named labels in inline assembly
--> $DIR/named-asm-labels.rs:156:14
|
LL | asm!(include_str!("named-asm-labels.s"));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting case. We should have a mechanism to display the contents of include and include_str and point inside of those files when complaining. At the very least, we should say "on line X col Y in file foo.s". Not to address in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I don't like the solution I came up with originally, but I opted not to change it for now, since that's a separate decision and complicates the implementation.

compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
@joshtriplett
Copy link
Member

joshtriplett commented Jun 26, 2024 via email

@asquared31415
Copy link
Contributor Author

This seems like a good starting point!

Eventually, I hope we can get LLVM to have a mode without this "feature", and then rust assembly should use that mode by default. But in the interim, we should warn people.

Somehow Clang does work around this. Perhaps something that it does with using AT&T syntax by default and making the user write .intel_syntax is meaningful? I don't know enough about how Clang uses inline assembly to be sure though. Agreed that ideally this lint does get downgraded to allow by default by fixing the issue, but I think it's important in the meantime to give people a better error.

@rust-log-analyzer

This comment has been minimized.

Comment on lines +157 to +160
//~^ ERROR avoid using named labels
//~^^ ERROR avoid using named labels
//~^^^ ERROR avoid using named labels
//~^^^^ ERROR avoid using named labels
Copy link
Contributor

Choose a reason for hiding this comment

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

For future reference, this can also be written as

Suggested change
//~^ ERROR avoid using named labels
//~^^ ERROR avoid using named labels
//~^^^ ERROR avoid using named labels
//~^^^^ ERROR avoid using named labels
//~^ ERROR avoid using named labels
//~| ERROR avoid using named labels
//~| ERROR avoid using named labels
//~| ERROR avoid using named labels

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 30, 2024

☔ The latest upstream changes (presumably #127174) made this pull request unmergeable. Please resolve the merge conflicts.

@estebank
Copy link
Contributor

estebank commented Jul 3, 2024

Please rebase on top of a recent HEAD and squash your commits. r=me after that.

@rust-cloud-vms rust-cloud-vms bot force-pushed the asm_binary_label branch from 4e0eeea to 87856c4 Compare July 9, 2024 01:24
@asquared31415
Copy link
Contributor Author

I additionally implemented the requested change to make the suggestion a label on the span rather than a help.

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 12, 2024

📌 Commit 87856c4 has been approved by estebank

It is now in the queue for this repository.

@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 Jul 12, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request 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 added a commit to rust-lang-ci/rust that referenced this pull request Jul 12, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#126502 (Ignore allocation bytes in some mir-opt tests)
 - rust-lang#126606 (Guard against calling `libc::exit` multiple times on Linux.)
 - rust-lang#126922 (add lint for inline asm labels that look like binary)
 - rust-lang#127295 (CFI: Support provided methods on traits)
 - rust-lang#127310 (Fix import suggestion ice)
 - rust-lang#127535 (Fire unsafe_code lint on unsafe extern blocks)
 - rust-lang#127631 (Remove `fully_normalize`)
 - rust-lang#127632 (Implement `precise_capturing` support for rustdoc)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 12, 2024
…kingjubilee

Rollup of 11 pull requests

Successful merges:

 - rust-lang#126502 (Ignore allocation bytes in some mir-opt tests)
 - rust-lang#126922 (add lint for inline asm labels that look like binary)
 - rust-lang#127209 (Added the `xop` target-feature and the `xop_target_feature` feature gate)
 - rust-lang#127310 (Fix import suggestion ice)
 - rust-lang#127338 (Migrate `extra-filename-with-temp-outputs` and `issue-85019-moved-src-dir` `run-make` tests to rmake)
 - rust-lang#127381 (Migrate `issue-83045`, `rustc-macro-dep-files` and `env-dep-info` `run-make` tests to rmake)
 - rust-lang#127535 (Fire unsafe_code lint on unsafe extern blocks)
 - rust-lang#127619 (Suggest using precise capturing for hidden type that captures region)
 - rust-lang#127631 (Remove `fully_normalize`)
 - rust-lang#127632 (Implement `precise_capturing` support for rustdoc)
 - rust-lang#127660 (Rename the internal `const_strlen` to just `strlen`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fc0136e into rust-lang:master Jul 13, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 13, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request 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``
tgross35 added a commit to tgross35/rust that referenced this pull request Jul 18, 2024
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.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request 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 pull request 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
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request 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
L-binary_asm_labels Lint: LLVM parses labels like 0, 1, 10, 11, etc. oddly S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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