Remove redundancy from the implementation of C variadics. #63492

eddyb · 2019-08-12T13:04:41Z

This cleanup was first described in #44930 (comment):

AST doesn't track c_variadic: bool anymore, relying solely on a trailing CVarArgs type in fn signatures
HIR doesn't have a CVarArgs anymore, relying solely on c_variadic: bool
- same for ty::FnSig (see tests for diagnostics improvements from that)
- {hir,mir}::Body have one extra argument than the signature when c_variadic == true
- rustc_typeck and rustc_mir::{build,borrowck} need to give that argument the right type (which no longer uses a lifetime parameter, but a function-internal scope)
rustc_target::abi::call doesn't need special hacks anymore (since it never sees the VaListImpl now, it's all inside the body)

r? @nagisa / @rkruppe cc @dlrobertson @oli-obk

dlrobertson

👍 Great stuff! This is much cleaner.

dlrobertson · 2019-08-13T11:30:41Z

src/librustc/hir/lowering.rs

@@ -2102,7 +2097,14 @@ impl<'a> LoweringContext<'a> {
    }

    fn lower_fn_args_to_names(&mut self, decl: &FnDecl) -> hir::HirVec<Ident> {
-        decl.inputs
+        // Skip the `...` (`CVarArgs`) trailing arguments from the AST,


Good idea to skip this here.

src/librustc_codegen_ssa/mir/block.rs

src/librustc_mir/build/mod.rs

bors · 2019-08-17T12:54:13Z

☔ The latest upstream changes (presumably #63655) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb · 2019-08-18T11:11:17Z

cc @matthewjasper @nikomatsakis for the MIR changes

totsteps · 2019-08-27T22:24:36Z

Ping from triage, can someone from @rust-lang/compiler review this PR.

Thanks

JohnCSimon · 2019-09-07T16:19:43Z

Ping from triage
@eddyb This has sat idle for a bit and has taken on several merge conflicts.
Thank you.

joelpalmer · 2019-09-16T10:24:23Z

Ping from Triage: Closing due to inactivity. If or when there are updates, please re-open. @eddyb Thanks for the PR.

src/librustc_codegen_ssa/mir/block.rs

src/librustc_mir/borrow_check/nll/type_check/input_output.rs

nagisa · 2019-09-27T02:56:46Z

@eddyb overall r=me, with the two nits fixed.

oli-obk · 2019-09-26T16:21:04Z

src/librustc_codegen_ssa/mir/mod.rs

+                    va_list.layout.ty,
+                    scope,
+                    variable_access,
+                    VariableKind::ArgumentVariable(arg_index + 1),


isn't this one too far? arg_index is already one beyond the list of normal arguments

+ 1 is pre-existing, I'm not sure what the deal is (see my comment on this in some of the prereq commits in #56231).

oli-obk · 2019-09-27T06:52:03Z

src/librustc_typeck/check/mod.rs

@@ -1100,8 +1100,26 @@ fn check_fn<'a, 'tcx>(
    let outer_hir_id = fcx.tcx.hir().as_local_hir_id(outer_def_id).unwrap();
    GatherLocalsVisitor { fcx: &fcx, parent_id: outer_hir_id, }.visit_body(body);

+    // C-variadic fns also have a `VaList` input that's not listed in `fn_sig`
+    // (as it's created inside the body itself, not passed in from outside).
+    let maybe_va_list = if fn_sig.c_variadic {


almost the same code snippet appears in src/librustc_mir/build/mod.rs

factor out into a function?

Feel free to open an issue but I think there is a more general problem here and it also involves the whole liberated_fn_sigs.

That is, there should be a common way to get the "function signature as viewed from inside the body" - possibly including reading the types associated with the hir::Body arguments as opposed to computing them from tcx.fn_sig(def_id).

Point is, I don't want a shared function for just the c_variadic part of this.

Centril · 2019-09-28T03:38:34Z

Failed in #64860 (comment), @bors r-

Centril · 2019-09-28T03:38:40Z

@bors retry

…gnatures.

…nCx`.

eddyb · 2019-09-28T15:00:04Z

@bors r-

r? @matthewjasper for the MIR/NLL borrowck (I had to redo it, first attempt was flawed)

matthewjasper · 2019-09-28T16:41:46Z

@bors r=nagisa,matthewjasper

bors · 2019-09-28T16:41:47Z

📌 Commit 057f23d has been approved by nagisa,matthewjasper

@nagisa

Remove redundancy from the implementation of C variadics. This cleanup was first described in rust-lang#44930 (comment): * AST doesn't track `c_variadic: bool` anymore, relying solely on a trailing `CVarArgs` type in fn signatures * HIR doesn't have a `CVarArgs` anymore, relying solely on `c_variadic: bool` * same for `ty::FnSig` (see tests for diagnostics improvements from that) * `{hir,mir}::Body` have one extra argument than the signature when `c_variadic == true` * `rustc_typeck` and `rustc_mir::{build,borrowck}` need to give that argument the right type (which no longer uses a lifetime parameter, but a function-internal scope) * `rustc_target::abi::call` doesn't need special hacks anymore (since it never sees the `VaListImpl` now, it's all inside the body) r? @nagisa / @rkruppe cc @dlrobertson @oli-obk

@ghost

Rollup of 5 pull requests Successful merges: - #63492 (Remove redundancy from the implementation of C variadics.) - #64589 (Differentiate AArch64 bare-metal targets between hf and non-hf.) - #64799 (Fix double panic when printing query stack during an ICE) - #64824 (No StableHasherResult everywhere) - #64884 (Add pkg-config to dependency list if building for Linux on Linux) Failed merges: r? @ghost

Rustup rust-lang/rust#63492 changelog: none

rust-highfive assigned nagisa Aug 12, 2019

This comment has been minimized.

# to view

eddyb force-pushed the cvarargs branch from 7e57d8f to d2503bb Compare August 12, 2019 16:02

This comment has been minimized.

# to view

eddyb force-pushed the cvarargs branch from d2503bb to c8a56c5 Compare August 13, 2019 06:54

dlrobertson reviewed Aug 13, 2019

View reviewed changes

eddyb force-pushed the cvarargs branch 2 times, most recently from 8b9d92c to f6d4b3b Compare August 14, 2019 10:58

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 17, 2019

eddyb force-pushed the cvarargs branch from f6d4b3b to cf65f4c Compare August 18, 2019 09:09

totsteps added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 27, 2019

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 7, 2019

joelpalmer closed this Sep 16, 2019

joelpalmer added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 16, 2019

eddyb reopened this Sep 26, 2019

nagisa reviewed Sep 27, 2019

View reviewed changes

src/librustc_codegen_ssa/mir/block.rs Outdated Show resolved Hide resolved

nagisa reviewed Sep 27, 2019

View reviewed changes

src/librustc_mir/borrow_check/nll/type_check/input_output.rs Outdated Show resolved Hide resolved

nagisa added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. labels Sep 27, 2019

oli-obk reviewed Sep 27, 2019

View reviewed changes

eddyb force-pushed the cvarargs branch from cf65f4c to abbd237 Compare September 27, 2019 14:37

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 28, 2019

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 28, 2019

eddyb added 4 commits September 28, 2019 17:38

syntax: don't keep a redundant c_variadic flag in the AST.

8a9d775

rustc: don't store a lifetime in hir::TyKind::CVarArgs.

7683d1c

rustc: rely on c_variadic == true instead of CVarArgs in HIR/Ty fn si…

a88d181

…gnatures.

rustc_codegen_ssa: remove redundant va_list_ref field from `Functio…

057f23d

…nCx`.

eddyb force-pushed the cvarargs branch from 003c5ec to 057f23d Compare September 28, 2019 14:58

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 28, 2019

rust-highfive assigned matthewjasper and unassigned nagisa Sep 28, 2019

eddyb added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 28, 2019

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 Sep 28, 2019

Centril mentioned this pull request Sep 29, 2019

Rollup of 5 pull requests #64886

Merged

bors merged commit 057f23d into rust-lang:master Sep 29, 2019

eddyb deleted the cvarargs branch September 29, 2019 10:42

tesuji added a commit to tesuji/rust-clippy that referenced this pull request Sep 29, 2019

Rustup rust-lang/rust#63492

6e3a0ea

bors added a commit to rust-lang/rust-clippy that referenced this pull request Sep 29, 2019

Auto merge of #4600 - lzutao:rustup-63492, r=oli-obk

b292183

Rustup rust-lang/rust#63492 changelog: none

eddyb mentioned this pull request Oct 3, 2019

Tracking issue for RFC 2091: Implicit caller location #47809

Closed

3 tasks

Remove redundancy from the implementation of C variadics. #63492

Remove redundancy from the implementation of C variadics. #63492

Uh oh!

Conversation

eddyb commented Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

dlrobertson left a comment

Choose a reason for hiding this comment

Uh oh!

dlrobertson Aug 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bors commented Aug 17, 2019

Uh oh!

eddyb commented Aug 18, 2019

Uh oh!

totsteps commented Aug 27, 2019

Uh oh!

JohnCSimon commented Sep 7, 2019

Uh oh!

joelpalmer commented Sep 16, 2019

Uh oh!

Uh oh!

Uh oh!

nagisa commented Sep 27, 2019

Uh oh!

oli-obk Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

eddyb Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

oli-obk Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

eddyb Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

eddyb Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Centril commented Sep 28, 2019

Uh oh!

Centril commented Sep 28, 2019

Uh oh!

eddyb commented Sep 28, 2019

Uh oh!

matthewjasper commented Sep 28, 2019

Uh oh!

bors commented Sep 28, 2019

Uh oh!

Uh oh!

eddyb commented Aug 12, 2019 •

edited

Loading

eddyb Sep 27, 2019 •

edited

Loading