Rewrite local labels in inline assembly #81570

asquared31415 · 2021-01-30T20:08:49Z

It is possible for certain asm! blocks to be duplicated and emitted more than once by the compiler through inlining or other optimizations. If a duplicated asm! block has a label, the compiler will error with a invalid symbol redefinition error. If the -g flag is passed to the compiler, the compiler will abort due to a crash with no output. #74262 shows an example of one such error and crash. This bug also occurs if two separate asm! blocks have the same label, preventing the reuse of simple labels like loop:.

This PR aims to fix these issues by making all labels in asm! blocks local labels using LLVM's ${:private} and ${:uid} as suggested in #81088. Local labels do not suffer from the same crashes and errors that previously occurred.

Closes #74262
Closes #81088
Addresses part of this comment in the inline assembly tracking issue

rust-highfive · 2021-01-30T20:08:52Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

asquared31415 · 2021-01-30T20:08:58Z

I have two concerns about this change.

Performance - This change adds a significant amount of string processing as compared to the simple operation of just adding a string before, however it's all compile time processing. Can/should this be tested for performance, and if so, how?
Is it legal to define a label in one asm! block and use it in another? This code would break that if that was allowed.

nagisa · 2021-01-30T20:19:43Z

Is it legal to define a label in one asm! block and use it in another? This code would break that if that was allowed.

I remember seeing that being done in some of the codebases using GCC inline assembly, but we're still in a position to call it UB or something along those lines.

asquared31415 · 2021-01-30T20:35:19Z

That's unfortunate. I couldn't find any information in the llvm docs or in the inline assembly rfc either confirming or denying that it was allowed. Though it makes sense that it's at least legal on llvm's side, since that's the cause of this whole issue anyway, that labels are global by default and llvm sees them as a redefinition.

compiler/rustc_codegen_llvm/src/asm.rs

asquared31415 · 2021-01-31T00:33:47Z

I have done some thinking on how to possibly solve the fact that this makes all labels local. The possible solutions that I can think of are:

labels are always local to your inline assembly block. This is possibly undesirable as it prevents use of labels across blocks entirely. This is what this PR currently does.
labels are local by default unless you specify them to be global. This can still cause issues with inline and the weird redefinition errors and it would still crash if a user specified global labels and they were duplicated and built with debug mode. However this time, it requires the user to actively specify something to break, and the default (and more intuitive to me) case is fine.
labels are global by default unless you specify them to be local. This is how the current state of asm! is and it causes these issues to appear.

oli-obk · 2021-02-01T14:54:43Z

r? @nagisa

feel free to pingpong back to me if you don't have capacity for this right now, just seemed like you know more about this than me :)

nagisa · 2021-02-02T01:26:50Z

I think it is fine to say labels are local to the given asm!() invocation for now, if not forever. We are not bound by having to support ancient code that relied on this accidental feature exposed by the more naive compilers.

We can consider adding something for the global labels when and if somebody comes with well motivated use-cases and, probably, an RFC.

nagisa · 2021-02-02T01:41:01Z

@Amanieu you mention in this comment that the RFC specified only local labels shall be used, but then suggest localizing the labels in this one. Should that be considered the official position of the WG-inline-asm?

If not, I perhaps would be more comfortable with a built-in diagnostic that prevents use of non-local labels in the first place, with suggestions on how the user could adjust their code to use the local labels instead. Denying that kind of code would also prevent accidental lock in into a design decision that we end up regretting later. That would also set us up for denying code that uses LLVM-specific features like… ${:uid}…

asquared31415 · 2021-02-02T01:42:33Z

Specifying all labels to be local is fine in my opinion as well, and I took that approach based on the issue you just linked, #81088. #76704 is possibly relevant, as if the decision is made to make all labels local, the unstable book should reflect that.

Amanieu · 2021-02-02T02:12:36Z

Labels defined in asm! are always local. You cannot use them outside the current asm block since the asm may be duplicated by LLVM.

You can only define labels with a global scope in global_asm! (which doesn't need any rewriting).

@Amanieu you mention in this comment that the RFC specified only local labels shall be used, but then suggest localizing the labels in this one. Should that be considered the official position of the WG-inline-asm?

The asm! RFC as written only allows GAS local labels (0:, 1:) in asm!. However I have since changed my mind about this:

This requirement is not obvious and is unfamiliar to people used to writing assembly.
It is easy to forget: code using named labels will usually work fine until LLVM suddenly decides to duplicate the asm! block in the future.
LLVM's intel asm syntax sometimes parses 0b and 1b as binary literals instead of local label references, which leads to confusing errors.

I raised this suggestion in the asm! tracking issue (#72016 (comment)) and nobody has objected so far. I think this change is strictly beneficial and doesn't actually lock us into any LLVM-specific features. GCC's inline asm has an equivalent to {:uid} (%=), and the cranelift fallback of going through an external assembler doesn't need it.

@rust-lang/wg-inline-asm Any objections to adding this feature?

Amanieu

This needs some codegen tests that check that the correct string is passed to LLVM.

Amanieu · 2021-02-02T02:14:17Z

compiler/rustc_codegen_llvm/src/asm.rs

@@ -212,6 +213,10 @@ impl AsmBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> {
 }
 }
 } else {
+ if let Some(label) = get_llvm_label_from_str(s) {


A template string piece may consist of multiple lines (e.g. raw string literals). These strings could define more than one label.

Ah thanks, I didn't think of this case

Amanieu · 2021-02-02T02:18:11Z

compiler/rustc_codegen_llvm/src/asm.rs

+ ('a'..='z').contains(&c) || ('A'..='Z').contains(&c) || '_' == c || '.' == c
+}
+
+// All subsequent chars are [a-zA-Z0-9_$.@]


I would prefer if we disallowed @ and $ in symbol names. We don't need to support all symbol names here, the user is free to shoot themselves in the foot if they choose to use a symbol name with weird characters.

I would prefer if we disallowed @ and $ in symbol names.

That should happen elsewhere, possibly in the same location parsing for formatting specifiers happens. Should also be a concern for a separate PR.

My main concern here is that these may not be valid characters in symbol names for all targets and may have other special meanings on those targets.

Sure, I agree. I still think this is the the wrong place in the codebase to enforce what characters we allow in asm labels. Probably should happen as a separate PR too.

We don't actually prevent the user from using such labels. We just don't provide automatic rewriting into local labels for them.

Also we're not handling all possible label definitions. For example labels can be defined after a ; on targets where that is a statement separator.

We're not trying to handle all possible cases here since that would require us to effectively parse the asm, which we really want to leave to LLVM. We're just trying to make the most common types of labels work correctly.

We don't actually prevent the user from using such labels. We just don't provide automatic rewriting into local labels for them.

Aha, I see the distinction you're trying to make now.

We're just trying to make the most common types of labels work correctly.

That seems like a recipe for a pretty poor experience with a very non-obvious cliff at which things may start failing in non-obvious ways. For instance, user with passing familiarity with Rust's asm! labels being local could write something like this:

#[inline] // not always pub fn peach() { unsafe { asm!("per$$imon: jmp per$$imon") } }

and it might very well work out in basic tests, but start failing once the function is released to the field. If we do go the route of making things automagic, its better they're magic all the time, not only sometimes.

that would require us to effectively parse the asm, which we really want to leave to LLVM

I believe just lexing would be sufficient, but I do agree that we'd rather not do that at all.

At the very least I would like @ to be removed from the set of allowed characters for symbols. @ is used as a symbol modifier in x86 intel syntax: sym@gotpcrel.

I want to confirm what we want to accept as a valid identifier for the purposes of replacing labels. Denying symbols outright should be properly implemented somewhere else, preferably with a compiler warning or error. This code can reflect what symbols are accepted, but it will ignore other things. For example, @foobar: will be replaced with @${:private}foobar${:uid} if we choose to not acknowledge @ in labels.

Is the set of symbols we will accept in a label [a-zA-Z0-9_$.]?

Additionally, do we want to accept labels that are strings like nagisa commented here?

I think that for label renaming we should only support [a-zA-Z0-9_.], and only plain labels not quoted ones.

Amanieu · 2021-02-02T02:26:09Z

compiler/rustc_codegen_llvm/src/asm.rs

+ // Fixes https://github.com/rust-lang/rust/issues/74262
+ for label in labels.iter() {
+ let ranges: Vec<_> = template_str
+ .match_indices(label)


I don't think this checks if the character before the match is a non-identifier character. This means that if you have a label bar then foobar will be replaced with foo${:private}bar${:uid} which is really not what we want.

Amanieu · 2021-02-02T02:28:33Z

compiler/rustc_codegen_llvm/src/asm.rs

+// Valid llvm symbols: https://llvm.org/docs/AMDGPUOperandSyntax.html#symbols
+// First char is [a-zA-Z_.]
+fn llvm_ident_start(c: char) -> bool {
+ ('a'..='z').contains(&c) || ('A'..='Z').contains(&c) || '_' == c || '.' == c


Suggested change

('a'..='z').contains(&c) || ('A'..='Z').contains(&c) || '_' == c || '.' == c

matches!(c, 'a'..='z' | 'A'..='Z' | '_' | '.')

matches! is more concise, thanks

nagisa · 2021-02-02T11:38:02Z

compiler/rustc_codegen_llvm/src/asm.rs

+ || '_' == c
+ || '$' == c
+ || '.' == c
+ || '@' == c


A ? mark is a valid continuation character according to clang: https://rust.godbolt.org/z/EsP4sW, though it does appear like a bug and is only intended to be allowed when emulating the microsoft cl. Nevertheless it is something that LLVM currently will let through.

nagisa · 2021-02-02T11:42:28Z

compiler/rustc_codegen_llvm/src/asm.rs

+ return Some(substr);
+ }
+ }
+ }


One way to get more arbitrary stuff into a asm label is to quote it as a string, like this:

void test(void) { __asm__(".intel_syntax noprefix\n" "\"01☺☺@banana.?@!\": jmp \"01☺☺@banana.?@!\""); }

or in Rust

#![feature(asm)] pub fn test() { unsafe { asm!(r#" "☺helloworld?@!$%^&": jmp "☺helloworld?@!$%^&" "#); } }

So that's something we should handle or forbid too.

Amanieu · 2021-02-02T12:06:52Z

compiler/rustc_codegen_llvm/src/asm.rs

+
+/// Gets a LLVM label from a &str if it exists
+fn get_llvm_label_from_str(s: &str) -> Option<&str> {
+ if let Some(colon_idx) = s.find(':') {


Use trim_start to strip leading whitespace from the line.

joshtriplett · 2021-02-03T21:40:46Z

Forcing all labels within an asm block to be local would certainly improve usability for the common case (so that people don't have to use local labels).

However, it is legal in clang and gcc to define a label in an asm block and make that label a global symbol. And I've seen that used, on occasion, in real projects, including the Linux kernel. (For instance, suppose you want a global symbol pointing to a location that can be overwritten as a "probe point".) Rewriting would cause such code to silently "work" while making a global symbol with the wrong name, rather than catching the issue.

I agree that such code is not especially safe, if you're not certain that the asm code will appear in the binary exactly once. (In C that's a little easier to guarantee.) It's safer in global_asm, and I do think that we should support it there eventually.

In the meantime, I personally would prefer to make a best-effort attempt to catch the use of non-local labels in asm! and error on them, rather than attempting to rewrite them (which requires more careful lexing of inline assembly). Alternatively, if the thought is that we might allow global labels in the future, perhaps we could attempt to lex a little more of the inline assembly, catch attempts to use .globl or .global, and emit an error when applying those directives to a label.

EDIT: I posted this to the PR because I would prefer to just reject non-local labels, which would be a change to this PR. If we decide instead to reject .globl/.global, then the discussion should move to the tracking issue and the outcome would be a separate PR.

Amanieu · 2021-02-03T21:52:41Z

Let's move this discussion over to #81088. Comments on the PR should be about reviewing the implementation, not the design or discussing whether we should do this.

joshtriplett · 2021-02-03T21:54:11Z

@Amanieu Alright, reposted to #81088 .

nagisa · 2021-03-07T18:49:54Z

I'm not entirely sure what it would take for this PR to land, myself. It seems pretty clear that we'd want to have all inline assembly labels be local, but on the other hand implementing a inline assembly parser in rustc is also not really something I think we should do.

Thinking about the ideal way to solve this, I think we'd want to add support for this kind of interpretation of the inline assembly labels to the backend (i.e. LLVM) itself. Then you wouldn't have to think hard about the corner cases such as label names being almost anything if quoted, or different targets supporting their own special flavour of syntax/identifiers.

I'm still not sure about only handling the most common cases, primarily because it might feed into a decision to stabilize the inline assembly in its partially finished state. Once stabilized, I don't think we'll have any leeway in changing the behaviour anymore (except maybe on edition boundaries?) – users would come to rely on the labels being either global or local, and behaviour change in any direction would break compatibility.

Amanieu · 2021-03-07T19:22:43Z

Perhaps we should just lint against labels for now until we have made a decision on how best to support labels in asm!.

nagisa · 2021-03-07T19:37:35Z

Yeah, banning/linting against label definitions that don't follow [0-9a-zA-Z_\.]+ (and then rewriting only those) for now is also a path we could take.

Dylan-DPC-zz · 2021-03-23T10:51:41Z

@asquared31415 this is waiting on you to reply to the above reviews. Thanks

bors · 2021-04-05T04:05:52Z

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

crlf0710 · 2021-04-23T13:38:46Z

Triage: Seems this is blocked on #81088

asquared31415 · 2021-10-08T21:03:14Z

I think that for the time being we have decided to not pursue this path. I am willing to do the work if this (or a similar) approach is decided on.

asquared31415 added 5 commits January 27, 2021 22:55

Avoid possible compiler crash and duplicate labels errors in asm! blocks

70f627c

Move get_llvm_label_from_str out of codegen_inline_asm

4bb2d5d

Add explicit inline and simple duplication tests

bce67de

Handle label declared after usage

e78a1bf

Make it only replace independent identifiers that are labels

b72835f

rust-highfive assigned oli-obk Jan 30, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 30, 2021

Better use of iterators and filter_map

be00fa4

jfrimmel reviewed Jan 30, 2021

View reviewed changes

compiler/rustc_codegen_llvm/src/asm.rs Outdated Show resolved Hide resolved

Better organize llvm identifier code

1e27711

rust-highfive assigned nagisa and unassigned oli-obk Feb 1, 2021

Amanieu reviewed Feb 2, 2021

View reviewed changes

nagisa reviewed Feb 2, 2021

View reviewed changes

Amanieu reviewed Feb 2, 2021

View reviewed changes

joshtriplett mentioned this pull request Feb 3, 2021

asm! should automatically rewrite labels in assembly code #81088

Open

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2021

Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 23, 2021

asquared31415 changed the title ~~Fix compiler crashes and errors with duplicate labels in inline assembly~~ Rewrite local labels in inline assembly Jul 7, 2021

asquared31415 closed this Oct 8, 2021

asquared31415 deleted the asm-duplicate-labels-fix branch October 9, 2021 17:07

	('a'..='z').contains(&c) \|\| ('A'..='Z').contains(&c) \|\| '_' == c \|\| '.' == c
	matches!(c, 'a'..='z' \| 'A'..='Z' \| '_' \| '.')

Rewrite local labels in inline assembly #81570

Rewrite local labels in inline assembly #81570

Conversation

asquared31415 commented Jan 30, 2021 • edited Loading

rust-highfive commented Jan 30, 2021

asquared31415 commented Jan 30, 2021

nagisa commented Jan 30, 2021 • edited Loading

asquared31415 commented Jan 30, 2021

asquared31415 commented Jan 31, 2021

oli-obk commented Feb 1, 2021

nagisa commented Feb 2, 2021

nagisa commented Feb 2, 2021

asquared31415 commented Feb 2, 2021

Amanieu commented Feb 2, 2021

Amanieu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshtriplett commented Feb 3, 2021 • edited Loading

Amanieu commented Feb 3, 2021

joshtriplett commented Feb 3, 2021

nagisa commented Mar 7, 2021 • edited Loading

Amanieu commented Mar 7, 2021

nagisa commented Mar 7, 2021 • edited Loading

Dylan-DPC-zz commented Mar 23, 2021

bors commented Apr 5, 2021

crlf0710 commented Apr 23, 2021

asquared31415 commented Oct 8, 2021

asquared31415 commented Jan 30, 2021 •

edited

Loading

nagisa commented Jan 30, 2021 •

edited

Loading

nagisa Feb 2, 2021 •

edited

Loading

joshtriplett commented Feb 3, 2021 •

edited

Loading

nagisa commented Mar 7, 2021 •

edited

Loading

nagisa commented Mar 7, 2021 •

edited

Loading