Implicit conversion from uint32& to uint32* should be reconsidered #39040
Assignees
Labels
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone
Comments
AaronRobinsonMSFT
added
the
area-CodeGen-coreclr
Dotnet-GitSync-Bot
added
the
untriaged
|
cc @dotnet/jit-contrib We still have little to no C++/CLI testing in the runtime repo, which seems unfortunate, given how quirky its IL generation has been. In past releases we had been getting some coverage of this from desktop mirroring of jit changes, but no longer. Actual jit behavior hasn't changed; we can presumably back off on asserting in some "safe" cases (eg address of a local) to reduce the spam from a checked jit. |
@AndyAyersMS That is good to know. Should that information about the local status be contained in the |
|
Something like this should work (rearranged so the extra checking is clearly done only in debug/checked builds): @@ -6881,14 +6881,22 @@ void Compiler::impPopArgsForUnmanagedCall(GenTree* call, CORINFO_SIG_INFO* sig)
for (GenTreeCall::Use& argUse : GenTreeCall::UseList(args))
{
- // We should not be passing gc typed args to an unmanaged call.
GenTree* arg = argUse.GetNode();
+ call->gtFlags |= arg->gtFlags & GTF_GLOB_EFFECT;
+
+#ifdef DEBUG
+ // We should not be passing gc typed args to an unmanaged call.
if (varTypeIsGC(arg->TypeGet()))
{
- assert(!"*** invalid IL: gc type passed to unmanaged call");
- }
+ // Tolerate, if this is clearly a stack address.
+ GenTreeLclVarCommon* lclNode = arg->IsLocalAddrExpr();
- call->gtFlags |= arg->gtFlags & GTF_GLOB_EFFECT;
+ if (lclNode == nullptr)
+ {
+ assert(!"*** invalid IL: gc type passed to unmanaged call");
+ }
+ }
+#endif
`` |
Do these "safe" cases have a risk of exposing us to a crash from #34279 again? |
|
Applying the logic above handles several issues I had found. However it doesn't handle the case in The problem signature is defined in ILDAsm as: JIT dump section: |
We didn't change jit behavior, so we're already exposed. In addition to not asserting, the jit can retype args that resolve to local addresses as native ints to avoid some cases. |
|
Would it be possible to retype all GC byrefs and not just local addresses? |
|
Yes, we could do that. |
|
@AaronRobinsonMSFT I take it you're just running some WPF sample to repro on x86 to repro the failures you're seeing? Would like to confirm my fix gets past all the sticking points. |
AndyAyersMS
removed
the
untriaged
AndyAyersMS
added a commit
to AndyAyersMS/runtime
that referenced
this issue
Jul 10, 2020
Make the jit more robust in cases where the IL producer is passing a byref to an unmanaged caller, by retyping the argument as native int. Allows the jit to produce self-consistent GC info and avoid the issues seen in dotnet#34279, at least for byrefs. Closes dotnet#39040.
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
The implicit conversion was previously performed by the JIT and during subsequent test passes it was discovered that IL stub generated depended on this behavior. The IL stub generation code issue was fixed in ddd458e and an assert was added:
runtime/src/coreclr/src/jit/importer.cpp
Lines 6924 to 6929 in ea2b09b
During a recent investigation of a WPF issue, dotnet/wpf#3226, it was discovered that the C++/CLI compiler also emits IL that depends on the previous behavior. The WPF issue doesn't appear related to this change, but that investigation is on going. The
FontFace::GetFileZero()function exhibits this behavior in aWindows_NT.x64.Checkedbuild.JIT dump section:
At this point it appears that it would be prudent to bring back this behavior. See #35026 (comment).
/cc @AndyAyersMS @BruceForstall @JulieLeeMSFT @jkotas @jeffschwMSFT
The text was updated successfully, but these errors were encountered: