Fix copy constructor injection for unsafe value classes #105779
Conversation
… in JitStress (as they won't be fixed when the JIT introduces these extra checks when not asking the VM)
|
cc: @jkotas @AaronRobinsonMSFT for thoughts on the best way to fix this. |
|
/cc @dotnet/jit-contrib |
Agree. Can we do this for .NET 9? |
What would this JIT/EE interface modification look like? |
src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs
Outdated
Show resolved
Hide resolved
| // can break C++/CLI's copy-constructor semantics by missing copies. | ||
| if (pClass->IsUnsafeValueClass() | ||
| && !(m_pMethodBeingCompiled->IsILStub() | ||
| && dac_cast<PTR_DynamicMethodDesc>(m_pMethodBeingCompiled)->GetILStubType() == DynamicMethodDesc::StubNativeToCLRInterop)) |
There was a problem hiding this comment.
I think it may be better to introduce a new bit that says "do not create a shadow copies of this argument, this argument must be stored in its natural location".
Fixing it like this on the VM side looks reasonable to me.
|
I was thinking of using a bit on I don't like fixing it by dropping the shadow variable because it defeats the GS cookie buffer overflow protections. |
Does it meant that we would or would not call the user provided constructor/destructor depending on the optimization settings? It does not sound right to me. |
|
No, we'd always call the copy constructor no matter the optimization level. Because this flag would only be set in the IL stubs (due to when the C++/CLI compiler emits it), we'd only do this particular optimization in places where the JIT emits a copy that the IL stub cannot predict (or predict easily, for the P/Invoke case). Basically, we'd utilize the flag in at most two places:
Since the VM will control the IL that the JIT sees with this modreq (as it is only used on P/Invokes and Reverse P/Invokes where the C++/CLI compiler can't emit the copy constructor calls itself) and we won't emit code that would do copies depending on optimization level, the number of times we call the copy constructor or destructor will not differ based on optimization level. |
|
Does it mean that there would be a new call of the copy constructor that is not there today? (ie the change would be user observable) |
|
Yes, there would be a new call to the copy constructor that isn't there today (as we're currently missing a call). Previously we have not been concerned about introducing additional copies where necessary for validity. (Additionally, if we were to implement the second of the two items above, we should be able to remove a call from the P/Invoke stubs). |
|
Does it mean that the fix would assume that the user type constructors are correctly implemented according to the C++ rules? I am not sure whether it is safe to assume that and how much real-world code it can break. |
|
The only assumptions the fix would make would be about the IL we emit (which I think we could guard with We already don't assume that copy constructors are implemented in any particular way. We just make sure to call them each time we make a copy that could eventually be passed out of the stub. |
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/backport to release/8.0-staging |
|
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/10222891618 |
|
@jkoritzinsky backporting to release/8.0-staging failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Add failing test case from distilled down stress repro
Applying: Disable GS checks in Reverse P/Invoke stubs to avoid missing these copies.
Applying: Since the fix is in the CoreCLR VM side, we need to disable the tests in JitStress (as they won't be fixed when the JIT introduces these extra checks when not asking the VM)
error: sha1 information is lacking or useless (src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0003 Since the fix is in the CoreCLR VM side, we need to disable the tests in JitStress (as they won't be fixed when the JIT introduces these extra checks when not asking the VM)
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@jkoritzinsky an error occurred while backporting to release/8.0-staging, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
For C++/CLI types with C-style array fields (
int x[N];), the JIT was introducing extra copies without calling the copy constructor.This was discovered through JitStress, which forcibly set the flag that the C++/CLI compiler sets when it sees a C-style array field.
To avoid adding a new API to the JIT interface just for usage by C++/CLI, I made a targeted fix to disable the GS cookie checking in Reverse P/Invoke stubs as we control the IL and we don't emit IL that would interact with these fields in a dangerous manner. As this fix was in CoreCLR and not propagated to the JIT, I still had to disable the tests that were failing from JitStress as setting the flag for the local in the JIT will still cause the bad behavior.
Alternatively, we could enhance the JIT interface to enable the JIT to discover when a type has a copy constructor and do one of the following:
GT_CALLnodes to the copy constructor and destructor instead of generating aGT_STORE_LCL_VARnode.If we were to go the second route, then I'd prefer moving all the "copy construct into the arg's stack slot" logic into the JIT and remove the code we added in the IL marshallers that supports that case.
I think a fix for this bug is worth considering backporting (as it would likely lead to hard-to-diagnose bugs and violates C++/CLI semantics). We may want a more targeted fix like this instead of a more expansive fix for backporting, I'm not sure.
Fixes #100751
Related to #100033