Lint against named asm labels #87324

asquared31415 · 2021-07-20T22:08:56Z

This adds a deny-by-default lint to prevent the use of named labels in inline asm!. Without a solution to #81088 about whether the compiler should rewrite named labels or a special syntax for labels, a lint against them should prevent users from writing assembly that could break for internal compiler reasons, such as inlining or anything else that could change the number of actual inline assembly blocks emitted.

This does not resolve the issue with rewriting labels, that still needs a decision if the compiler should do any more work to try to make them work.

rust-highfive · 2021-07-20T22:08:59Z

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

asquared31415 · 2021-07-20T22:09:34Z

r? @Amanieu

asquared31415 · 2021-07-20T22:20:18Z

I decided to initially make this a deny-by-default lint because the use of named labels is almost always wrong. The RFC even says that only GAS style labels should be used. However I think it could be important to be able to be disabled if a user really understands what that means. This could be made warn by default (which I don't think is a good idea, named labels are objectively wrong) or a hard error though.

asquared31415 · 2021-07-20T22:52:57Z

Whoops, had some git issues and seemingly didn't rebase that file right when trying to fix things

compiler/rustc_builtin_macros/src/asm.rs

compiler/rustc_lint_defs/src/lib.rs

compiler/rustc_lint_defs/src/builtin.rs

compiler/rustc_builtin_macros/src/asm.rs

Amanieu · 2021-07-21T13:03:18Z

compiler/rustc_builtin_macros/src/asm.rs

+                            "do not use named labels in inline assembly",
+                            BuiltinLintDiagnostics::NamedAsmLabel("Only GAS local labels of the form `N:` where N is a number may be used in inline asm".to_string()),


I would word this slightly differently:

Suggested change

"do not use named labels in inline assembly",

BuiltinLintDiagnostics::NamedAsmLabel("Only GAS local labels of the form `N:` where N is a number may be used in inline asm".to_string()),

"avoid using named labels in inline assembly",

BuiltinLintDiagnostics::NamedAsmLabel("Only GAS local labels of the form `N:` where N is a number should be used in inline asm".to_string()),

I'm not sure if you missed my comment or if you prefer sticking to the existing wording.

I've incorporated this and also made the help a little more concise

asquared31415 · 2021-07-21T22:29:44Z

I think it's right that that fails, technically, but that makes that test hard to do. What should I do, allow the lint there?

Amanieu · 2021-07-22T07:11:20Z

Yes, just allow the lint there.

bors · 2021-07-24T16:26:29Z

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

bors · 2021-07-25T07:26:05Z

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

bors · 2021-08-04T10:19:43Z

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

Amanieu · 2021-07-30T11:06:01Z

compiler/rustc_builtin_macros/src/asm.rs

+                    for span in spans.into_iter() {
+                        ecx.parse_sess().buffer_lint_with_diagnostic(
+                            lint::builtin::NAMED_ASM_LABELS,
+                            span,


buffer_lint_with_diagnostic can take a Vec<Span> so that we can emit a single lint for all the labels in an asm!.

Oh neat I didn't realize that was what MultiSpan meant, though it seems pretty obvious in hindsight and I should have checked the impls for it. That looks a lot cleaner.

Amanieu · 2021-07-30T11:06:25Z

compiler/rustc_builtin_macros/src/asm.rs

+                        ecx.current_expansion.lint_node_id,
+                        "avoid using named labels in inline assembly",
+                        BuiltinLintDiagnostics::NamedAsmLabel("only local labels of the form `<number>:` should be used in inline asm".to_string()),
+                    );


Avoid the code duplication if the span is the only difference between the two calls.

Amanieu · 2021-08-04T16:02:32Z

(sorry about the late review, I thought that I submitted it last week but apparently something went wrong and it just got buffered)

npmccallum · 2021-08-16T14:45:59Z

@Amanieu @bjorn3 While I definitely think we want a lint similar to this, I wish it had gone through some broader review before merging. We awoke this morning to find our CI/CD broken because of this lint erroring on local symbolic labels (i.e. .Lfoo).

https://docs.oracle.com/cd/E19120-01/open.solaris/817-5477/esqaq/index.html
https://sourceware.org/binutils/docs-2.25/as/Symbol-Names.html

The motivation for this patch is to prevent emitting conflicting entries to the object file's symbol table. Numeric labels don't cause this problem and are therefore allowed. But this patch additionally disables the use of local symbolic labels which also don't cause this problem and these should be allowed too.

This lint breaks the compilation of existing, working and valid code. I propose that this patch be reverted until it allows local symbolic labels.

cc @haraldh @joshtriplett

Amanieu · 2021-08-16T14:59:17Z

Actually local labels (.Lfoo) can still cause conflicts if there are multiple definitions within the same object file/compilation unit. This can happen if a function containing inline assembly is inlined or duplicated within the same compilation unit. So the lint is correct as it is right now.

Amanieu · 2021-08-16T15:01:46Z

Example: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=af54e969951198a25e406c42cf37fc8a

npmccallum · 2021-08-16T15:06:56Z

@Amanieu This still seems to me like hammering a nail with a sledge hammer.

Before this lint, the compiler produced a completely readable error message regarding the unsupported configuration. Therefore, this lint is not an improvement of an existing defect.

error: invalid symbol redefinition
  --> src/lib.rs:11:15
   |
11 |         asm!(".Lfoo: nop");
   |               ^
   |
note: instantiated into assembly here
  --> <inline asm>:2:2
   |
2  |     .Lfoo: nop
   |     ^

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

If we want this lint (I'm not sure we do), it should only be active in the cases where this collision can occur (i.e. during inlining).
It should definitely not be active on naked functions where the pain is most acute. Naked functions will never be inlined or duplicated. Naked functions also benefit the most from local named labels.

Amanieu · 2021-08-16T15:13:00Z

Do you have any suggestions on how the lint message can be improved?
Duplication can happen anywhere. Even if a function is marked #[inline(never)], compiler optimizations can still duplicate parts of the function body, including asm! blocks.
This is a valid point, perhaps we can disable this lint for naked functions. This seems a bit difficult with the current implementation since it emits the lint during macro expansion. Could #[naked] implicitly add #[allow(named_asm_label)]?

npmccallum · 2021-08-16T15:21:22Z

@Amanieu

Do you have any suggestions on how the lint message can be improved?

The lint should specify that local named labels are unsupported outside naked functions.

Duplication can happen anywhere. Even if a function is marked #[inline(never)], compiler optimizations can still duplicate parts of the function body, including asm! blocks.

I concede this point.

This is a valid point, perhaps we can disable this lint for naked functions. This seems a bit difficult with the current implementation since it emits the lint during macro expansion. Could #[naked] implicitly add #[allow(named_asm_label)]?

I'd have to think through the implications of that. Would it mean that developers couldn't change this to an error for naked functions?

However, this PR still breaks existing, valid code and should be reverted until the implications of this change are properly understood and the semantics are agreed upon.

bjorn3 · 2021-08-16T15:40:42Z

However, this PR still breaks existing, valid code

Apart from naked functions, it has never been valid to use named labels, even before this PR. A future LLVM version or other backend may cause this to break at any point. The point of this PR is exactly to prevent people from writing seemingly valid, but not actually valid inline assembly.

I agree that it makes sense to disable this lint for naked functions though, but I don't think it warrants a revert of this PR, thus an extension to disable the lint for naked functions.

and should be reverted until the implications of this change are properly understood and the semantics are agreed upon.

The inline asm RFC has already defined it as not allowed:

https://rust-lang.github.io/rfcs/2873-inline-asm.html

You cannot assume that an asm! block will appear exactly once in the output binary. The compiler is allowed to instantiate multiple copies of the asm! block, for example when the function containing it is inlined in multiple places.

As a consequence, you should only use local labels inside inline assembly code. Defining symbols in assembly code may lead to assembler and/or linker errors due to duplicate symbol definitions.

Apart from naked functions I don't think there are any edge cases where it would be fine to allow named asm labels. If there are, an issue can be opened and a solution can be discussed. As asm!() is still nightly-only, it is allowed to break it. That doesn't mean we should randomly do so, but it is acceptable if there is an alternative that can be used. In this case that is using numbered labels. There have been much bigger breaking changes to nightly-only features in the past, including renaming asm!() to llvm_asm!() to make place for the much better asm!() we currently have.

npmccallum · 2021-08-16T16:10:36Z

@bjorn3 Which still means that this PR breaks existing, valid code in the case of naked functions. That is, a change in one feature has broken valid code in another feature. The implementor of the (now broken) feature wasn't consulted and is being asked to scramble to fix the breakage or live with it. This isn't contributor friendly.

It should be the responsibility of the PR author to fix breakage in other features caused by the PR. Ideally, this should be caught before merge. But when it isn't, how should we handle it? I would argue that it is still the responsibility of the PR author to fix the breakage. And the only way, procedurally, that we can enforce this is to revert the PR and reopen it.

asquared31415 · 2021-08-16T16:16:39Z

I agree with bjorn on all 3 points. Rust makes no stability guarantees about unstable features, and it is allowed and there is precedent to entirely change unstable features. If someone must rely on a nightly feature never changing, they should pin the nightly version to a specific, known working version, and update with caution, until the feature is stabilized.

I was not aware that #[naked] functions prevent code duplication or inlining, and because of that implemented this lint unconditionally to match the the RFC. If it is true that #[naked] functions are exempt from the potential issues, they should be granted an exception in this lint, and not be the rule.

npmccallum · 2021-08-16T16:32:18Z

I agree with bjorn on all 3 points. Rust makes no stability guarantees about unstable features, and it is allowed and there is precedent to entirely change unstable features. If someone must rely on a nightly feature never changing, they should pin the nightly version to a specific, known working version, and update with caution, until the feature is stabilized.

Agreed completely.

I was not aware that #[naked] functions prevent code duplication or inlining, and because of that implemented this lint unconditionally to match the the RFC. If it is true that #[naked] functions are exempt from the potential issues, they should be granted an exception in this lint, and not be the rule.

I think the main question is who does this work and when. Reverting the PR gives us breathing room to figure this out and coordinate across features.

asquared31415 · 2021-08-16T16:45:05Z

#[naked] applies the same flag as #[inline(never)] for LLVM. So unless there's additional undocumented behavior to naked in LLVM, it is still possible for naked functions to be inlined. The noinline attribute says

This attribute indicates that the inliner should never inline this function in any situation. This attribute may not be used together with the alwaysinline attribute.

I believe that should never is not a guarantee, just a very strong recommendation, so I don't think this can be relied on.

Even if there is that undocumented behavior to the naked attribute that would force never inlining always, how much should we rely on that undocumented behavior.

asquared31415 · 2021-08-16T16:51:19Z

If it does turn out that we can rely on this behavior of #[naked] I will fix the lint to add an exception

npmccallum · 2021-08-16T17:02:52Z

Naked functions MUST NOT ever be inlined. If there is some undefined behavior that causes one to be inlined, it is a bug that should be fixed.

When I disabled inlining for naked functions, I did not implement it as desugaring to #[inline(never)]. In fact, we lint so that mentioning inlining at all on a naked function is disallowed.

asquared31415 · 2021-08-16T17:14:06Z

Then this is a shortcoming of LLVM, it doesn't seem to have a way to specify "no never inline this ever, if you do this is wrong". The best is noinline which as stated above seems to be a very strong hint, and is currently what we pass to LLVM for naked functions, and is the same attribute used for #[inline(never)]

bjorn3 · 2021-08-16T17:19:07Z

I think naked functions can still be inlined if and only if you ensure that you setup all registers and the stack anyway as if you would actually do a real call. This is a silly thing to do, but indistinguishable from a real call except that it would duplicate the inline asm and possibly avoid a single call instruction in favor of falling through to the naked function.

Amanieu · 2021-08-16T18:24:49Z

At this point I'm fairly confident that we can guarantee that asm! in a naked function is never duplicated, so we should just go ahead and disable this lint for asm! in naked functions. If LLVM's behavior changes then we can always switch to lowering naked functions to global_asm! instead.

npmccallum · 2021-08-16T18:25:02Z

@bjorn3 There are possibly other invariants as well (depending on platform). But while it is theoretically possible to inline a naked function, the complexity of this problem is too difficult and with too little gain. Therefore, in the Constrained Naked RFC, we commit the compiler to never inlining a naked function. This may be a promise the compiler cannot (currently) keep. But that might change in the future. And in the meantime, we clearly communicate that no one should ever depend on this behavior and can reliably depend on the opposite behavior.

asquared31415 · 2021-08-16T19:44:07Z

If we are advising users to not depend on naked functions being never inlined, shouldn't this lint still activate then, since the compiler cannot currently promise that?

npmccallum · 2021-08-16T19:49:02Z

@asquared31415 Per the Constrained Naked Functions RFC we are advising users to depend on naked functions never being inlined.

asquared31415 · 2021-08-16T19:53:32Z

Oh I misinterpreted your previous statement then. I thought this behavior meant that they aren't inlined.

I still think it is possible that LLVM changes in the future to actually inline things that are noinline if it really has a good reason to, but since it currently doesn't inline these functions, we could make this exception and handle that scenario if it happens. I'm not sure whether there's any additional rules about duplicating code blocks, but I assume they're similar and that it does not currently happen.

npmccallum · 2021-08-16T19:55:45Z

@asquared31415 I'm sure that if LLVM adds functionality to inline even those functions, the next thing people will ask for is a way to disable it.

haraldh · 2021-08-17T09:16:27Z

Also the linter is triggered on comments:

#[naked]
pub unsafe extern "sysv64" fn _start() -> ! {
    asm!("
    nop          # here is a comment about syscall
    nop          // and another syscall comment
    jmp syscall
    hlt
.global syscall
syscall:
    hlt
    ",
            options(noreturn)
    );
}

gives:

error: avoid using named labels in inline assembly
  --> src/lib.rs:7:44
   |
7  |       nop          # here is a comment about syscall
   |  ____________________________________________^
8  | |     nop          // and another syscall comment
9  | |     jmp syscall
10 | |     hlt
11 | | .global syscall
12 | | syscall:
   | |_______^
   |
   = note: `#[deny(named_asm_labels)]` on by default
   = help: only local labels of the form `<number>:` should be used in inline asm
   = note: see the asm section of the unstable book <https://doc.rust-lang.org/nightly/unstable-book/library-features/asm.html#labels> for more information

Playground

bjorn3 · 2021-08-17T14:52:28Z

The lint triggers on the label definition (syscall:) and then tries to find uses of it using a heuristic. This heuristic skips // comments, but not platform dependent comments. It does not trigger on comments except if the comment contains a ;. It seems that ; is used as statement separator before // comments are stripped. It should have been the opposite way around. First stripping comments and then splitting at ;.

asquared31415 · 2021-08-19T20:41:09Z

@haraldh please file a separate issue about that, it's a lot easier to track the issues that way

I was not aware that platform specific comments existed, I tried to make sure that I wasn't missing platform specific behavior, but must have missed that anyway.

rust-highfive assigned davidtwco Jul 20, 2021

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

rust-highfive assigned Amanieu and unassigned davidtwco Jul 20, 2021

This comment has been minimized.

Sign in to view

asquared31415 force-pushed the named-asm-labels branch from 8f18f8c to 3222c6b Compare July 20, 2021 22:52

bjorn3 reviewed Jul 21, 2021

View reviewed changes

compiler/rustc_builtin_macros/src/asm.rs Show resolved Hide resolved

Amanieu reviewed Jul 21, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

asquared31415 force-pushed the named-asm-labels branch from 28dd531 to 5bed7c8 Compare July 24, 2021 18:56

asquared31415 force-pushed the named-asm-labels branch from 5bed7c8 to 6ca8373 Compare July 29, 2021 22:57

Amanieu reviewed Aug 4, 2021

View reviewed changes

asquared31415 added 8 commits August 4, 2021 13:38

Lint against named asm labels

75915ad

Comment tweaks

1e1f219

Revert accidental removal of attributes

1f8f863

Proper characters in labels, ignore comments

6f45f62

Handle leading colons properly

8e7bbc9

Fix lint capitalization and ignoring, test with include_str

1ae19b6

Update error message

ae8a1ba

Combine spans into one error, deduplicate code

51e414f

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 14, 2021

bors merged commit e55c13e into rust-lang:master Aug 14, 2021

rustbot added this to the 1.56.0 milestone Aug 14, 2021

asquared31415 mentioned this pull request Aug 19, 2021

Exempt #[naked] fns from named_asm_labels lint #88169

Closed

asquared31415 deleted the named-asm-labels branch September 2, 2021 13:54

asquared31415 mentioned this pull request Oct 21, 2021

Compiler null dereference when there are conflicting asm labels #74262

Closed

		"do not use named labels in inline assembly",
		BuiltinLintDiagnostics::NamedAsmLabel("Only GAS local labels of the form `N:` where N is a number may be used in inline asm".to_string()),

Lint against named asm labels #87324

Lint against named asm labels #87324

Conversation

asquared31415 commented Jul 20, 2021

rust-highfive commented Jul 20, 2021

asquared31415 commented Jul 20, 2021

asquared31415 commented Jul 20, 2021

This comment has been minimized.

asquared31415 commented Jul 20, 2021

Amanieu Jul 21, 2021

Choose a reason for hiding this comment

Amanieu Jul 25, 2021

Choose a reason for hiding this comment

asquared31415 Jul 29, 2021

Choose a reason for hiding this comment

This comment has been minimized.

asquared31415 commented Jul 21, 2021

Amanieu commented Jul 22, 2021

bors commented Jul 24, 2021

bors commented Jul 25, 2021

bors commented Aug 4, 2021

Amanieu Jul 30, 2021

Choose a reason for hiding this comment

asquared31415 Aug 4, 2021

Choose a reason for hiding this comment

Amanieu Jul 30, 2021

Choose a reason for hiding this comment

Amanieu commented Aug 4, 2021

npmccallum commented Aug 16, 2021

Amanieu commented Aug 16, 2021

Amanieu commented Aug 16, 2021

npmccallum commented Aug 16, 2021

Amanieu commented Aug 16, 2021

npmccallum commented Aug 16, 2021

bjorn3 commented Aug 16, 2021 • edited Loading

npmccallum commented Aug 16, 2021 • edited Loading

asquared31415 commented Aug 16, 2021 • edited Loading

npmccallum commented Aug 16, 2021

asquared31415 commented Aug 16, 2021 • edited Loading

asquared31415 commented Aug 16, 2021

npmccallum commented Aug 16, 2021

asquared31415 commented Aug 16, 2021 • edited Loading

bjorn3 commented Aug 16, 2021

Amanieu commented Aug 16, 2021

npmccallum commented Aug 16, 2021

asquared31415 commented Aug 16, 2021

npmccallum commented Aug 16, 2021

asquared31415 commented Aug 16, 2021 • edited Loading

npmccallum commented Aug 16, 2021

haraldh commented Aug 17, 2021 • edited Loading

bjorn3 commented Aug 17, 2021

asquared31415 commented Aug 19, 2021

bjorn3 commented Aug 16, 2021 •

edited

Loading

npmccallum commented Aug 16, 2021 •

edited

Loading

asquared31415 commented Aug 16, 2021 •

edited

Loading

asquared31415 commented Aug 16, 2021 •

edited

Loading

asquared31415 commented Aug 16, 2021 •

edited

Loading

asquared31415 commented Aug 16, 2021 •

edited

Loading

haraldh commented Aug 17, 2021 •

edited

Loading