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

remove support for the (unstable) #[start] attribute #134299

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Dec 14, 2024

As explained by @Noratrieb:
#[start] should be deleted. It's nothing but an accidentally leaked implementation detail that's a not very useful mix between "portable" entrypoint logic and bad abstraction.

I think the way the stable user-facing entrypoint should work (and works today on stable) is pretty simple:

  • std-using cross-platform programs should use fn main(). the compiler, together with std, will then ensure that code ends up at main (by having a platform-specific entrypoint that gets directed through lang_start in std to main - but that's just an implementation detail)
  • no_std platform-specific programs should use #![no_main] and define their own platform-specific entrypoint symbol with #[no_mangle], like main, _start, WinMain or my_embedded_platform_wants_to_start_here. most of them only support a single platform anyways, and need cfg for the different platform's ways of passing arguments or other things anyways

#[start] is in a super weird position of being neither of those two. It tries to pretend that it's cross-platform, but its signature is a total lie. Those arguments are just stubbed out to zero on Windows wasm, for example. It also only handles the platform-specific entrypoints for a few platforms that are supported by std, like Windows or Unix-likes. my_embedded_platform_wants_to_start_here can't use it, and neither could a libc-less Linux program.
So we have an attribute that only works in some cases anyways, that has a signature that's a total lie (and a signature that, as I might want to add, has changed recently, and that I definitely would not be comfortable giving any stability guarantees on), and where there's a pretty easy way to get things working without it in the first place.

Note that this feature has not been RFCed in the first place.

This comment was posted in May and so far nobody spoke up in that issue with a usecase that would require keeping the attribute.

Closes #29633

@rustbot
Copy link
Collaborator

rustbot commented Dec 14, 2024

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 14, 2024
@RalfJung RalfJung changed the title remove support for the #[start] attribute remove support for the (unstable) #[start] attribute Dec 14, 2024
@RalfJung RalfJung added the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 14, 2024
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Dec 14, 2024

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@RalfJung RalfJung added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Dec 14, 2024
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 14, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

@Noratrieb

Those arguments are just stubbed out to zero on Windows, for example.

Given that we have several tests that inspect these arguments and that apparently pass on Windows, this cannot be entirely true.

@rust-log-analyzer

This comment has been minimized.

@ChrisDenton
Copy link
Member

Given that we have several tests that inspect these arguments and that apparently pass on Windows, this cannot be entirely true.

There is a difference here between #[lang = "start"] and #[start]. The former we use zeros, the latter we grab them from the C main function.

@Noratrieb
Copy link
Member

The start stuff is very confusing, it's possible I got that wrong yeah. Removing #[start] makes it simpler :)

@RalfJung
Copy link
Member Author

There is a difference here between #[lang = "start"] and #[start]. The former we use zeros, the latter we grab them from the C main function.

Where is this implemented?

@ChrisDenton
Copy link
Member

fn get_argc_argv<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(bx: &mut Bx) -> (Bx::Value, Bx::Value) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have codegen/mainsubprogram.rs which tests that this works for regular binaries.

@RalfJung
Copy link
Member Author

fn get_argc_argv<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(bx: &mut Bx) -> (Bx::Value, Bx::Value) {

That function acts depending on target.main_needs_argc_argv, not depending on whether we use #[start] or the start lang item. So this doesn't seem to match what you said earlier?

main_needs_argc_argv is set by default for most targets. The exception are some headless ARM targets and wasm targets.

@ChrisDenton
Copy link
Member

Huh, you're right. I'm 99% sure we used to but maybe that was a long time ago.

@joshtriplett
Copy link
Member

Temporarily shuffling labels around in order to run an FCP.

@joshtriplett joshtriplett added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 20, 2024
@joshtriplett
Copy link
Member

This is unstable, and TTBOMK isn't part of any accepted RFC.

I'm sad to see it go, but I agree that in its current form it may not make sense to support. I'd love to see a design proposal for a portable version, but the current #[start] isn't it.

👍 for removing it.

@joshtriplett
Copy link
Member

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Dec 20, 2024

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

Concerns:

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Dec 20, 2024
@joshtriplett
Copy link
Member

@rfcbot concern check-with-embedded-folks

I would like to get some indication from folks who know the embedded community well to make sure this isn't something widely used. Even though it's a nightly feature, I know that some embedded development does use nightly, and I'd like to get some kind of affirmative indication from at least one person who knows the embedded community well to confirm that this isn't one of the mechanisms used. To the best of my knowledge, it's more common to use things like a #[no_mangle] main function, but I'm not an expert in current embedded Rust.

ping @jamesmunns @BartMassey?

@joshtriplett joshtriplett added the I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination label Dec 20, 2024
@joshtriplett
Copy link
Member

Marking this as I-lang-easy-decision, conditional on hearing back from embedded folks that this isn't widely used. Assuming that's the case, this is a nightly feature, and removing it shouldn't be too controversial.

@joshtriplett joshtriplett added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 20, 2024
@jamesmunns
Copy link
Member

Pinged in chat, we just started our two week break for regular meetings, I'll ping back here if I hear any objections.

@berkus
Copy link
Contributor

berkus commented Dec 20, 2024

Not speaking for the whole embedded community but this is the first time I read about #[start] - have never ever seen it in any embedded software in rust.

@jamesmunns
Copy link
Member

jamesmunns commented Dec 20, 2024

Also grep app only finds like four projects on github that seem to use this attribute (outside of UI tests, etc.): https://grep.app/search?q=%23%5Bstart%5D&filter[lang][0]=Rust - none of them embedded specific.

edit, found some more:

(and some more if you keep digging through "Load More Results", I don't think this is a blocker, and there are likely other ways to achieve this)

Not sure if we want to ping those users, but more as a "heads up your code is going to break".

Either way, it's not used in the embedded wg's crates, but it does seem to be used in a handful of bare metal kernel/OS projects. It would probably be good to have a diagnostic that explains (or links to an explanation) of what to do instead.

@RalfJung
Copy link
Member Author

I filed a bunch of issues pointing here, so if any of these uses is critical, people will hopefully show up and tell us about it.

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

1 similar comment
@traviscross
Copy link
Contributor

@rfcbot reviewed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination I-lang-nominated Nominated for discussion during a lang team meeting. PG-exploit-mitigations Project group: Exploit mitigations proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for the start feature