Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Codegen generates code referencing RuntimeType for __Canon[] #101175

Closed
MichalStrehovsky opened this issue Apr 17, 2024 · 7 comments · Fixed by #101173
Closed

Codegen generates code referencing RuntimeType for __Canon[] #101175

MichalStrehovsky opened this issue Apr 17, 2024 · 7 comments · Fixed by #101173

Comments

@MichalStrehovsky
Copy link
Member

This may have a repro with JIT, but I hit this with PublishAot:

Activator.CreateInstance(typeof(Foo<>).MakeGenericType(typeof(object)));

class Foo<T>
{
    public Foo() { new T[0].ToString(); }
}

Codegen for Foo..ctor is:

00007FF77E6791D0  push        rbx  
00007FF77E6791D1  sub         rsp,30h  
00007FF77E6791D5  mov         qword ptr [rsp+28h],rcx  
00007FF77E6791DA  mov         rbx,rcx  
00007FF77E6791DD  mov         rcx,qword ptr [rbx]  
00007FF77E6791E0  lea         rcx,[__RuntimeType___Array<System___Canon> (07FF77E6AC918h)]  
00007FF77E6791E7  call        System.RuntimeType__ToString (07FF77E5DF220h)  
00007FF77E6791EC  nop  
00007FF77E6791ED  add         rsp,30h  
00007FF77E6791F1  pop         rbx  
00007FF77E6791F2  ret  

Notice an attempt to do a runtime lookup but then codegen changing it's mind and overwriting rcx with a const frozen object for __Canon[] RuntimeType (which is non-sensical and will crash at runtime).

I'm making the compiler assert on this #101173.

Cc @EgorBo

@MichalStrehovsky MichalStrehovsky added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 17, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Apr 17, 2024
Copy link
Contributor

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

@EgorBo
Copy link
Member

EgorBo commented Apr 17, 2024

I don't see any issues in the codegen for CoreCLR here, I think the issue is in the NAOT's implementation for getRuntimeTypePointer API, e.g. in CoreCLR it filters out all shared types: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/jitinterface.cpp#L6191-L6215

Because it's fine for VM internally to allocate cannonical type objects on FOH, isn't? it's just that JIT should never see raw constant handles for them

@MichalStrehovsky
Copy link
Member Author

Why does codegen even attempt to do this if there is a runtime lookup? Is it difficult to abort this if there is a runtime lookup? The typeHnd.IsCanonicalSubtype() honestly feels like something that should be an assert, unless it's difficult for RyuJIT to know this requires a runtime lookup.

(In theory, on native AOT side it would be easy to allow embedding these into generic dictionaries, so one could actually have a runtime lookup for a frozen RuntimeType instance.)

@EgorBo
Copy link
Member

EgorBo commented Apr 17, 2024

Why does codegen even attempt to do this if there is a runtime lookup? Is it difficult to abort this if there is a runtime lookup

I think it happens just because JIT doesn't expect EE for getRuntimeTypePointer to return cannonical type so it doesn't bother checking. It can be fixed on all uses of getRuntimeTypePointer in JIT to check CLS for being info.compCompHnd->getClassAttribs(handle) & CORINFO_FLG_SHAREDINST) == 0, but I think it's better to do on VM side (less changes, less chances for future bugs).

Here is the place where we create it for this case:

// Try to fold obj.GetType() if we know the exact type of obj.
bool isExact = false;
bool isNonNull = false;
CORINFO_CLASS_HANDLE cls = gtGetClassHandle(tree->gtGetOp1(), &isExact, &isNonNull);
if ((cls != NO_CLASS_HANDLE) && isExact && isNonNull)
{
CORINFO_OBJECT_HANDLE typeObj = info.compCompHnd->getRuntimeTypePointer(cls);
if (typeObj != nullptr)

N010 ( 74, 25) [000019] --CXG------                         *  CALL      ref    System.RuntimeType:ToString():System.String:this
N009 ( 60, 19) [000018] --CXG------ this in rcx             \--*  INTRINSIC ref    objGetType
N008 ( 24, 15) [000010] --CXG------                            \--*  CALL help ref    CORINFO_HELP_NEWARR_1_VC
N006 (  9,  7) [000008] #--X------- arg0 in rcx                   +--*  IND       long  
N005 (  6,  5) [000007] #--X-------                               |  \--*  IND       long  
N004 (  4,  3) [000006] ---X---N---                               |     \--*  ADD       long  
N002 (  3,  2) [000004] #--X-------                               |        +--*  IND       long  
N001 (  1,  1) [000003] !----------                               |        |  \--*  LCL_VAR   ref    V00 this         u:1
N003 (  1,  1) [000005] -----------                               |        \--*  CNS_INT   long   48
N007 (  1,  1) [000002] ----------- arg1 in rdx                   \--*  CNS_INT   long   0

N001 [000003]   LCL_VAR   V00 this         u:1 => $80 {InitVal($40)}
IND(obj) is actually a class handle for Foo`1[System.__Canon]

@MichalStrehovsky
Copy link
Member Author

OK, I'll add this on the VM side then.

Can you quickly point me to where are the tests for these?

@MichalStrehovsky MichalStrehovsky added area-NativeAOT-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Apr 17, 2024
Copy link
Contributor

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

@EgorBo
Copy link
Member

EgorBo commented Apr 17, 2024

Can you quickly point me to where are the tests for these?

I don't think there are dedicated tests for RuntimeType being frozen, but there are a lot of typeof(T) related tests in runtime tests. Typically, for bugs, we add new tests under https://github.com/dotnet/runtime/tree/main/src/tests/JIT/Regression/JitBlue with github Id as a name

@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants