Exempt `#[naked]` fns from `named_asm_labels` lint #88169

asquared31415 · 2021-08-19T20:40:25Z

#[naked] fns are guaranteed to never be inlined or duplicated so the named_asm_labels lint was over-eagerly and incorrectly warning about named labels that would never be a problem within them.

Fixes an issue introduced with the lint PR #87324

r? @Amanieu
cc @bjorn3 @npmccallum

npmccallum · 2021-08-19T20:43:17Z

@asquared31415 Thank you! This is helpful to me. (FYI @haraldh)

nbdd0121

Did you account for this case?

#![feature(asm)]
#![feature(naked_functions)]

#[naked]
extern "C" fn naked() {
    fn non_naked() {
        
    }
    unsafe { asm!("/* named labels? */", options(noreturn)) };
}

compiler/rustc_builtin_macros/src/asm.rs

compiler/rustc_ast/src/ast.rs

asquared31415 · 2021-08-19T22:02:46Z

No I forgot to account for nested fns, despite even thinking about it and almost adding a test, then forgetting to for some reason. Is there a better way to fix this than a stack of flags that is pushed to in check_item for fns and popped from in check_item_post for fns?

nbdd0121 · 2021-08-19T22:17:38Z

No I forgot to account for nested fns, despite even thinking about it and almost adding a test, then forgetting to for some reason. Is there a better way to fix this than a stack of flags that is pushed to in check_item for fns and popped from in check_item_post for fns?

I am unsure about what's the best way to do it in early lint pass. If this is done in a late lint pass, then you should be able to use tcx.get_attrs.

compiler/rustc_lint/src/builtin.rs

Amanieu · 2021-08-19T22:43:05Z

compiler/rustc_ast/src/ast.rs

@@ -2028,10 +2028,12 @@ pub enum InlineAsmOperand {
 #[derive(Clone, Encodable, Decodable, Debug)]
 pub struct InlineAsm {
    pub template: Vec<InlineAsmTemplatePiece>,
+    pub template_strs: Box<[(Symbol, Option<String>, Span)]>,


This can just be a Vec, it's not like any of the other Vecs in here are ever modified either.

With the above is_local bool, a Vec was increasing the total size of the ItemKind enum and causing an assert to fail. While I could have bumped the size of that assert, it seemed like a bad idea to increase the size of that enum for a very infrequent use case for performance concerns. The Box<[]> was meant to reduce that size since I didn't need to be able to mutate it once it was created.

However with the removal of is_local, I can probably make that a Vec if you think that's clearer/better.

Amanieu · 2021-08-19T22:43:26Z

compiler/rustc_ast/src/visit.rs

@@ -19,20 +19,20 @@ use crate::token;
 use rustc_span::symbol::{Ident, Symbol};
 use rustc_span::Span;

-#[derive(Copy, Clone, PartialEq)]


Leftover debug code?

Amanieu · 2021-08-19T22:43:48Z

compiler/rustc_builtin_macros/src/asm.rs

@@ -7,10 +7,10 @@ use rustc_errors::{Applicability, DiagnosticBuilder};
 use rustc_expand::base::{self, *};
 use rustc_parse::parser::Parser;
 use rustc_parse_format as parse;
-use rustc_session::lint::{self, BuiltinLintDiagnostics};
+use rustc_session::lint::{self};


I'm surprised rustfmt isn't complaining about this.

Suggested change

use rustc_session::lint::{self};

use rustc_session::lint;

That's quite odd. I definitely have the stage0 rustfmt and the tidy commit hook, and would have expected CI to yell as well. Probably a rustfmt bug/oversight?

Update: rust-analyzer has a hint and automatic fix for this, but it's one of the small and hard to notice suggestions, and not applied by rustfmt

asquared31415 · 2021-08-20T02:42:25Z

Moved to a late lint pass over the HIR. It simplifies the logic for handling whether the containing fn is #[naked] greatly from the stack-based solution that I had.

compiler/rustc_lint/src/builtin.rs

nbdd0121 · 2021-08-20T02:52:47Z

It seems that there are quite a few lints in builtin.rs. Does anyone know what's the rationale for that?

tmiasko · 2021-08-20T12:31:03Z

In the current implementation, local symbol names are likely to remain problematic even for naked functions. For example in LTO builds ensuring that names are unique within each crate is not sufficient to avoid duplicates.

npmccallum · 2021-08-20T14:01:28Z

@tmiasko That appears to be true for both gcc and clang who fail to compile this due to symbol redefinition. NASM however, compiles this successfully. This is unfortunate...

loop1:
.Lloop:
    jmp .Lloop

loop2:
.Lloop:
    jmp .Lloop

main:
    jmp loop1
    jmp loop2

Amanieu · 2021-08-20T17:01:23Z

In the current implementation, local symbol names are likely to remain problematic even for naked functions. For example in LTO builds ensuring that names are unique within each crate is not sufficient to avoid duplicates.

That's a good point: if you use .Lfoo in one place, you cannot use that label name anywhere else in the whole program, including in other crates.

For this reason I prefer only using numbered local labels except when explicitly exporting a symbol.

Amanieu · 2021-08-20T17:01:58Z

compiler/rustc_lint/src/builtin.rs

+                        }
+                    }
+
+                    debug!("NamedAsmLabels::check_expr(): found_labels: {:#?}", &found_labels);


More leftover debugging code.

npmccallum · 2021-08-20T17:03:28Z

In the current implementation, local symbol names are likely to remain problematic even for naked functions. For example in LTO builds ensuring that names are unique within each crate is not sufficient to avoid duplicates.

That's a good point: if you use .Lfoo in one place, you cannot use that label name anywhere else in the whole program, including in other crates.

For this reason I prefer only using numbered local labels except when explicitly exporting a symbol.

I accept that this unfortunate restriction is unlikely to be fixed soon. So I think we have no choice but to restrict this even on naked functions.

asquared31415 · 2021-08-20T17:54:49Z

It's trivial to cause the error even without LTO as well, simply use the same label name twice in two different #[naked] functions. I think that because of this, we need to leave the lint as is, and users that are certain that their usage is correct and willing to accept the risks of using named labels should just set the lint to allow or warn as needed.

nbdd0121 · 2021-08-20T17:57:07Z

Regardless I think it's reasonable to move the check to a lint pass. Especially since it adds span info to InlineAsm so it allows people to write custom lints for their assemblies if they want to.

asquared31415 · 2021-08-20T18:10:05Z

allows people to write custom lints for their assemblies if they want to.

Could you expand on this? Do you mean in for example a proc macro using the AST? I agree that it seems nicer to handle things as a lint using the AST or HIR (I personally like the HIR better), but I'm curious of the use case you're imagining, and would like to make it as good as possible for those cases.

nbdd0121 · 2021-08-20T19:17:44Z

Could you expand on this? Do you mean in for example a proc macro using the AST? I agree that it seems nicer to handle things as a lint using the AST or HIR (I personally like the HIR better), but I'm curious of the use case you're imagining, and would like to make it as good as possible for those cases.

No, proc macros can't see ASTs, they can only see token trees. I meant custom lints using plugins or clippy lints.

asquared31415 · 2021-08-20T19:20:18Z

ah right

Amanieu · 2021-08-20T20:01:46Z

If you are interested, #81088 (comment) discusses some option for allowing named labels in asm!.

As for this PR, I agree that we should keep the lint enabled for naked functions, but it may still be beneficial to move this lint to HIR instead of macro expansion.

asquared31415 · 2021-08-20T20:18:20Z

Alright, I'll close this and reopen without the exception as a refactor

…ctor, r=Amanieu Refactor `named_asm_labels` to a HIR lint As discussed on rust-lang#88169, the `named_asm_labels` lint could be moved to a HIR lint. That allows future lints or custom plugins or clippy lints to more easily access the `asm!` macro's data and create better error messages with the lints.

rust-highfive assigned Amanieu Aug 19, 2021

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

asquared31415 force-pushed the naked-named-asm-exception branch from 2b6ffb3 to f8087e1 Compare August 19, 2021 20:53

nbdd0121 reviewed Aug 19, 2021

View reviewed changes

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

compiler/rustc_ast/src/ast.rs Outdated Show resolved Hide resolved

Amanieu reviewed Aug 19, 2021

View reviewed changes

asquared31415 force-pushed the naked-named-asm-exception branch from f8087e1 to ded8f01 Compare August 20, 2021 02:41

asquared31415 force-pushed the naked-named-asm-exception branch from ded8f01 to 36807ae Compare August 20, 2021 02:50

nbdd0121 reviewed Aug 20, 2021

View reviewed changes

compiler/rustc_lint/src/builtin.rs Outdated Show resolved Hide resolved

asquared31415 force-pushed the naked-named-asm-exception branch from 36807ae to 81dd8ac Compare August 20, 2021 03:02

Exempt #[naked] fns from named_asm_labels lint

a74f19a

asquared31415 force-pushed the naked-named-asm-exception branch from 81dd8ac to a74f19a Compare August 20, 2021 03:22

Amanieu reviewed Aug 20, 2021

View reviewed changes

asquared31415 closed this Aug 20, 2021

asquared31415 mentioned this pull request Aug 20, 2021

Refactor named_asm_labels to a HIR lint #88196

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exempt `#[naked]` fns from `named_asm_labels` lint #88169

Exempt `#[naked]` fns from `named_asm_labels` lint #88169

asquared31415 commented Aug 19, 2021

npmccallum commented Aug 19, 2021

nbdd0121 left a comment

asquared31415 commented Aug 19, 2021

nbdd0121 commented Aug 19, 2021

Amanieu Aug 19, 2021

asquared31415 Aug 20, 2021

Amanieu Aug 19, 2021

Amanieu Aug 19, 2021

asquared31415 Aug 20, 2021

asquared31415 Aug 20, 2021

asquared31415 commented Aug 20, 2021

nbdd0121 commented Aug 20, 2021

tmiasko commented Aug 20, 2021

npmccallum commented Aug 20, 2021

Amanieu commented Aug 20, 2021

Amanieu Aug 20, 2021

npmccallum commented Aug 20, 2021

asquared31415 commented Aug 20, 2021

nbdd0121 commented Aug 20, 2021

asquared31415 commented Aug 20, 2021

nbdd0121 commented Aug 20, 2021

asquared31415 commented Aug 20, 2021

Amanieu commented Aug 20, 2021

asquared31415 commented Aug 20, 2021

Exempt #[naked] fns from named_asm_labels lint #88169

Exempt #[naked] fns from named_asm_labels lint #88169

Conversation

asquared31415 commented Aug 19, 2021

npmccallum commented Aug 19, 2021

nbdd0121 left a comment

Choose a reason for hiding this comment

asquared31415 commented Aug 19, 2021

nbdd0121 commented Aug 19, 2021

Amanieu Aug 19, 2021

Choose a reason for hiding this comment

asquared31415 Aug 20, 2021

Choose a reason for hiding this comment

Amanieu Aug 19, 2021

Choose a reason for hiding this comment

Amanieu Aug 19, 2021

Choose a reason for hiding this comment

asquared31415 Aug 20, 2021

Choose a reason for hiding this comment

asquared31415 Aug 20, 2021

Choose a reason for hiding this comment

asquared31415 commented Aug 20, 2021

nbdd0121 commented Aug 20, 2021

tmiasko commented Aug 20, 2021

npmccallum commented Aug 20, 2021

Amanieu commented Aug 20, 2021

Amanieu Aug 20, 2021

Choose a reason for hiding this comment

npmccallum commented Aug 20, 2021

asquared31415 commented Aug 20, 2021

nbdd0121 commented Aug 20, 2021

asquared31415 commented Aug 20, 2021

nbdd0121 commented Aug 20, 2021

asquared31415 commented Aug 20, 2021

Amanieu commented Aug 20, 2021

asquared31415 commented Aug 20, 2021

Exempt `#[naked]` fns from `named_asm_labels` lint #88169

Exempt `#[naked]` fns from `named_asm_labels` lint #88169