Stream: t-compiler/wg-polymorphization

Topic: cycles


davidtwco (Mar 08 2020 at 18:59, on Zulip):

@nikomatsakis, eddyb said I should speak to you about this.

There's an ICE when bootstrapping in stage two. I'll probably mangle this explanation, but here goes. Shims cannot be polymorphized, so I need to detect when they'll be generated and mark some generic parameters as used. I did something similar with vtables, if I saw an unsized cast, I visited the source and target types in an exhaustive mode that visited closures, this meant that more generic parameters were marked as used and vtables would not be generated. However, with drop shims, while I can make a similar change, my analysis needs to be transitive for it to work.. but that causes cycle errors. I can't figure a way to work around the cycle errors, because my query is for each individual function, there's no way for me to keep track of where I've been. eddyb linked me to #68828, and as far as I can gather from that, there's no consensus on how to deal with cycle errors.

The only other idea that eddyb had was to use ty::Placeholder (or ty::Erased, something like that), instead of ty::Param when I use the results of my analysis, and then told me to speak to you about it.

I'm slightly concerned because this seems to be about the last blocker to have this whole thing working, but a solution to cycle errors doesn't seem to becoming soon, and re-working shims to avoid the issue would probably take a while; it's getting v. close to when I need to start evaluating and writing my final paper on this (which is my fault for not getting further earlier).

davidtwco (Mar 08 2020 at 19:00, on Zulip):

(#69749 doesn't yet contain the patch to deal with drop shims that don't require the analysis be transitive, but it does contain the patch that I used for vtables)

davidtwco (Mar 08 2020 at 23:24, on Zulip):

Wrapping up for today, didn't make much progress on resolving this issue (today's progress before hitting this issue). Here's what I've got uncommitted:

mark-i-m (Mar 09 2020 at 02:51, on Zulip):

Probably a stupid question, but why why does the shim need to be monomorphic? Is the closure dropped in a different way depending on its context? Or am I misunderstanding?

davidtwco (Mar 09 2020 at 08:01, on Zulip):

I don’t understand completely, it’s a limitation of the infrastructure for that in the compiler.

davidtwco (Mar 09 2020 at 10:09, on Zulip):

Thinking on it more, I assume it's because the drop shim needs to identify the specific Drop impl, so it can call Drop::drop - it's intuitive that would inherently require all the generic parameters be known?

davidtwco (Mar 09 2020 at 10:48, on Zulip):

The only solution I've been able to come up with (lacking sufficient knowledge of drop shims to tackle the problem from that angle) is to make the analysis transitive - which I can do, it's not too hard - but I'd need to have a function like this to avoid cycle errors:

fn would_cause_query_error<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> bool {
    let query_map = tcx.queries.try_collect_active_jobs().unwrap();
    let mut current_job = ty::tls::with_related_context(tcx, |icx| icx.query);
    while let Some(job) = current_job {
        let info = query_map.get(&job).unwrap();
        if let ty::query::Query::used_generic_params(other_def_id) = info.info.query {
            if other_def_id == def_id {
                return true;
            }
        }

        current_job = info.job.parent;
    }

    false
}

I could probably make that function faster by exiting early as soon as I see a query that isn't used_generic_params, since if the analysis is transitive, there won't be any other queries in-between calls to used_generic_params.

oli (Mar 09 2020 at 11:09, on Zulip):

The mir inliner has the same problem: https://github.com/rust-lang/rust/pull/68828

davidtwco (Mar 09 2020 at 11:14, on Zulip):

Yeah, eddyb linked me to that issue, I referenced it in the initial message.

oli (Mar 09 2020 at 12:44, on Zulip):

Well... true cycle detection in the query system isn't possible, but if you can split your problem in into a pre cycle part and a post cycle part, you can do cycle detection on the pre cycle part

davidtwco (Mar 09 2020 at 12:47, on Zulip):

I'm struggling to imagine an example of what that would look like (not just for the polymorphization case, but more generally too).

nikomatsakis (Mar 09 2020 at 18:14, on Zulip):

@davidtwco hmm

nikomatsakis (Mar 09 2020 at 18:15, on Zulip):

I might need to get more of an overview of exactly how your analysis works

nikomatsakis (Mar 09 2020 at 18:15, on Zulip):

maybe we can find a time to chat about this? I have to leave early today but plausibly could chat tomorrow or (maybe easier) Wed morning (my time)

davidtwco (Mar 09 2020 at 18:16, on Zulip):

#69749 has all of the code (minus the change to get drop shims working w/out transitivity)

davidtwco (Mar 09 2020 at 18:16, on Zulip):

I can do that

nikomatsakis (Mar 09 2020 at 18:16, on Zulip):

Would you recommend I start by trying to read your PR? Maybe that's best

davidtwco (Mar 09 2020 at 18:16, on Zulip):

Tomorrow or Wednesday' afternoon (my time) works for me.

nikomatsakis (Mar 09 2020 at 18:16, on Zulip):

but I was assuming it'd be a lot of code :)

davidtwco (Mar 09 2020 at 18:16, on Zulip):

It's not a huge amount.

davidtwco (Mar 09 2020 at 18:17, on Zulip):

Having some familiarity from the PR would be helpful.

nikomatsakis (Mar 09 2020 at 18:17, on Zulip):

(Which reminds me that this might make a nice addition to rustc-dev-guide)

nikomatsakis (Mar 09 2020 at 18:17, on Zulip):

(That is, some notes on how it works)

davidtwco (Mar 09 2020 at 18:17, on Zulip):

I'll make a note to do that

nikomatsakis (Mar 09 2020 at 18:17, on Zulip):

I guess src/librustc_mir/monomorphize/polymorphize.rs is sort of the "main file"?

davidtwco (Mar 09 2020 at 18:17, on Zulip):

Yeah, that's the analysis itself.

davidtwco (Mar 09 2020 at 18:18, on Zulip):

There's a polymorphize function in rustc/ty/instance.rs which applies the results to an Instance.

davidtwco (Mar 09 2020 at 18:18, on Zulip):

There's a minor change to rustc_mir/monomorphize/collector.rs which uses that in create_fn_mono_item.

davidtwco (Mar 09 2020 at 18:18, on Zulip):

And a handful of places in rustc_codegen_ssa which also use that function.

davidtwco (Mar 09 2020 at 18:19, on Zulip):

The majority of the work has been debugging where the analysis needed updated to fix obscure substitution failures from later in the compiler.

mark-i-m (Mar 09 2020 at 18:36, on Zulip):

(Which reminds me that this might make a nice addition to rustc-dev-guide)

Funny you should mention that... https://github.com/rust-lang/rustc-guide/pull/605/files#diff-d504f9659c36662fe3433e5a64d139ac

mark-i-m (Mar 09 2020 at 18:37, on Zulip):

It's just a stub currently, since I don't know much, but whenever you have time, expanding on it would be great!

davidtwco (Mar 09 2020 at 19:26, on Zulip):

@nikomatsakis do you want to schedule a chat for tomorrow or wednesday then?

davidtwco (Mar 10 2020 at 21:41, on Zulip):

@nikomatsakis ping; have you had an opportunity to look into this at all? I've got time tomorrow that I'd like to be able to use to make progress on this.

nikomatsakis (Mar 10 2020 at 22:45, on Zulip):

@davidtwco I was totally slammed today, but I could take a look first thing tomorrow morning (9am my time, more or less)

davidtwco (Mar 10 2020 at 23:22, on Zulip):

@nikomatsakis I’d appreciate that, I’ll be around at that time.

nikomatsakis (Mar 11 2020 at 13:18, on Zulip):

@davidtwco got a spotty wifi but I'm around :)

davidtwco (Mar 11 2020 at 13:18, on Zulip):

o/

davidtwco (Mar 11 2020 at 13:21, on Zulip):

@nikomatsakis the primary issue at the moment is that I don't know how to support drop shims w/ the analysis, so I'm blocked on being able to get it passing CI and then landed.

nikomatsakis (Mar 11 2020 at 13:22, on Zulip):

right

davidtwco (Mar 11 2020 at 13:24, on Zulip):

Because drop shims need to be monomorphic (I don't understand the infrastructure around that enough to feel confident in my understanding of why that is) and because closures inherit type parameters, if a function passes a closure to another function, which is then dropped, then that causes a substitution failure (and presumably more failures later on that we don't get to) - to detect this case, the analysis needs to be transitive, which is hard because of cycle errors.

davidtwco (Mar 11 2020 at 13:25, on Zulip):

If @eddyb is around, they might be able to explain this better.

eddyb (Mar 11 2020 at 13:26, on Zulip):

uhhhhhhh

eddyb (Mar 11 2020 at 13:26, on Zulip):

I should've been around this topic from the start, so sorry

eddyb (Mar 11 2020 at 13:26, on Zulip):

so the explanation is that shims are fake MIR bodies for a real definition

eddyb (Mar 11 2020 at 13:27, on Zulip):

e.g. drop_in_place::<Foo> gets a shim Instance that has substs=[Foo]

eddyb (Mar 11 2020 at 13:27, on Zulip):

and the problem is that is that Foo is also in the InstanceDef determining the shim MIR body

eddyb (Mar 11 2020 at 13:28, on Zulip):

so if there are any generic parameters in the InstanceDef (which contains Foo in my example above), they would be attempted to be substituted with { #0 -> Foo }

nikomatsakis (Mar 11 2020 at 13:30, on Zulip):

I'm not sure I quite follow, @eddyb -- I think you're saying that Foo is "hard-coded" in the InstanceDef, so just changing the substitution doesn't suffice?

eddyb (Mar 11 2020 at 13:30, on Zulip):

so drop_in_place::<Vec<#0>> (I'm using #0 but this could be T) produces a MIR body referring to Vec<#0> but which also is going to get monomorphized by { #0 -> Vec<#0> }

eddyb (Mar 11 2020 at 13:30, on Zulip):

so you'll create Vec<Vec<#0>> which is absurd

nikomatsakis (Mar 11 2020 at 13:30, on Zulip):

I'm trying in any case to read more into the PR because I don't quite understand where the cycle comes from

nikomatsakis (Mar 11 2020 at 13:31, on Zulip):

that is, what is the query involved

eddyb (Mar 11 2020 at 13:31, on Zulip):

@nikomatsakis there's both a type in InstanceDef but also the substs for the original definition (or declaration if you want) that is getting shimmed

eddyb (Mar 11 2020 at 13:31, on Zulip):

wait what cycle

eddyb (Mar 11 2020 at 13:31, on Zulip):

oh I'm not talking about the cycle

nikomatsakis (Mar 11 2020 at 13:31, on Zulip):

.used_generic_params I guess?

eddyb (Mar 11 2020 at 13:31, on Zulip):

@nikomatsakis the cycle is like MIR inlining

eddyb (Mar 11 2020 at 13:31, on Zulip):

recursive functions result in recursive queries etc.

davidtwco (Mar 11 2020 at 13:31, on Zulip):

This code demonstrates a case where this causes the PR to break; but that case can be fixed. But as soon as you extend it, like this, the analysis would need to be transitive (calling itself on functions the current MIR body invokes) to detect the problematic case, but that can cause cycle errors where (foo -> bar -> foo) - the PR, right now, doesn't do that, because it cycle errors.

eddyb (Mar 11 2020 at 13:32, on Zulip):

@davidtwco making the analysis transitive is likely much harder than fixing shims and vtables

nikomatsakis (Mar 11 2020 at 13:32, on Zulip):

So basically it's that same problem of needing to anayze / walk the call graph?

eddyb (Mar 11 2020 at 13:32, on Zulip):

I'm sorry I didn't explain this better

eddyb (Mar 11 2020 at 13:32, on Zulip):

nikomatsakis said:

So basically it's that same problem of needing to anayze / walk the call graph?

yupp

nikomatsakis (Mar 11 2020 at 13:32, on Zulip):

I'm not sure how much experimentation has been done there?

eddyb (Mar 11 2020 at 13:33, on Zulip):

nikomatsakis said:

I'm not sure I quite follow, eddyb -- I think you're saying that Foo is "hard-coded" in the InstanceDef, so just changing the substitution doesn't suffice?

it's just that right now we have Foo in two places and that basically causes double substitution if it has any generic parameters in it

nikomatsakis (Mar 11 2020 at 13:34, on Zulip):

Personally, I still favor introducing a "construct the call graph query" and seeing how well that works, but I think that extending the query system to better accommodate cycles in this case would also be ok. It seems like this case is different from MIR inlining in some particulars.

eddyb (Mar 11 2020 at 13:34, on Zulip):

we could probably solve this by not not using instance.substs when instance.def is a shim and we're working with the MIR body of the shim

nikomatsakis (Mar 11 2020 at 13:34, on Zulip):

@eddyb ok, I admit I still don't really follow what you're talking about, though I think I have a sense for it,

nikomatsakis (Mar 11 2020 at 13:35, on Zulip):

are you saying that this would obviate the need for cyclic queries / walking call graph in some sense?

eddyb (Mar 11 2020 at 13:35, on Zulip):

it's that PR of mine that asserts that doesn't resolve polymorphic shims

eddyb (Mar 11 2020 at 13:35, on Zulip):

@nikomatsakis it would fix the problems that led @davidtwco down the eldritch path of trying to make the analysis transitive

nikomatsakis (Mar 11 2020 at 13:35, on Zulip):

nikomatsakis said:

Personally, I still favor introducing a "construct the call graph query" and seeing how well that works, but I think that extending the query system to better accommodate cycles in this case would also be ok. It seems like this case is different from MIR inlining in some particulars.

to elaborate on this: I feel like we could have some way to say "cyclic queries are ok as long as all the members of the cycle are the same query and it is appropriately annotated", though we'd have to be careful about explaining the rules that permit a query to be so annotated

davidtwco (Mar 11 2020 at 13:35, on Zulip):

The issue with making the analysis transitive and that causing cycle errors is just a symptom of one potential solution (changing the analysis) in response to the root problem - substs in Instance for shims.

davidtwco (Mar 11 2020 at 13:36, on Zulip):

davidtwco said:

The issue with making the analysis transitive and that causing cycle errors is just a symptom of one potential solution (changing the analysis) in response to the root problem - substs in Instance for shims.

I think I confused this in my messaging, sorry.

nikomatsakis (Mar 11 2020 at 13:36, on Zulip):

eddyb said:

it's that PR of mine that asserts that doesn't resolve polymorphic shims

yeah, I remember that PR, but

eddyb (Mar 11 2020 at 13:36, on Zulip):

I was really happy when @davidtwco initially told me the analysis isn't transitive, because that means we can land and enable it in a reasonable timeframe

eddyb (Mar 11 2020 at 13:36, on Zulip):

instead of wasting months trying to do cyclic nonsense

nikomatsakis (Mar 11 2020 at 13:37, on Zulip):

can someone clarify for me what is meant by the analysis being intransitive

davidtwco (Mar 11 2020 at 13:37, on Zulip):

It's much simpler without being transitive, it seems to be only this case that "needs" it.

nikomatsakis (Mar 11 2020 at 13:37, on Zulip):

it analyzes the fn body but not callees, I guess..?

davidtwco (Mar 11 2020 at 13:37, on Zulip):

nikomatsakis said:

it analyzes the fn body but not callees, I guess..?

Exactly this.

eddyb (Mar 11 2020 at 13:37, on Zulip):

yes, assumes callees use all of their substs

nikomatsakis (Mar 11 2020 at 13:37, on Zulip):

that will clearly lose precision

nikomatsakis (Mar 11 2020 at 13:37, on Zulip):

but we are saying "that's ok"

eddyb (Mar 11 2020 at 13:37, on Zulip):

it only solves closures, really, since it's transitive through those

eddyb (Mar 11 2020 at 13:38, on Zulip):

@nikomatsakis it's unattainable precision without a lot of effort nobody has time for

eddyb (Mar 11 2020 at 13:38, on Zulip):

that's my position

nikomatsakis (Mar 11 2020 at 13:38, on Zulip):

I'm not arguing necessarily, just trying to understand

nikomatsakis (Mar 11 2020 at 13:38, on Zulip):

I agree it's a reasonable "first cut"

eddyb (Mar 11 2020 at 13:38, on Zulip):

so we're not losing anything by starting without it because we could never "just do it"

eddyb (Mar 11 2020 at 13:38, on Zulip):

and closures are arguably important enough

nikomatsakis (Mar 11 2020 at 13:38, on Zulip):

I mostly wanted to be sure I wasn't missing something

davidtwco (Mar 11 2020 at 13:39, on Zulip):

(I'm happy to sink time into it in future, but right now, an intransitive approach is desirable because it's 95% done and I've got a paper to write :sad:)

nikomatsakis (Mar 11 2020 at 13:39, on Zulip):

i.e., it's not like it wouldn't be useful in the abstract

eddyb (Mar 11 2020 at 13:39, on Zulip):

right, I'm happy about avoiding the complexity upfront

davidtwco (Mar 11 2020 at 13:39, on Zulip):

Making the analysis transitive is a solution to the problem that eddyb was describing initially.

eddyb (Mar 11 2020 at 13:40, on Zulip):

I never meant to imply it was a reasonable solution, and it's probably not even that hard to make the shims work correctly when polymorphic tbh

eddyb (Mar 11 2020 at 13:41, on Zulip):

(to the extent that they can be polymorphic, I mean)

nikomatsakis (Mar 11 2020 at 13:42, on Zulip):

So maybe we should come back a bit to the problem

nikomatsakis (Mar 11 2020 at 13:42, on Zulip):

but first, one other question

nikomatsakis (Mar 11 2020 at 13:42, on Zulip):

would landing your PR @eddyb allow this PR to land?

nikomatsakis (Mar 11 2020 at 13:42, on Zulip):

I feel like I r+'d it

nikomatsakis (Mar 11 2020 at 13:42, on Zulip):

but maybe I didn't :)

nikomatsakis (Mar 11 2020 at 13:43, on Zulip):

I'm having a hard time keeping up with reviews

eddyb (Mar 11 2020 at 13:43, on Zulip):

no, it only acts as an early detection for the problem

davidtwco (Mar 11 2020 at 13:43, on Zulip):

I don't think so.

eddyb (Mar 11 2020 at 13:43, on Zulip):

it still causes an ICE or w/e

nikomatsakis (Mar 11 2020 at 13:43, on Zulip):

but I still don't have a good "gut understanding" of the problem and i'd kind of like to

nikomatsakis (Mar 11 2020 at 13:43, on Zulip):

okok

nikomatsakis (Mar 11 2020 at 13:43, on Zulip):

davidtwco said:

This code demonstrates a case where this causes the PR to break; but that case can be fixed. But as soon as you extend it, like this, the analysis would need to be transitive (calling itself on functions the current MIR body invokes) to detect the problematic case, but that can cause cycle errors where (foo -> bar -> foo) - the PR, right now, doesn't do that, because it cycle errors.

eddyb (Mar 11 2020 at 13:43, on Zulip):

it's not a fix, it's just enforcing the implicit rules

davidtwco (Mar 11 2020 at 13:43, on Zulip):

It would have saved me some time working out what the problem was, had @eddyb's PR landed.

eddyb (Mar 11 2020 at 13:43, on Zulip):

oops

eddyb (Mar 11 2020 at 13:43, on Zulip):

I should finish it

nikomatsakis (Mar 11 2020 at 13:44, on Zulip):

I don't quite understand the example you gave @davidtwco

nikomatsakis (Mar 11 2020 at 13:45, on Zulip):

first of all, is the F: Fn() bound significant?

eddyb (Mar 11 2020 at 13:45, on Zulip):

all examples are going to be weird since only closures can really introduce this problem

davidtwco (Mar 11 2020 at 13:45, on Zulip):

It's a minimization of the ICE that comes out of the rustc crate when compiled with my stage one. I think it's about as small as it can get.

nikomatsakis (Mar 11 2020 at 13:46, on Zulip):

I'm guessing no

davidtwco (Mar 11 2020 at 13:46, on Zulip):

I don't think it is.

davidtwco (Mar 11 2020 at 13:46, on Zulip):

(there goes it being as small as it could be)

nikomatsakis (Mar 11 2020 at 13:47, on Zulip):

so the desired effect here

nikomatsakis (Mar 11 2020 at 13:47, on Zulip):

is that R, S are unused for foo?

nikomatsakis (Mar 11 2020 at 13:47, on Zulip):

except they are not, because

nikomatsakis (Mar 11 2020 at 13:47, on Zulip):

well, they are, right? :)

davidtwco (Mar 11 2020 at 13:48, on Zulip):

The analysis correctly determines that R and S are unused in foo and foo::{{closure}} and foo::{{closure}}::{{closure}}- but that causes an issue.

nikomatsakis (Mar 11 2020 at 13:48, on Zulip):

and the issue is?

nikomatsakis (Mar 11 2020 at 13:48, on Zulip):

ok, I'm starting to maybe see the connection to the drop shim

davidtwco (Mar 11 2020 at 13:48, on Zulip):

That code results in a drop shim Instance being created.

nikomatsakis (Mar 11 2020 at 13:48, on Zulip):

we have a drop shim for OnDrop

nikomatsakis (Mar 11 2020 at 13:49, on Zulip):

or rather OnDrop<foo::{{closure}}::{{closure}}>

nikomatsakis (Mar 11 2020 at 13:49, on Zulip):

which I will shorten to "fcc" :)

nikomatsakis (Mar 11 2020 at 13:49, on Zulip):

and that fcc type has a substs that would include R, S ..

nikomatsakis (Mar 11 2020 at 13:49, on Zulip):

and .. somehow .. this leads to ICE, right?

nikomatsakis (Mar 11 2020 at 13:50, on Zulip):

one thing I don't know: in your PR, if we determine a parameter is unused, during monomorphization, are we using ty::Param as its value?

nikomatsakis (Mar 11 2020 at 13:50, on Zulip):

I think this is what @eddyb advocated for some time back

davidtwco (Mar 11 2020 at 13:50, on Zulip):

My understanding of the root problem is slightly flakey, but because Instance will contain the type parameters in both the instance.def = InstanceDef::DropShim (which contains the type because that's used for the MIR shim part), and the instance.substs, that causes double substitution - and an ICE as a result. I'm not sure if there are more issues with drop shims and type parameters that follow after that one, because I've not got that far.

davidtwco (Mar 11 2020 at 13:50, on Zulip):

nikomatsakis said:

one thing I don't know: in your PR, if we determine a parameter is unused, during monomorphization, are we using ty::Param as its value?

We replace unused parameters with the identity substitution.

nikomatsakis (Mar 11 2020 at 13:51, on Zulip):

ok so

nikomatsakis (Mar 11 2020 at 13:51, on Zulip):

let me go look at instance but

nikomatsakis (Mar 11 2020 at 13:51, on Zulip):

well let me go look at instance, I sort of understand

nikomatsakis (Mar 11 2020 at 13:51, on Zulip):

davidtwco said:

We replace unused parameters with the identity substitution.

OK, I think that's a "yes"

davidtwco (Mar 11 2020 at 13:51, on Zulip):

It is, sorry.

Eh2406 (Mar 11 2020 at 13:55, on Zulip):

Btw, I'm enjoying being a fly on the wall for this conversation. Thanks for having it in the open!

nikomatsakis (Mar 11 2020 at 13:56, on Zulip):

OK, I swas looking at InstanceDef

nikomatsakis (Mar 11 2020 at 13:56, on Zulip):

I guess I should review what @eddyb wrote earlier

davidtwco (Mar 11 2020 at 13:57, on Zulip):

I only really started understanding the ICE that I spent Sunday looking at after reading @eddyb's earlier messages today.

nikomatsakis (Mar 11 2020 at 13:59, on Zulip):

I'm guessing part of the problem is that we treat instance.def as if it were .. huh, what's the term... not something that needs substitution, anyway

nikomatsakis (Mar 11 2020 at 13:59, on Zulip):

like a DefId

davidtwco (Mar 11 2020 at 13:59, on Zulip):

I think so..

nikomatsakis (Mar 11 2020 at 14:01, on Zulip):

OK, I have to run for a bit, I will re-read what @eddyb wrote, but I guess the question for @eddyb is whether they think they've got a solution in mind :)

nikomatsakis (Mar 11 2020 at 14:01, on Zulip):

if so, you should probably do that, and I'll try to catch up :)

nikomatsakis (Mar 11 2020 at 14:02, on Zulip):

I'm trying to find the code where we create an InstanceDef::DropGlue, to start

davidtwco (Mar 11 2020 at 14:04, on Zulip):

https://github.com/rust-lang/rust/blob/303d8aff6092709edd4dbd35b1c88e9aa40bf6d8/src/librustc/ty/instance.rs#L340-L344 -> https://github.com/rust-lang/rust/blob/303d8aff6092709edd4dbd35b1c88e9aa40bf6d8/src/librustc_ty/instance.rs#L35-L44

davidtwco (Mar 11 2020 at 14:07, on Zulip):

You can see that in (1) above, if the type contains type parameters, then that ends up being both in instance.def and instance.substs at (3) (I think), so you end up with the sort of scenario that @eddyb described above:

so drop_in_place::<Vec<#0>> (I'm using #0 but this could be T) produces a MIR body referring to Vec<#0> but which also is going to get monomorphized by { #0 -> Vec<#0> }

eddyb (Mar 11 2020 at 14:07, on Zulip):

@nikomatsakis the problem is that InstanceDef::DropGlue contains a type with parameters in it, and that ends up in the MIR body for the shim, but those parameters shouldn't be substituted by Instance's substs (which are for the DefId that we resolved to a shim, not for the shim itself)

eddyb (Mar 11 2020 at 14:08, on Zulip):

you still need Instance to have substs because we still rely on (instance.def_id(), instance.substs) for e.g. getting the signature

eddyb (Mar 11 2020 at 14:08, on Zulip):

but the MIR body is the one that needs to be treated differently

eddyb (Mar 11 2020 at 14:08, on Zulip):

codegen, miri and MIR inlining all substitute MIR bodies

davidtwco (Mar 11 2020 at 14:09, on Zulip):

That clarifies things a lot.

davidtwco (Mar 11 2020 at 14:10, on Zulip):

So do you think the solution for this is to modify these substitutions in some way so that drop shims can be polymorphic?

eddyb (Mar 11 2020 at 14:11, on Zulip):

yeah

eddyb (Mar 11 2020 at 14:11, on Zulip):

basically only subst for InstanceDef::Item

eddyb (Mar 11 2020 at 14:11, on Zulip):

you'll probably get to this before I do, lol

davidtwco (Mar 11 2020 at 14:11, on Zulip):

I can start on this right away.

davidtwco (Mar 11 2020 at 14:12, on Zulip):

It doesn't sound like a large amount of work..?

eddyb (Mar 11 2020 at 14:12, on Zulip):

https://github.com/rust-lang/rust/pull/69036#issuecomment-597655036

davidtwco (Mar 11 2020 at 14:12, on Zulip):

(in that I might actually be able to do it)

davidtwco (Mar 11 2020 at 14:17, on Zulip):

Could you link where the MIR body gets substituted? I can only find this line - nothing else in that file or at callers of the query.

davidtwco (Mar 11 2020 at 14:19, on Zulip):

Also, would this remove the need for the vtable special-casing in the analysis, @eddyb?

eddyb (Mar 11 2020 at 14:21, on Zulip):

ughhhh who murdered ty::instance

eddyb (Mar 11 2020 at 14:21, on Zulip):

oh wow this is bold https://github.com/rust-lang/rust/commit/0e652c550711b301086b8f5ead2f6c90418fe7a1

eddyb (Mar 11 2020 at 14:21, on Zulip):

I hope it gets querified quick

eddyb (Mar 11 2020 at 14:46, on Zulip):

@davidtwco usually it's in a fn monomorphize or similar

eddyb (Mar 11 2020 at 14:46, on Zulip):

it's something like subst_and_normalize or similar

davidtwco (Mar 11 2020 at 14:47, on Zulip):

I looked through calls to both those and nothing jumped out at me.

eddyb (Mar 11 2020 at 14:47, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_codegen_ssa/mir/mod.rs#L91-L95

nikomatsakis (Mar 11 2020 at 14:48, on Zulip):

@eddyb I'm not sure I understand, let me check if I do.

davidtwco (Mar 11 2020 at 14:48, on Zulip):

Yeah, I just couldn't find the callsite where the MIR body was passed to it, or am I misunderstanding?

eddyb (Mar 11 2020 at 14:48, on Zulip):

@davidtwco ah no only MIR inlining does that, it's a bit absurd to do it because it unnecessarily allocates

eddyb (Mar 11 2020 at 14:49, on Zulip):

monomorphize functions are called on the fly with various bits of the original MIR

davidtwco (Mar 11 2020 at 14:49, on Zulip):

I understand, alright.

davidtwco (Mar 11 2020 at 15:18, on Zulip):

Is there some more specific circumstance than just "not an InstanceDef::Item" where I shouldn't substitute?

davidtwco (Mar 11 2020 at 15:19, on Zulip):

A trivial change that only calls subst_and_normalize_erasing_regions for InstanceDef::Item, and normalize_erasing_regions otherwise doesn't quite work.

davidtwco (Mar 11 2020 at 15:21, on Zulip):

(initially, a ClosureOnceShim had a could not fully normalize `<Self as ops::function::FnOnce<Args>>::Output` , then a DropGlue resulted in a failed to get layout for `*mut T`: the type `T` has an unknown layout if I skip ClosureOnceShim)

eddyb (Mar 11 2020 at 15:24, on Zulip):

@davidtwco hmm can you limit it to these 3 https://github.com/rust-lang/rust/pull/69036/files#diff-44a4a8b18aebfe4cd2261ccc019a1f0fR42-R75

davidtwco (Mar 11 2020 at 15:25, on Zulip):

trying that

davidtwco (Mar 11 2020 at 15:29, on Zulip):

It still ICEs, could not fully normalize `(i32, <F as float::Float>::Int)` somewhere in libcore.

davidtwco (Mar 11 2020 at 15:31, on Zulip):

nevermind, user error

davidtwco (Mar 11 2020 at 15:33, on Zulip):

Alright, that gets me back to the layout error that I sent previously - which makes sense, last time when I stopped doing ClosureOnceShim, the next failure was from a DropGlue, makes sense that it would be first now.

Last update: Apr 03 2020 at 16:00UTC