rustc: split FnAbi's into definitions/direct calls ("of_instance") and indirect calls ("of_fn_ptr"). by eddyb · Pull Request #65947 · rust-lang/rust

eddyb · 2019-10-29T20:36:21Z

After this PR:

InstanceDef::Virtual is only used for "direct" virtual calls, and shims around those calls use InstanceDef::ReifyShim (i.e. for <dyn Trait as Trait>::f as fn(_))
- this could easily be done for intrinsics as well, to allow their reification, but I didn't do it
FnAbi::of_instance is always used for declaring/defining an fn, and for direct calls to an fn
- this is great for e.g. Implement #[track_caller] attribute. (RFC 2091 4/N) #65881 (#[track_caller]), which can introduce the "caller location" argument into "codegen signatures" by only changing FnAbi::of_instance, after this PR
FnAbi::of_fn_ptr is used primarily for indirect calls, i.e. to fn pointers
- not virtual calls (which use FnAbi::of_instance with InstanceDef::Virtual)
- there's also a couple uses where the rustc_codegen_llvm needs to declare (i.e. FFI-import) an LLVM function that has no Rust declaration available at all
  - at least one of them could probably be a "weak lang item" instead

As there are many steps, this PR is best reviewed commit by commit - some of which arguably should be in their own PRs, I may have gotten carried away a bit.

cc @nagisa @rkruppe @oli-obk @anp

rust-highfive · 2019-10-29T20:36:26Z

r? @varkor

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

eddyb · 2019-10-30T10:29:48Z

@bors try @rust-timer queue

rust-timer · 2019-10-30T10:29:50Z

Awaiting bors try build completion

bors · 2019-10-30T10:29:59Z

⌛ Trying commit a3153cb25e0c2eca2a987433763f40383a774cf8 with merge 924ec06b909e3054669b8cec62e173ff2e962a06...

bors · 2019-10-30T13:36:08Z

☀️ Try build successful - checks-azure
Build commit: 924ec06b909e3054669b8cec62e173ff2e962a06 (924ec06b909e3054669b8cec62e173ff2e962a06)

rust-timer · 2019-10-30T13:36:11Z

Queued 924ec06b909e3054669b8cec62e173ff2e962a06 with parent 0b7e28a, future comparison URL.

rust-timer · 2019-10-30T19:00:51Z

Finished benchmarking try commit 924ec06b909e3054669b8cec62e173ff2e962a06, comparison URL.

varkor · 2019-11-08T16:00:20Z

I've looked through, and while the changes look sensible to me, I'm not familiar enough to be sure I haven't missed some detail, so I'm going to reassign. r? @nagisa

varkor · 2019-11-08T16:02:19Z

this could easily be done for intrinsics as well, to allow their reification, but I didn't do it

Does this mean #15694 could be addressed?

eddyb · 2019-11-08T17:26:50Z

Does this mean #15694 could be addressed?

@varkor Yeah, and it's been possible ever since MIR shims were added, really.
I'm just making the diff for that be a couple less lines from a dozen, or thereabouts.

nagisa

Overall LGTM.

r=me once you’re comfortable with landing this.

eddyb · 2019-11-27T12:18:25Z

@nagisa @oli-obk I've added 6 more commits, which should deal with both the review comments and my own concerns (mostly wrt the misusability of Instance::fn_sig).

…FnAbi.

…rtual.

eddyb · 2019-12-03T14:41:20Z

@bors r=oli-obk,nagisa

bors · 2019-12-03T14:41:22Z

📌 Commit c2f4c57 has been approved by oli-obk,nagisa

bors · 2019-12-04T08:22:12Z

⌛ Testing commit c2f4c57 with merge 5f1d6c4...

@nagisa

rustc: split FnAbi's into definitions/direct calls ("of_instance") and indirect calls ("of_fn_ptr"). After this PR: * `InstanceDef::Virtual` is only used for "direct" virtual calls, and shims around those calls use `InstanceDef::ReifyShim` (i.e. for `<dyn Trait as Trait>::f as fn(_)`) * this could easily be done for intrinsics as well, to allow their reification, but I didn't do it * `FnAbi::of_instance` is **always** used for declaring/defining an `fn`, and for direct calls to an `fn` * this is great for e.g. #65881 (`#[track_caller]`), which can introduce the "caller location" argument into "codegen signatures" by only changing `FnAbi::of_instance`, after this PR * `FnAbi::of_fn_ptr` is used primarily for indirect calls, i.e. to `fn` pointers * *not* virtual calls (which use `FnAbi::of_instance` with `InstanceDef::Virtual`) * there's also a couple uses where the `rustc_codegen_llvm` needs to declare (i.e. FFI-import) an LLVM function that has no Rust declaration available at all * at least one of them could probably be a "weak lang item" instead As there are many steps, this PR is best reviewed commit by commit - some of which arguably should be in their own PRs, I may have gotten carried away a bit. cc @nagisa @rkruppe @oli-obk @anp

bors · 2019-12-04T11:22:19Z

☀️ Test successful - checks-azure
Approved by: oli-obk,nagisa
Pushing 5f1d6c4 to master...

@eddyb

Cleanup BodyCache After this PR: - `BodyCache` is renamed to `BodyAndCache` - `ReadOnlyBodyCache` is renamed to `ReadOnlyBodyAndCache` - `ReadOnlyBodyAndCache::body` fn is removed and all calls to it are replaced by a deref (possible due to fix of its `Deref` imp in rust-lang#65947) cc @eddyb @oli-obk

@eddyb

Cleanup BodyCache After this PR: - `BodyCache` is renamed to `BodyAndCache` - `ReadOnlyBodyCache` is renamed to `ReadOnlyBodyAndCache` - `ReadOnlyBodyAndCache::body` fn is removed and all calls to it are replaced by a deref (possible due to fix of its `Deref` imp in rust-lang#65947) cc @eddyb @oli-obk

@eddyb

Cleanup BodyCache After this PR: - `BodyCache` is renamed to `BodyAndCache` - `ReadOnlyBodyCache` is renamed to `ReadOnlyBodyAndCache` - `ReadOnlyBodyAndCache::body` fn is removed and all calls to it are replaced by a deref (possible due to fix of its `Deref` imp in rust-lang#65947) cc @eddyb @oli-obk

rust-highfive assigned varkor Oct 29, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 29, 2019

eddyb commented Oct 30, 2019
View reviewed changes

Comment threadsrc/librustc/ty/layout.rs Outdated

eddyb commented Oct 30, 2019
View reviewed changes

Comment threadsrc/librustc_codegen_llvm/callee.rs Outdated

eddyb mentioned this pull request Oct 31, 2019
FnAbi computation should be cached. #65999
Open

anp mentioned this pull request Nov 2, 2019
Implement #[track_caller] attribute. (RFC 2091 4/N) #65881
Merged

eddyb force-pushed the fn-abi branch from a3153cb to 43c07ddCompare November 3, 2019 09:06

eddyb mentioned this pull request Nov 7, 2019
Implement arbitrary-self dynamic method receivers rust-lang/miri#1038
Closed

rust-highfive assigned nagisa and unassigned varkor Nov 8, 2019

This comment has been minimized.
Sign in to view

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 Nov 16, 2019

This comment has been minimized.
Sign in to view

nagisa reviewed Nov 19, 2019
View reviewed changes

Comment threadsrc/librustc/ty/instance.rs Outdated
Comment threadsrc/librustc_codegen_llvm/callee.rs Outdated
Comment threadsrc/librustc_codegen_llvm/intrinsic.rs

This comment has been minimized.
Sign in to view

eddyb mentioned this pull request Nov 27, 2019
[WIP] rustc_mir: use FnAbi to get abi::call::Conv in miri. #66807
Closed

eddyb force-pushed the fn-abi branch from 43c07dd to 0232114Compare November 27, 2019 12:11

eddyb added 13 commits December 3, 2019 15:28

rustc_codegen_ssa: take a FnAbi instead of a FnSig in declare_fn.
95b9442

rustc_codegen_llvm: move NoReturn attribute to apply_attrs_llfn.
5b7d0f3

rustc_codegen_ssa: use FnAbi::of_instance wherever possible.
4b68afe

rustc: use ReifyShim for reifying Virtual call instances.
39e50e2

rustc: compute FnAbi's for virtual calls through FnAbi::of_instance.
052d0ed

rustc: rename FnAbi::new to FnAbi::of_fn_ptr.
2b921d5

rustc: take a PolyFnSig instead of an FnSig in FnAbi::of_fn_ptr.
902433b

rustc_codegen_llvm: rewrite debuginfo::get_function_signature to use …
8a8749b
…FnAbi.

rustc_codegen_llvm: take an Instance in attributes::from_fn_attrs.
cd3c324

rustc_codegen_llvm: privatize as much of attributes::* as possible.
e93aa10

rustc_target: add abi::call::Conv::Rust distinct from Conv::C.
79d908b

rustc: move Instance::fn_sig to ty::layout and privatize it.
9da14a8

rustc: add docs to FnAbi::{of_fn_ptr,of_instance} and InstanceDef::Vi…
c2f4c57
…rtual.

eddyb force-pushed the fn-abi branch from 0ef6219 to c2f4c57Compare December 3, 2019 13:56

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 Dec 3, 2019

eddyb mentioned this pull request Dec 3, 2019
Cleanup BodyCache #66991
Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 4, 2019

bors merged commit c2f4c57 into rust-lang:masterDec 4, 2019

This was referenced Dec 4, 2019
Reorganize borrow check diagnostic code #66815
Merged
Add a raw "address of" operator #64588
Merged

eddyb deleted the fn-abi branch December 4, 2019 11:29

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

rust-highfive commented Oct 29, 2019

Uh oh!

eddyb commented Oct 30, 2019

Uh oh!

rust-timer commented Oct 30, 2019

Uh oh!

bors commented Oct 30, 2019

Uh oh!

Uh oh!

bors commented Oct 30, 2019

Uh oh!

rust-timer commented Oct 30, 2019

Uh oh!

Uh oh!

rust-timer commented Oct 30, 2019

Uh oh!

varkor commented Nov 8, 2019

Uh oh!

varkor commented Nov 8, 2019

Uh oh!

eddyb commented Nov 8, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

nagisa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

eddyb commented Nov 27, 2019

Uh oh!

eddyb commented Dec 3, 2019

Uh oh!

bors commented Dec 3, 2019

Uh oh!

bors commented Dec 4, 2019

Uh oh!

bors commented Dec 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

eddyb commented Oct 29, 2019•
edited
Loading