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

Add naked attribute #29189

Closed
wants to merge 6 commits into from
Closed

Add naked attribute #29189

wants to merge 6 commits into from

Conversation

jackpot51
Copy link
Contributor

Description:
The naked attribute allows functions to be defined that do not have standard wrappers, like stack frames, local variables, or a function return.

This change exposes the LLVM Naked function attribute by using the #[naked] attribute on a Rust function.

Because it is unsafe to use, it is feature gated behind #![feature(naked)]. I have already verified that Rust builds correctly with this change.

Rationale:
The current rustc provides no mechanism for defining these functions correctly, which is why this change is required for operating systems development in Rust, such as the development of Redox.

This is highly important when designing operating systems, due to requirements for interrupt handlers and context switching, or other functions like green threads, virtual machines, and emulators. These require predictable assembly output, without any wrapping instructions. This has limited some features to only being possible in external assembly or C.

http://wiki.osdev.org/Interrupt_Service_Routines#Naked_Functions

Example:
A very simple example of use, inside an interrupt handler:

#![feature(naked)]

#[naked]
#[cfg(target_arch = "x86")]
fn interrupt_handler(){
    asm!("iretd");
}

Which will generate:

interrupt_handler:
   iretd

This is more correct than the current operation:

#[cfg(target_arch = "x86")]
fn interrupt_handler(){
    asm!("iretd");
}

Which will generate something similar to:

interrupt_handler:
   push ebp
   mov ebp, esp
   iretd
   ret

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (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. 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.

@ticki
Copy link
Contributor

ticki commented Oct 20, 2015

Big 👍 here. Makes interrupt handling easier.

@arcnmx
Copy link
Contributor

arcnmx commented Oct 20, 2015

I was just about to do this myself, +1!

@hauleth
Copy link
Contributor

hauleth commented Oct 20, 2015

👍

3 similar comments
@NobbZ
Copy link

NobbZ commented Oct 20, 2015

👍

@tedsta
Copy link
Contributor

tedsta commented Oct 20, 2015

+1

@ambaxter
Copy link

+1

@nagisa
Copy link
Member

nagisa commented Oct 20, 2015

See rust-lang/rfcs#1275 and rust-lang/rfcs#1201.

I’m not against landing this gated and seeing how things work out, personally. The danger of it becoming de facto solution to interrupts (even, when gated) is tremendous, though.

@LazyOxen
Copy link

+1

@jackpot51
Copy link
Contributor Author

This is the simplest, most correct method of achieving this missing functionality. It directly exposes the LLVM Naked attribute the same exact way that the Unwind attribute is exposed, and in just 20 lines of code.

The change is correctly documented, which, by the way, #[unwind] does not have complete documentation.

The style matches the Rust coding standards perfectly.

It aligns with the goals of making Rust a systems level language replacement for C and assembler.

It solves what many people have been requesting, such as rust-lang/rfcs#1275, not just in https://github.com/redox-os/redox, but in https://github.com/ryanra/RustOS, https://github.com/zonyitoo/coio-rs, and other big projects.

There is nothing wrong with this change, it solves a very important issue in the simplest way possible and many important projects will continue to struggle without it.

@arcnmx
Copy link
Contributor

arcnmx commented Oct 20, 2015

Mm, I don't really understand why there's been so much pushback on this feature previously... It's a clean standard way to give programs control over calling conventions. Inherently unsafe, sure, but you can build (safer) abstractions over it to do whatever you need.

@nagisa
Copy link
Member

nagisa commented Oct 20, 2015

@jackpot51 @arcnmx opinion of such sort is better suited on RFCs rather than on this PR. You should post them there.

@jackpot51
Copy link
Contributor Author

@nagisa I don't feel like being shut down in a RFC because people cannot visualize the change. I have working code that executes this change, and therefore all that has to happen is a single click on "Merge pull request".

You had posted your own opinion here before editing it out and then back in:

See rust-lang/rfcs#1275 and rust-lang/rfcs#1201.
I’m not against landing this gated and seeing how things work out, personally.
The danger of it becoming de facto solution to interrupts (even, when gated) is tremendous, though.

@nagisa
Copy link
Member

nagisa commented Oct 20, 2015

You had posted your own opinion here before editing it out and then back in:

The only thing I’ve done in the sole edit that comment had is adding the opinion and a link to the second RFC.

I don't feel like being shut down in a RFC because people cannot visualize the change.

Linking to the PR/branch (and providing some examples, not unlike in first PR comment) should help with that, shouldn’t it? Keeping the discussion in a single place is very important, so people (and the teams evaluating the RFCs in question) don’t have to go running around looking for breadcrumbs all over the internet. Comments, opinions and preferences from the target audience (in this specific case – kernel developers, such as yourselves) is even more important, and oftentimes influences the decision on the RFC strongly.

Given these points, do you still find the request to at least copy-paste your comments over to the RFC, unreasonable?

@jackpot51
Copy link
Contributor Author

I am curious what @nrc, who was assigned to this request, thinks about it. Yeah, let's go to the RFC

@jackpot51
Copy link
Contributor Author

Do you want to use rust-lang/rfcs#1275 or should I create a new one?

@ghost
Copy link

ghost commented Oct 20, 2015

👍 from me as well. This is essential for our redox::env::args() function to have the applications run correctly.

@nagisa
Copy link
Member

nagisa commented Oct 20, 2015

rust-lang/rfcs#1275 should be fine.

@jackpot51
Copy link
Contributor Author

@nagisa Before you go, how long do these things realistically take to merge in? Just so I can make a decision about using external ASM

@nagisa
Copy link
Member

nagisa commented Oct 20, 2015

RFCs have to at least go through the week-long final comment period, however I see no reason (other than rust-lang/rfcs#1201 having been closed) that blocks experimentally landing this PR without a decision on RFC. PRs still take about a day to go through the queue and buildbots once approved.

I pinged some people from compiler team to take a look at the PR sooner.

@jackpot51
Copy link
Contributor Author

Thanks about that, I will have more examples to post on the RFC after switching Redox over to #[naked].

Getting some other Redox contributors to compile these changes as well, and comment on their experiences.

@eefriedman
Copy link
Contributor

It should be an error to write a naked function which contains anything other than an asm!() block without any operands (with maybe an exception for an unsafe { } block).

Does the naked attribute work correctly for functions which have arguments or a non-void return type? We should either make it work correctly (make sure we don't generate code to store arguments or return values into temporaries) or make it an error.

Also, this PR doesn't have any tests; at the very least, there should be a test to make sure we actually generate the expected LLVM IR for naked functions.

@jackpot51
Copy link
Contributor Author

@eefriedman If you point me to where the tests for cold, unwind, or inline are, I would be happy to write some tests.

I think the tests would resolve your other questions.

@eefriedman
Copy link
Contributor

@jackpot51 https://github.com/rust-lang/rust/blob/master/src/test/codegen/ is where tests which check LLVM IR go.

@nrc
Copy link
Member

nrc commented Oct 20, 2015

This was proposed as an RFC and closed (rust-lang/rfcs#1201). We'd need a new RFC in order to accept this into the language. As noted above, rust-lang/rfcs#1275 is still open, that (or a new RFC) would need to be accepted before merging this PR.

Having said that, thanks for doing the implementation work - having an implementation, even if not merged, makes accepting an RFC more likely. If you think it is necessary to get experience with the feature before landing the RFC, then a case should be made in the RFC comments, note that this would have to be really exceptional.

@nrc nrc closed this Oct 20, 2015
@jackpot51
Copy link
Contributor Author

@eefriedman I added the tests
https://github.com/redox-os/rust/blob/a9fe6913d9d94202c8bd9bac8c8149351ee57273/src/test/codegen/naked-functions.rs

By the way, arguments and returns are supported.

@jackpot51
Copy link
Contributor Author

@nrc You should reopen this PR as it still applies if the RFC were to be accepted.

@nrc
Copy link
Member

nrc commented Oct 21, 2015

@jackpot51 we can reopen if/when the RFC is accepted - better to encourage discussion there and to keep a lid on the number of open PRs to manage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet