rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.58k stars 12.48k forks source link

Tracking Issue for RFC #2972: Constrained Naked Functions #90957

Open nikomatsakis opened 2 years ago

nikomatsakis commented 2 years ago

This is a tracking issue for the RFC "Constrained Naked Functions" (rust-lang/rfcs#2972). The feature gate for the issue is #![feature(naked_functions)].

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

None.

Implementation history

safinaskar commented 2 years ago

"produce a clear warning if any of the above suggestions are not heeded" in RFC seems to contain typo

bstrie commented 2 years ago

This is implemented, checking the box.

Adding a new step: "Confirm that all errors and warnings are emitted properly".

bstrie commented 2 years ago

Request for Stabilization

A proposal that the naked_functions feature be stabilized for Rust 1.60, to be released 2022-04-07. A stabilization PR can be found at https://github.com/rust-lang/rust/pull/93587.

Summary

Adds a new attribute, #[naked], which may be applied to functions. When applied to a function, the compiler will not emit a function prologue when generating code for this function. This attribute is analogous to __attribute__((naked)) in C. The use of this feature allows the programmer to have precise control over the assembly that is generated for a given function.

The body of a naked function must consist of a single asm! invocation. This asm! invocation is heavily restricted: the only legal operands are const and sym, and the only legal options are noreturn (which is mandatory) and att_syntax. In lieu of specifying operands, the asm! within a naked function relies on the specified extern calling convention in order to determine the validity of registers.

An example of a naked function:

const THREE: usize = 3;

#[naked]
/// Adds three to a number and returns the result.
pub extern "sysv64" fn add_n(number: usize) -> usize {
    // SAFETY: the validity of these registers is guaranteed according to the "sysv64" ABI
    unsafe {
        std::arch::asm!(
            "add rdi, {}",
            "mov rax, rdi",
            "ret",
            const THREE,
            options(noreturn)
        );
    }
}

Documentation

The Rust Reference: https://github.com/rust-lang/reference/pull/1153

Tests

History

This feature was originally proposed in RFC 1201, filed on 2015-07-10 and accepted on 2016-03-21. Support for this feature was added in #32410, landing on 2016-03-23. Development languished for several years as it was realized that the semantics given in RFC 1201 were insufficiently specific. To address this, a minimal subset of naked functions was specified by RFC 2972, filed on 2020-08-07 and accepted on 2021-11-16. Prior to the acceptance of RFC 2972, all of the stricter behavior specified by RFC 2972 was implemented as a series of warn-by-default lints that would trigger on existing uses of the naked attribute; these lints became hard errors in #93153 on 2022-01-22. As a result, today RFC 2972 has completely superseded RFC 1201 in describing the semantics of the naked attribute.

Unresolved Questions

bjorn3 commented 2 years ago

Since most of the utility of naked functions comes from preventing the prologue rather than the epilogue, for the time being this feature has restricted itself to only guaranteeing the absence of the prologue.

There is no observable difference between LLVM adding an epilogue and a following unnamed function starting with instructions identical to a regular epilogue.

bstrie commented 2 years ago

@bjorn3 I believe it can cause problems in circumstances such as https://github.com/rust-lang/rust/issues/32408#issuecomment-898815309 . If someone were to work around that issue by assuming the existence of extra instructions and manually accounting for it, then their code would be broken if Rust ever stopped generating those instructions. It is this sort of edge case that this defensive wording is designed to address.

roblabla commented 2 years ago

Since most of the utility of naked functions comes from preventing the prologue rather than the epilogue, for the time being this feature has restricted itself to only guaranteeing the absence of the prologue.

There is no observable difference between LLVM adding an epilogue and a following unnamed function starting with instructions identical to a regular epilogue.

I believe it's possible to observe a difference when naked is coupled with #[link_section]. You could apply that attributed to a naked function to put it in a well-known section that you expect to only contain your function.

joshtriplett commented 2 years ago

:+1: for stabilizing, though I'd like to make sure that there's consensus on https://github.com/rust-lang/rust/issues/32408#issuecomment-1016931998 .

joshtriplett commented 2 years ago

Shall we stabilize constrained naked functions?

@rfcbot merge

rfcbot commented 2 years ago

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

Amanieu commented 2 years ago

I'd like to have a whitelist of attributes that are allowed to be applies to a naked function. In the future we may want to lower naked functions directly to LLVM module-level assembly instead of using LLVM's native naked function support. For that to happen rustc needs to be able to manually translate any attributes (e.g. #[link_section]) to the corresponding asm directive.

comex commented 2 years ago

In the future we may want to lower naked functions directly to LLVM module-level assembly instead of using LLVM's native naked function support.

Why, because of the epilogue issue? Personally I would much rather see that fixed on LLVM’s end, rather than going to great lengths to work around it in rustc. Or is there another reason?

bjorn3 commented 2 years ago

Lowering to global_asm! would allow all codegen backends to share the same naked function handling. Also codegen backends without native inline assembly support (like cg_clif) will have to lower to global_asm! even if no code is shared between codegen backends, so it will need this same restriction.

bstrie commented 2 years ago

@comex In addition to the epilogue issue, it seems the inlining issue could also be resolved by lowering to global_asm!.

@Amanieu I'd be interested in seeing such a list. Obviously part of the appeal of naked functions is that they can be treated just like a normal function in most cases, which includes the ability to be marked with attributes (e.g. #[doc]), so I'm curious how restrictive this list would be. It occurs to me that, in addition to forbidding #[inline], naked functions also already forbid #[track_caller]; I should probably mention this in the reference.

roblabla commented 2 years ago

Most attributes that work with arbitrary functions should work with naked functions IMO. Taking the list here, those make sense to use with naked fns:

The only ones that I think make sense to disallow are those in the "Code Generation" category: inline, cold, track_caller.

target_feature is an interesting one. I'm not sure what it does, it may make sense to allow it in naked fns.

joshtriplett commented 2 years ago

@roblabla I think cold makes sense on a naked function. It may affect where the code ends up in the final binary, and what it's placed next to, all of which seems fine.

I think target_feature makes a little sense, if you want to use it for its semantic of "you shouldn't reference this function outside of an unsafe block or a function with the same target feature", but it'd take some care to get right; prohibiting target_feature on a naked function seems fine for now until we nail down that semantic.

I agree that inline and track_caller don't make sense on a naked function.

test and related attributes also don't make sense. And must_use doesn't make sense, since the function doesn't actually have a normal return value.

nikomatsakis commented 2 years ago

@joshtriplett

And must_use doesn't make sense, since the function doesn't actually have a normal return value.

Can't naked functions declare a return value if they wish? it's just up to the caller to manage the ABI side of things.

nikomatsakis commented 2 years ago

@rfcbot reviewed

@bstrie thank you for the summary! It answered my main questions.

npmccallum commented 2 years ago

I'm just adding a note as the author of Constrained Naked Functions that I support this direction and I believe all substantial issues are resolved. After this, we should be in a position to improve functionality incrementally as needed in a well defined way. 👍 🚀 🥳

bstrie commented 2 years ago

I think target_feature makes a little sense, if you want to use it for its semantic of "you shouldn't reference this function outside of an unsafe block or a function with the same target feature", but it'd take some care to get right; prohibiting target_feature on a naked function seems fine for now until we nail down that semantic.

I agree that inline and track_caller don't make sense on a naked function.

test and related attributes also don't make sense.

This seems reasonable to me. ignore and track_caller are already forbidden on naked functions. I'll file a PR forbidding the following attributes as well:

I'll also document this in the reference.

Can anyone else think of any other attributes to forbid on naked functions, or is this list complete? I've added a new checkbox to the tracking issue for this concern; if my aforementioned PR is merged and if there are no further objections then I'll consider it resolved.

fintelia commented 2 years ago

Obviously the correctness of naked functions relies on Rust setting up the callstack as if a function call were being performed, even in cases where the function is ultimately inlined.

Are you sure that this actually happens when rustc decides to inline a naked function? In the past, inlining a naked function also skipped setting up the callstack properly.

However, there may be observable differences in behavior based on whether or not a naked function is inlined, e.g. due to exported symbols or named labels.

If I understand correctly, this is equivalent to saying that asm blocks in naked functions may not export symbols because you'd get linker errors if an inlined and non-inlined version were both included in the binary?

bstrie commented 2 years ago

Are you sure that this actually happens when rustc decides to inline a naked function? In the past, https://github.com/rust-lang/rfcs/pull/2774#issuecomment-536145700.

We're at the mercy of LLVM to not inline a naked function. We emit LLVM's noinline attributes on naked functions, which is documented like so: "This attribute indicates that the inliner should never inline this function in any situation." We have a codegen test to ensure that this attribute is emitted properly on naked functions. To my mind that would appear to settle the issue, but some others are concerned that it may not be absolutely bulletproof, so the defensive language you're referring to, while likely overly conservative, is designed to sidestep the need to precisely define "inlining" while still giving users assurance that naked functions will behave as they expect even in the absence of that definition. If we update your linked example we can observe that indeed the function is no longer inlined: https://rust.godbolt.org/z/rvPe4r8no , but if you can come up with a counterexample I'd be happy to see it. Personally I think this is a bit of a quibble (Rust has stabilized features with less reference-level rigor than this before), but I'd rather avoid arguing if it means unblocking this feature from stabilizing.

If I understand correctly, this is equivalent to saying that asm blocks in naked functions may not export symbols because you'd get linker errors if an inlined and non-inlined version were both included in the binary?

I don't believe so, asm blocks already have this issue (any function containing an asm block might be inlined), and even if you use global_asm that doesn't stop an end-user from including two different versions of your crate in their binary (consider LTO as well). Naked functions don't add any new hazard here, if anything they might eventually make it a bit easier since you might be able to leverage the compiler's own mangling scheme to guarantee the uniqueness of global symbols. The legalese here is designed to tell users that, for the moment, any number of copies of the function might exist in the binary, but that in the future the compiler may decide not to include that many copies, and that if you do any bizarre thing that somehow relies on there being more than one copy in the binary then you are outside of the stability guarantee; I find it implausible that anyone could ever write reasonable code that would run afoul of this clause.

fintelia commented 2 years ago

I'd prefer that inlining a naked function was always considered a compiler bug. No one wants it to happen. No one has demonstrated a case where it currently happens. No one has shown that the code exists to configure the callstack properly if it did. At least one case where inlining used to happen didn't do anything to configure the callstack. And if Rust doesn't draw a line in the sand now, it is unclear we'd ever be confident in LLVM's behavior to know make the promise in the future.

The current text is even rather vague about what it means to configure the callstack and registers properly. At a minimum, the program counter register might not equal the function address. But beyond that, some of the Zulip comments suggest that backtraces from inlined functions might not work as expected. Does that mean that the frame pointer (if the ABI specifies one) might hold a misleading value?

The guidance seems to be that if you don't want your function to break when it is inlined, you shouldn't count on "observable differences in behavior". Except that's a circular definition! If you want your function's behavior not to change when inlined, you must make sure your code doesn't behave differently when inlined.

The legalese here is designed to tell users that, for the moment, any number of copies of the function might exist in the binary, but that in the future the compiler may decide not to include that many copies, and that if you do any bizarre thing that somehow relies on there being more than one copy in the binary then you are outside of the stability guarantee; I find it implausible that anyone could ever write reasonable code that would run afoul of this clause.

The case I'm worried about is that the compiler currently inserts exactly one copy of the function and everything works fine, but without warning later breaking things by deciding to include multiple copies. At worst, the old and new versions might both compile without errors or warnings but behave differently at runtime.

bstrie commented 2 years ago

@fintelia I'm happy to tweak the proposed language in the reference (PRs welcome). As mentioned, I think the risk of anything going wrong here is quite low, and plenty of things to do with soon-to-be-stable-asm already feature relatively fragile semantics (e.g. labels and directives) so even in the worst case this would be neither unprecedented nor outside the norm for asm users. However at this point what matters most is 1) what semantics the language team is willing to commit to, and 2) what semantics the compiler team thinks that rustc is reasonably capable of enforcing. I've added a new checkbox for this concern. I encourage you to resurrect the Zulip thread on inlining and raise your concerns there as well.

rfcbot commented 2 years ago

:bell: This is now entering its final comment period, as per the review above. :bell:

rfcbot commented 2 years ago

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

archshift commented 2 years ago

This was marked ready to merge 3 months ago. Has anything changed since then?

joshtriplett commented 2 years ago

I think it just needs a stabilization PR.

https://github.com/rust-lang/rust/pull/93587

Lokathor commented 2 years ago

The RFC says that the asm! "must not contain any other options except att_syntax", but the raw option should also be permitted.

Lokathor commented 2 years ago

Also, having people put noreturn on a thing that generally does return is very confusing design, but the FCP for that is over so I don't expect a bigger update like that.

joshtriplett commented 2 years ago

@Lokathor Both of those can still change after stabilizing naked functions; that's not a blocker.

@bstrie or others: gentle ping on stabilization?

Lokathor commented 2 years ago

There was a very small amount of discussion for this on zulip (in the inline-asm stream). The proposed noreturn fix wasn't just "remove the requirement" (which could be done post-stabilization i guess), but a somewhat stronger "require an alternative asm option specific to naked functions" and possibly even going as far as "make naked_asm a separate macro that emits a global_asm usage". Actually doing either of those would not be fixable post-stabilization.

At the time, bstrie seemed open to the idea of a separate macro specific to naked function usage.

Lokathor commented 2 years ago

update: seems that raw is already accepted as an option to naked function inline assembly, so the docs just need to be fixed whenever.

repnop commented 2 years ago

while talking to @memoryruins about another feature which might interact poorly with LLVM's function deduplication pass (fn_align), it looks like LLVM gladly deduplicates #[naked] functions when the body is the same even in the face of different alignment requirements (which appears to pick the higher alignment between the two at least) on the functions: https://rust.godbolt.org/z/WzWTEqM9d

I can't immediately think of how this would cause issues off the top of my head, and it seems the moment that the asm blocks are not exactly equivalent (if you add a comment, for instance) it won't dedup, but seeing as how there were issues with #54685 I wanted to bring attention to it. cc @RalfJung since you pioneered the fix for the original issue and might have some insight I don't here

my immediate feeling here is that non-inlineable functions shouldn't be be able to be deduplicated

fintelia commented 2 years ago

@repnop If you do binary patching, then the deduplication you're describing will break things. One example: a program that creates two identical naked functions bar and baz and then at runtime overwrites the contents of bar. Any calls to baz are unexpectedly going to have the new behavior.

RalfJung commented 2 years ago

https://github.com/rust-lang/rust/issues/54685 was different, that was a case of inconsistent compilation in the face of function deduplication. This is a case of potentially undesirable deduplication. I have no idea what the intended behavior is here (I leave that to the domain experts), nor how to control LLVM's deduplication (I leave that to the LLVM experts).

@fintelia that argument applies equally to non-naked functions, doesn't it? Cc @Amanieu who IIRC said that binary patching is an intended usecase for asm!. Should all functions containing asm! be exempt from deduplication? (No idea if that is feasible, but that is a separate question.)

Amanieu commented 2 years ago

Binary patching is allowed, but the compiler is still allowed to duplicate an asm! block. The way that would work is that, from within the asm!, you record instruction addresses in a separate section which you can later use to find patchable code blocks.

joshtriplett commented 2 years ago

We reviewed this in today's @rust-lang/lang meeting. We'd love to see this stabilized, and we think it's ready to stabilize.

We do think https://github.com/rust-lang/rust/pull/93809 should go in first, and then https://github.com/rust-lang/rust/pull/93587 .

Lokathor commented 2 years ago

Was there a discussion regarding the noreturn asm option being used on the assembly despite it returning anyway? It has already lead at least two people to misunderstand how that asm option works, we should not stabilize that form.

Amanieu commented 2 years ago

I just published a proc-macro version of #[naked]: https://github.com/Amanieu/naked-function

It works by lowering a naked function to a global_asm! block and an extern "C" function import.

IMO this covers 99% of the use cases for naked functions.

ghost commented 2 years ago

One use case that looks to not (yet?) be covered by that proc-macro is aligned naked functions. This is needed to implement trap/exception/interrupt handlers on some architectures. For example, RISC-V requires direct trap handlers to be aligned to 4 bytes, so I'm currently using this attribute alongside the fn_align feature (#82232) to get the desired effect.

Amanieu commented 2 years ago

The proc macro can handle various attributes on the function and generate the equivalent assembly code sequence. Currently the following attributes are handled:

But it is relatively easy to add support for more attributes such as repr(align) or instruction_set.

bjorn3 commented 2 years ago

That proc macro still has the limitation that it is impossible to handle multiple versions of the same crate. Even two different crates will conflict if they happen to use the same function name, despite not using no_mangle.

Lokathor commented 2 years ago

@bjorn3 would you say that the only entity able to fix that specific problem is the compiler itself? Would it be fixable if some sort of extra API were added to proc-macros so that they could get the correct full name of the crate they're expanding code for and then use that in some way?

I'm not against having an attribute in the compiler, but if it can be just a normal proc-macro that seems convenient from a maintenance stand point. And if it's a proc-macro that has a fairly clear de-sugaring into an existing language feature that's easier to teach. The proc-macro version could presumably eventually be put into core::arch or something like that, so it would still be available in a basic program with no deps, same as the attribute form.

bjorn3 commented 2 years ago

Would it be fixable if some sort of extra API were added to proc-macros so that they could get the correct full name of the crate they're expanding code for and then use that in some way?

It would need to be the crate name, stable crate id and module path. In addition even that is not enough if we want to support generics.

The proc-macro version could presumably eventually be put into core::arch or something like that, so it would still be available in a basic program with no deps, same as the attribute form.

The standard library can't use or expose proc macros unless you want to break cross compilation due to the proc macro not being compiled for the right target. (cross compiling crates using rustc_driver is already impossible because of this issue. even cross compiling rustc itself requires the -Zdual-proc-macro hack) All non-decl macros exported by the standard library have to be built into rustc.

Lokathor commented 2 years ago

Oh. Well, I guess that moves my vote back to the attribute. It's really gotta be something that works "out of the box", and it definitely has to work with cross compilation.

Amanieu commented 2 years ago

That proc macro still has the limitation that it is impossible to handle multiple versions of the same crate. Even two different crates will conflict if they happen to use the same function name, despite not using no_mangle.

It's somewhat possible to do this by hashing the CARGO_PKG_VERSION and CARGO_PKG_NAME environment variables into the symbol name. It might required proc_macro::tracked_env to be stabilized though.

bjorn3 commented 2 years ago

Even that doesn't work for crates also used by the standard library. They have the same name and version, but a different -Cmetadata value and thus different stable crate id.

roblabla commented 2 years ago

Honestly, I kind of wish rust had a macro that allowed creating a mangled name out of a string "as if" it was here, e.g. mangled_name!("test") would return the same mangled name as if a function/static had been created with that name here. In fact, this is kind of why I really like naked functions: they essentially allow writing asm that integrates with the name mangling scheme.

hudson-ayers commented 1 year ago

Is the current status of stabilizing this that someone needs to open and ultimately merge a new/updated version of https://github.com/rust-lang/rust/pull/93809 ? From what I can tell, this was ready for stabilization modulo #93809 , but it fell out of date.

Has this changed? I searched zulip and did not see anything indicating that a different path was preferred. I am trying to figure out whether there is still a path forward to naked functions being stabilized.

Amanieu commented 1 year ago

I have reservations about stabilizing naked functions as they are. My main concern is that they are a massive kludge on the codegen side, needing to generate very specific LLVM IR to make it work as expected. I would prefer if naked functions were re-implemented on top of global_asm!. This could be as a proc macro, like my naked-function crate, or it could be implemented in rustc itself (which would allow for generic naked functions).