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

RFC 1201 ammendments: Naked function corrections #2774

Open
wants to merge 2 commits into
base: master
from

Conversation

@alistair23
Copy link

commented Sep 27, 2019

This PR does two things both related to naked functions. Check the individual commits for more details.

@fintelia

This comment has been minimized.

Copy link

commented Sep 27, 2019

My personal preference is for option 2, but only for for extern "C" functions. Calling naked functions with arguments from other Rust code otherwise becomes much harder. I'm not actually sure how it could work. Combining the naked function definition with a separate extern declaration containing the real argument list seems hypothetically possible but doesn't really seem like an improvement?

If we're tweaking the rules for naked functions, we should probably also say that the compiler is no longer allowed to inline them.

@alistair23

This comment has been minimized.

Copy link
Author

commented Sep 27, 2019

My personal preference is for option 2, but only for for extern "C" functions.

Is is possible to pass that information down to LLVM to have it handle that?

Calling naked functions with arguments from other Rust code otherwise becomes much harder.

Yes, this will be difficult to do, but from my understanding of the current RFC that is discouraged anyway.

Combining the naked function definition with a separate extern declaration containing the real argument list seems hypothetically possible but doesn't really seem like an improvement?

Yeah, I guess that is possible.

If we're tweaking the rules for naked functions, we should probably also say that the compiler is no longer allowed to inline them.

Good idea, I'll add an extra commit to this PR with that.

@alistair23 alistair23 changed the title RFC 1201 ammendment: Mark naked functions with arguments as not allowed RFC 1201 ammendments: Naked function arguments and inlining Sep 27, 2019
@alistair23 alistair23 force-pushed the alistair23:alistair/naked-args branch from fe5b4b4 to e540557 Sep 27, 2019
@alistair23

This comment has been minimized.

Copy link
Author

commented Sep 27, 2019

I have added a change about inlining naked functions

@comex

This comment has been minimized.

Copy link

commented Sep 27, 2019

Naked functions with arguments are useful. For instance, to do a system call:

#[naked]
pub unsafe fn stat(buf: *const u8, buf: *const u8) {
    asm!("mov $0x20000bc, %%eax; syscall");
}

I don't see a reason to ban them. At most, rustc could emit a warning until the LLVM bug has been confirmed fixed.

@alistair23

This comment has been minimized.

Copy link
Author

commented Sep 27, 2019

LLVM specifically specifies:

Don't allow non-asm statements in naked functions
Don't allow asm statements to refer to parameters in naked functions.

https://reviews.llvm.org/D5183

so although it seems useful I don't think it should be supported.

@alistair23

This comment has been minimized.

Copy link
Author

commented Sep 27, 2019

I am open to a warning though, which would allow people who really want to have arguments use them but discourage people from doing it.

Also, while we are changing everything should we fix the unresolved question of allowing non asm! functions? It also seems like a bad idea that LLVM doesn't support.

@alistair23 alistair23 force-pushed the alistair23:alistair/naked-args branch from e540557 to a747299 Sep 27, 2019
@alistair23

This comment has been minimized.

Copy link
Author

commented Sep 27, 2019

I have updated the PR to indicate that naked functions will never be inlined as the LLVM code won't inline a naked function. This makes that information explicit.

https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CodeGenModule.cpp#L1539

@fintelia

This comment has been minimized.

Copy link

commented Sep 28, 2019

The current compiler is definitely willing to inline a naked function: https://rust.godbolt.org/z/n9JvSL

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

#[naked]
extern "C" fn foo() {
    unsafe { asm!("int3") }
}

pub fn main() {
    foo();
}

Produces this assembly:

example::main:
        int3
        ret
@comex

This comment has been minimized.

Copy link

commented Sep 28, 2019

LLVM specifically specifies:

Don't allow non-asm statements in naked functions
Don't allow asm statements to refer to parameters in naked functions.

You don't refer to the parameters explicitly (i.e. naming then in constraints). Instead, the assembly code in the inline asm block would expect them to be in certain registers as per the platform ABI. Normally you wouldn't be allowed to assume anything about register state from an inline asm block, but the point of naked functions is that the contents of the block make up the entire body of the function; the compiler won't do any register manipulation behind your back. Or at least, it's not supposed to.

@comex

This comment has been minimized.

Copy link

commented Sep 28, 2019

I have updated the PR to indicate that naked functions will never be inlined as the LLVM code won't inline a naked function. This makes that information explicit.

https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CodeGenModule.cpp#L1539

The code you linked is from Clang, not LLVM proper. It sounds like rustc should be doing the same thing (adding the noinline attribute in addition to the naked attribute), but it currently isn't. :)

Edit: Which corresponds to the bug report that @fintelia linked.

@alistair23

This comment has been minimized.

Copy link
Author

commented Sep 28, 2019

Ok, I'm going to drop the arguments change and do some more research. It sounds like arguments should be supported and there is a bug somewhere in the naked function handling, either in rust or LLVM.

On the other hand function arguments shouldn't be directly accessed and I think naked functions should never be inlined.

I'll update the patches with those changes.

@roblabla

This comment has been minimized.

Copy link

commented Sep 28, 2019

If naked functions should only have a single asm statement, and shouldn't be inlined, what's the difference between them and global_asm? It seems to me that they could be desuggared simply:

// Original:
#[naked]
pub unsafe fn stat(buf: *const u8, buf: *const u8) {
    asm!("mov $0x20000bc, %%eax; syscall");
}

// Desuggared:
global_asm! {
"""
.global mangled_stat
mangled_stat:
   mov $0x20000bc, %%eax; syscall
"""
}

extern {
    #[link_name = "mangled_stat"]
    fn stat(buf: *const u8, buf: *const u8);
}

This would sidestep the asm bug, and further enforce the rule that Naked functions must only contain a single asm call.

@comex

This comment has been minimized.

Copy link

commented Sep 29, 2019

There's not much difference. Indeed, naked functions may not be truly necessary to have as a feature, but some people apparently prefer them over global_asm for reasons that are not entirely clear to me. I can think of a few concrete advantages:

  • Compiler handles name mangling
  • Compiler handles linking (.globl may not be the right declaration, e.g. if you want a hidden visibility symbol inside a shared object)
  • Can be combined with generics to create multiple variations of a function
    • but it might be better to just use assembly macros for that
@fintelia

This comment has been minimized.

Copy link

commented Sep 29, 2019

I think that @roblabla's example shows a couple more reasons why people prefer naked functions:

  • The desuggared way to write the function is over twice as long (and is almost entirely boilerplate)
  • It is rather non-obvious from any of the documentation I read that global_asm + extern block + link_name is the correct incantation. I didn't know that would work prior to reading this thread while the #[naked] annotation "just works" with minimal guessing.
  • Having a separate extern block to control the signature of the function defined in the global asm block feels a lot like action at a distance.
alistair23 added 2 commits Sep 27, 2019
clang will automatically set the NoInline attribute for naked functions
and the NoInline attribute wins over AlwaysInline attribute. Let's
follow their lead and specify the same requiremet.

As the compiller can't safely change/remove the prologue and epilogue
for naked functions when it inlines them inlining the naked functions
could result in strange bugs.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Following the requirements of clang/LLVM let's do two things:
 - Require naked functions to only contain inline assembly
 - Require naked functions to not reference the parameters

As the compiler doesn't make any guarentees about the stack the two
above points would be unsafe to allow.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
@alistair23 alistair23 force-pushed the alistair23:alistair/naked-args branch from a747299 to ce06be6 Sep 30, 2019
@alistair23 alistair23 changed the title RFC 1201 ammendments: Naked function arguments and inlining RFC 1201 ammendments: Naked function corrections Sep 30, 2019
@alistair23

This comment has been minimized.

Copy link
Author

commented Sep 30, 2019

I have updated the PR to drop the argument removal requirement and to add some new ones. Let me know what you think.

@alistair23

This comment has been minimized.

Copy link
Author

commented Oct 7, 2019

Thoughts on the new commits?

feature as having "very system-specific consequences", which the programmer must
be aware of.
storage for local variable bindings, including anything but inline assembly
inside a naked function is not allowed. Refering to the parameters inside of a

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Oct 8, 2019

This seems to suggest that you can have a naked function with parameters, but you can't use them. This seems incorrect - parameters will always have an implicit use due to the generated drop calls. Shouldn't naked functions be forbidden from taking any parameters?

This comment has been minimized.

Copy link
@alistair23

alistair23 Oct 8, 2019

Author

That was my original intention, but passing arguments still makes sense as you can reference them directly from the registers in the assembly.

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Oct 8, 2019

That would require rust to generate a prologue, though - which naked functions are supposed to avoid.

This comment has been minimized.

Copy link
@roblabla

roblabla Oct 8, 2019

No, it doesn't require rust to generate a prologue, it requires the ASM author to know about the calling convention (which is the case for extern C functions) and properly handle it themselves.

drop calls are a good point. I guess arguments should be !Drop?

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Oct 8, 2019

@roblabla: What if the calling convention requires generating a prologue? I don't think a naked fn should cause Rust to generate some weird subset of the normal ASM for a function.

Note that it's not sufficient to require arguments to be !Drop - they also must not have any drop glue (e.g. struct MyStruct(String) is !Drop but has drop glue).

I really don't see passing parameters to naked functions as being useful, or even very well-defined.

This comment has been minimized.

Copy link
@roblabla

roblabla Oct 8, 2019

Then they should be Copy - which AFAIU denies the drop glue. This is similar to the restrictions union currently have - unions may not have items that are Drop, so as an approximation they only allow Copy types.

Passing parameters to naked function that have a known ABI (such as extern "C") is well-defined - that's sort of the whole point of those ABIs. If a CC requires a prologue as part of the ABI, then it's up to the person writing the naked function to insert that prologue. This is similar to defining functions with global_asm!.

People have further shown that they are useful - for instance look at this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.