Inject IJW Copy Constructor calls in the JIT instead of in the IL stubs

[jkoritzinsky]

Fixes Issue #100751 and #100033 by injecting copy constructor calls in the JIT instead of injecting them in the IL and then using special hooks/interceptors to handle non-IL-supported/accessible scenarios.

Re-enable GS checks in Reverse P/Invokes now that we handel them correctly.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[AndyAyersMS]

I'm somewhat skeptical that annotating lcl vars in the jit will be sufficient. Isn't the need to invoke a copy ctor a property of the type and not of the variable?

[jkoritzinsky]

In our case, it actually is a property of the variable. In this particular scenario, the modifier means "this variable needs a copy constructor to be used whenever the value is copied, but the C++ compiler is not generating the body of this method. The runtime needs to emit copy constructor calls in this method."

[jakobbotsch]

This is a bit too viral for my taste. It is basically adding a new type of local in the JIT that cannot be bitwise copied. All current and future transformations within the JIT need to account for this kind of local with its special rules. Also, it does not seem right to me that internal copies for this type of local results in insertion of user visible calls. For example, simple primitive functions like gtSplitTree will result in additional calls to the copy constructor.

I think instead we should restrict the insertion of the copy constructors to the few places we expect the copy to be user visible. Particularly we know that if the address of an IL local is taken, then it is going to be stable throughout the method. Because of that I do not think that internal copies made by the JIT need to call the copy constructor. Rather, we need to call the copy constructor at just the boundaries, before the address is stable and at the copy that the IL stub is inserting in the IL:

1. As you noticed, the shadow parameters results in user visible differences for the caller argument address and the callee argument address. I think the shadow parameters pass should be responsible for inserting the call to the copy constructor for this case. This is the boundary going into the function, where the local's address may change before being "published" to the IL. I don't think we have other places like that for address exposed variables.
2. For the copy in the IL, I assume it always comes in the form of the struct being pushed as an argument to the call. Can we instead track somewhere (either as a side table in the JIT or as information on the argument that we query the VM for) that the argument has an associated source IL local that the copy constructor must be invoked for? Then lowering would just call the copy constructor with <address of argument>, LCL_ADDR <that local that we got from the VM/side table>.

Of course this works based on the fact that the IL patterns here are very restricted, but I really would like to avoid that we try to teach the current JIT and all future changes to the JIT about the fact that types with copy constructors exist.

Also, what about the destructor? When we insert one of these copy constructors it seems we inevitably also need to insert a destructor somewhere.

[jkoritzinsky]

If I revert 390185c then that moves the handling for shadow parameters and spills into the GS check/importer level. However, that still leaves the PUTARG_STK handling to happen in lowering.

If you have a recommendation for a better way to handle that case (and get it out of lowering) than how I'm currently doing it, I'm open for suggestions.

Of course this works based on the fact that the IL patterns here are very restricted, but I really would like to avoid that we try to teach the current JIT and all future changes to the JIT about the fact that types with copy constructors exist.

I'm fine with only supporting restricted IL patterns, but I'd like suggestions of where to put asserts to ensure that any other IL patterns cause JIT asserts.

Also, what about the destructor? When we insert one of these copy constructors it seems we inevitably also need to insert a destructor somewhere.

In this case (at least the way that it's traditionally been structured), when the IL stub does a copy, it actually does "copy constructor of new + destructor of old", which is what I implemented RuntimeHelpers.CopyConstruct<T> to do in this case.

This provides us with the expected behavior as per our implicit contract with the C++/CLI compiler's codegen.

[jakobbotsch]

If you have a recommendation for a better way to handle that case (and get it out of lowering) than how I'm currently doing it, I'm open for suggestions.

I don't think we will be able to get it out of lowering. But what I'm after is that we make use of the fact that the only copies we allow inserting the copy constructor call for are the shadow params one and the stack pushed x86 one to the specific pinvoke call that we expect to see in this IL.
Other internal copies made by the JIT (whether it be CSE's, copies due to splitting nearby trees, promotion of some fields, introduction of temporaries for any reason) should not require any copy constructor invocations.

I'm fine with only supporting restricted IL patterns, but I'd like suggestions of where to put asserts to ensure that any other IL patterns cause JIT asserts.

I'm specifically after allowing the JIT to make (bitwise) copies of these locals as long as it does not alter user visible behavior. Fundamentally once the address is taken of the IL local the JIT has to guarantee that it remains stable, and the transformations it makes cannot alter that visible behavior.

Here is an example: say that your IL stub for some reason is passing a static field as the value of an argument that comes after the IL local with the copy constructor. The JIT is going to change your M(vectorWithCopyConstructor, class.SomeStaticField) into the equivalent of

var copy = vectorWithCopyConstructor;
if (!class.IsInitialized)
  class.Cctor();
M(copy, class.SomeStaticField)

The same form of change can happen for a multitude of reasons internally within the JIT. I would expect that you can hit this particular introduction of a copy today when the JIT runs with STRESS_SPLIT_TREES_RANDOMLY.
With this PR, we will be invoking the copy constructor one more time when the JIT makes this internal transformation. However, I think we should instead have lowering recognize the call to M and introduce the copy constructor call with <address of argument>, LCL_ADDR(vectorWithCopyConstructor), despite the fact that previous transformations within the JIT now means that the bits are in copy instead.

I don't think it is simple to assert anything here, but I don't think the introducing the copy constructor calls more liberally improves the situation. You can still create IL that does not properly maintain the "move" semantics you are implementing here (e.g. two copies from the same IL local would cause problems that would not result in any asserts here).

[jkoritzinsky]

I think I understand better now. I'll try to adjust the implementation implement it with a table in the JIT to do the copy/move from the "original" local into the arg slot for the specific GT_PUTARG_STK case and do the copy/move into the shadow arg for the Reverse P/Invoke case.

[JulieLeeMSFT]

@jkoritzinsky please check test failures.

[jakobbotsch]

JIT changes LGTM, with a few minor things.

[jkoritzinsky]

@jakobbotsch I had to make a few changes in Lowering::LowerSpecialCallArgs because of how arguments are ordered in unmanaged calls. If you could take one more look, I'd appreciate it.

[jakobbotsch]

I think you need some handling for retbufs as well.

[jakobbotsch]

Ah, I guess not... Should be skipped by IsUserArg

[jakobbotsch]

When is this argIndex >= comp->info.compILargsCount true?

[jkoritzinsky]

This fails on the LCID interop tests, in which we introduce an additional argument into the native call that isn't present in the managed P/Invoke signature.

The C++/CLI compiler will never emit a P/Invoke that uses both copy constructors and introduces the LCID argument.

[jakobbotsch]

I guess one way to be completely explicit about it would be to check m_specialCopyArgs != nullptr above and add an assert(call.CountUserArgs() == comp->info.compILargsCount), but this looks ok to me as well.

[jkoritzinsky]

I'll add the assert. Better to be explicit about it, and adding that check will probably help jit throughput (as we won't iterate through the args unless we actually are in a scenario where we may do something).

[jkoritzinsky]

/azp run runtime, runtime-dev-innerloop

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).