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

Don't add argc and argv arguments to main on WASI. #65576

Merged
merged 1 commit into from
Oct 19, 2019

Conversation

sunfishcode
Copy link
Member

Add a target setting to allow targets to specify whether the generated
main function should be passed argc and argv arguments. Set it
to false on wasm32-wasi, since WASI's args::args() calls into the
WASI APIs itself. This will allow the WASI toolchain to avoid linking
and running command-line argument initialization code when the arguments
aren't actually needed.

Add a target setting to allow targets to specify whether the generated
`main` function should be passed `argc` and `argv` arguments. Set it
to false on wasm32-wasi, since WASI's `args::args()` calls into the
WASI APIs itself. This will allow the WASI toolchain to avoid linking
and running command-line argument initialization code when the arguments
aren't actually needed.
@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2019
@sunfishcode
Copy link
Member Author

r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 18, 2019

📌 Commit b25e323 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2019
@@ -414,8 +414,11 @@ pub fn maybe_create_entry_wrapper<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(cx: &'
rust_main_def_id: DefId,
use_start_lang_item: bool,
) {
let llfty =
cx.type_func(&[cx.type_int(), cx.type_ptr_to(cx.type_i8p())], cx.type_int());
let llfty = if cx.sess().target.target.options.main_needs_argc_argv {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this wants a comment.

let param_argv = bx.get_param(1);
let arg_argc = bx.intcast(param_argc, cx.type_isize(), true);
let arg_argv = param_argv;
let (arg_argc, arg_argv) = if cx.sess().target.target.options.main_needs_argc_argv {
Copy link
Contributor

Choose a reason for hiding this comment

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

The function is already large; this bit could be extracted into its own (+ appropriate doc comment).

let arg_argc = bx.intcast(param_argc, cx.type_isize(), true);
let arg_argv = param_argv;
let (arg_argc, arg_argv) = if cx.sess().target.target.options.main_needs_argc_argv {
// Params from native main() used as args for rust start function
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Params from native main() used as args for rust start function
// Params from native `main()` used as args for rust start function

let arg_argv = param_argv;
(arg_argc, arg_argv)
} else {
// The Rust start function doesn't need argc and argv, so just pass zeros.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The Rust start function doesn't need argc and argv, so just pass zeros.
// The Rust start function doesn't need `argc` and `argv`, so just pass zeros.

@Centril
Copy link
Contributor

Centril commented Oct 19, 2019

@sunfishcode I noticed this was r+ed now -- if this PR is merged before, can you follow up to address my review comments?

Centril added a commit to Centril/rust that referenced this pull request Oct 19, 2019
…=alexcrichton

Don't add `argc` and `argv` arguments to `main` on WASI.

Add a target setting to allow targets to specify whether the generated
`main` function should be passed `argc` and `argv` arguments. Set it
to false on wasm32-wasi, since WASI's `args::args()` calls into the
WASI APIs itself. This will allow the WASI toolchain to avoid linking
and running command-line argument initialization code when the arguments
aren't actually needed.
bors added a commit that referenced this pull request Oct 19, 2019
Rollup of 6 pull requests

Successful merges:

 - #65174 (Fix zero-size uninitialized boxes)
 - #65252 (expand: Simplify expansion of derives)
 - #65485 (Suppress ICE when validators disagree on `LiveDrop`s in presence of `&mut`)
 - #65542 (Refer to "associated functions" instead of "static methods")
 - #65545 (More symbol cleanups)
 - #65576 (Don't add `argc` and `argv` arguments to `main` on WASI.)

Failed merges:

r? @ghost
@bors bors merged commit b25e323 into rust-lang:master Oct 19, 2019
@sunfishcode sunfishcode deleted the main-needs-argc-argv branch October 21, 2019 17:51
@sunfishcode
Copy link
Member Author

@Centril The PR was indeed merged; I'll submit a new PR to address your feedback.

JohnTitor pushed a commit to JohnTitor/rust that referenced this pull request Oct 22, 2019
This makes a few code cleanups to follow up on the review comments in
rust-lang#65576.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 22, 2019
Code cleanups following up on rust-lang#65576.

This makes a few code cleanups to follow up on the review comments in
rust-lang#65576.

r? @Centril
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 23, 2019
Code cleanups following up on rust-lang#65576.

This makes a few code cleanups to follow up on the review comments in
rust-lang#65576.

r? @Centril
bors added a commit that referenced this pull request Oct 23, 2019
Rollup of 14 pull requests

Successful merges:

 - #64145 (Target-feature documented as unsafe)
 - #65007 (Mention keyword closing policy)
 - #65417 (Add more coherence tests)
 - #65507 (Fix test style in unused parentheses lint test)
 - #65591 (Add long error explanation for E0588)
 - #65617 (Fix WASI sleep impl)
 - #65656 (Add option to disable keyboard shortcuts in docs)
 - #65678 (Add long error explanation for E0728)
 - #65681 (Code cleanups following up on #65576.)
 - #65686 (refactor and move `maybe_append` )
 - #65688 (Add some tests for fixed ICEs)
 - #65689 (bring back some Debug instances for Miri)
 - #65695 (self-profiling: Remove module names from some event-ids in codegen backend.)
 - #65706 (Add missing space in librustdoc)

Failed merges:

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants