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.

Sign up for GitHub

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

Jump to bottom

[RISC-V] Avoid using native layout info to calculate register flags for small structs where possible #97877

Merged
merged 13 commits into from
Mar 15, 2024
Merged

[RISC-V] Avoid using native layout info to calculate register flags for small structs where possible #97877

merged 13 commits into from
Mar 15, 2024

Conversation

tomeksowi
Copy link
Contributor

@tomeksowi tomeksowi commented Feb 2, 2024 •
edited
Loading

Calculating uninitialized native layout info during GC stack walks may cause ArgIterator to trigger undue class loads above level approxparents. It also calculated interop marshaling info which we shouldn't need for managed-managed calls. Avoid using native layout where possible.

This fixes the following tests failing intermittently on assert(level == CLASS_LOAD_APPROXPARENTS) with crossgen2 System.Private.CoreLib.dll:

  • JIT/opt/OSR/pinnedlocal/pinnedlocal.sh
  • GC/Regressions/Github/Runtime_76219/Runtime_76219/Runtime_76219.sh
  • readytorun/DynamicMethodGCStress/DynamicMethodGCStress/DynamicMethodGCStress.sh

Part of #84834
cc @wscho77 @HJLeee @clamp03 @JongHeonChoi @t-mustafin @gbalykov @viewizard @ashaurtaev @brucehoult @sirntar @yurai007 @Bajtazar

@tomeksowi
[RISC-V] Skip ComputeReturnFlags in ArgIteratorTemplate::HasRetBuffArg
c4f7e13 
ComputeReturnFlags on RISC-V is complicated due to ABI rules on enregistering small structs and it can even potentially trigger an unnecessary class load. So take a shortcut when we only need to check whether a struct is returned in a buffer.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 2, 2024
@build-analysis build-analysis bot mentioned this pull request Feb 2, 2024
Closed
@clamp03 clamp03 added the arch-riscv Related to the RISC-V architecture label Feb 3, 2024
@tomeksowi tomeksowi marked this pull request as ready for review February 5, 2024 08:38
@clamp03 clamp03 requested a review from jkotas February 6, 2024 00:42
@jkotas
Copy link
Member

jkotas commented Feb 6, 2024

This fixes the following tests failing intermittently on assert(level == CLASS_LOAD_APPROXPARENTS) with crossgen2

What is the stacktrace of this assert?

As you have said, this fix is an unreliable workaround for the actual problem.

At any rate, an early return here is useful.

It is only an improvement if the arg iterator is called to compute HasRetBuffArg and nothing else. It is a regression for more typical use of arg iterator that needs most of the things computed.

@tomeksowi
Copy link
Contributor Author

tomeksowi commented Feb 6, 2024 •
edited
Loading

What is the stacktrace of this assert?

On latest main (67604d3) it doesn't repro on Debug nor under a debugger (neither GDB stub on QEMU, LLDB on Vision Five 2). This is from Checked so the stack trace is a bit spotty.

src/coreclr/vm/siginfo.cpp:1106 SigPointer::GetTypeHandleThrowing(ModuleBase*, SigTypeContext const*, ClassLoader::LoadTypesFlag, ClassLoadLevel, int, Substitution const*, ZapSig::Context const*, MethodTable*, SigPointer::HandleRecursiveGenericsForFieldLayoutLoad*) const
src/coreclr/vm/clsload.cpp:1822 ClassLoader::LoadTypeDefOrRefOrSpecThrowing(Module*, unsigned int, SigTypeContext const*, ClassLoader::NotFoundAction, ClassLoader::PermitUninstantiatedFlag, ClassLoader::LoadTypesFlag, ClassLoadLevel, int, Substitution const*, MethodTable*)
src/coreclr/vm/methodtablebuilder.cpp:9386  MethodTableBuilder::LoadExactInterfaceMap(MethodTable*)
src/coreclr/vm/class.cpp:1093   ClassLoader::LoadExactParentAndInterfacesTransitively(MethodTable*)
src/coreclr/vm/methodtable.inl:31   MethodTable::GetClassWithPossibleAV()
src/coreclr/vm/clsload.cpp:2645 ClassLoader::DoIncrementalLoad(TypeKey const*, TypeHandle, ClassLoadLevel)
src/coreclr/vm/clsload.cpp:3277 ClassLoader::LoadTypeHandleForTypeKey_Body(TypeKey const*, TypeHandle, ClassLoadLevel)
src/coreclr/vm/clsload.cpp:3009 ClassLoader::LoadTypeHandleForTypeKey(TypeKey const*, TypeHandle, ClassLoadLevel, InstantiationContext const*)
src/coreclr/vm/clsload.cpp:1998 ClassLoader::LoadTypeDefThrowing(Module*, unsigned int, ClassLoader::NotFoundAction, ClassLoader::PermitUninstantiatedFlag, unsigned int, ClassLoadLevel, Instantiation*)
clsload.cpp:?  ??
src/coreclr/vm/clsload.cpp:386  ClassLoader::LoadTypeHandleThrowIfFailed(NameHandle*, ClassLoadLevel, Module*)
src/coreclr/vm/binder.cpp:71    CoreLibBinder::LookupClassLocal(BinderClassID)
src/coreclr/vm/binder.h:341 CoreLibBinder::GetClass(BinderClassID)
src/coreclr/inc/volatile.h:272 void VolatileStore<EEClassNativeLayoutInfo*>(EEClassNativeLayoutInfo**, EEClassNativeLayoutInfo*)
src/coreclr/vm/methodtable.inl:31   MethodTable::GetClassWithPossibleAV()
src/coreclr/vm/fieldmarshaler.h:249 EEClassNativeLayoutInfo::GetNumFields() const
src/coreclr/vm/callingconvention.h:2043 ArgIteratorTemplate<ArgIteratorBase>::ComputeReturnFlags()
src/coreclr/vm/callingconvention.h:423  ArgIteratorTemplate<ArgIteratorBase>::HasRetBuffArg()
??:?    GetCLRRuntimeHost
??:?    GetCLRRuntimeHost
src/coreclr/vm/stackwalk.cpp:847    Thread::MakeStackwalkerCallback(CrawlFrame*, StackWalkAction (*)(CrawlFrame*, void*), void*, unsigned int)
??:?    GetCLRRuntimeHost
src/coreclr/vm/stackwalk.cpp:1010   Thread::StackWalkFrames(StackWalkAction (*)(CrawlFrame*, void*), void*, unsigned int, Frame*)
src/coreclr/vm/threads.inl:37   GetThreadNULLOk()
src/coreclr/vm/gcenv.ee.cpp:304 GCToEEInterface::GcScanRoots(void (*)(Object**, ScanContext*, unsigned int), int, int, ScanContext*)
src/coreclr/gc/gc.cpp:29217 WKS::gc_heap::mark_phase(int)
src/coreclr/gc/gc.cpp:12512 WKS::gc_heap::check_gen0_bricks()
src/coreclr/gc/gc.cpp:24190 WKS::gc_heap::garbage_collect(int)
src/coreclr/gc/gc.cpp:50292 WKS::GCHeap::GarbageCollectGeneration(unsigned int, gc_reason)
??:?    GetCLRRuntimeHost
??:?    GetCLRRuntimeHost
??:?    GetCLRRuntimeHost
src/coreclr/vm/threadsuspend.cpp:2376   Thread::RareEnablePreemptiveGC()
src/coreclr/vm/threads.inl:42   GetThread()
src/coreclr/vm/prestub.cpp:2424 ??
src/coreclr/vm/riscv64/asmhelpers.S:379 ??

The context at the assertion in GetTypeHandleThrowing() is: IsGCThread()=1, IsStackWalkerThread()=0, level=2(CLASS_LOAD_EXACTPARENTS), dropGenericArgumentLevel=1

As you have said, this fix is an unreliable workaround for the actual problem.

Yes, the idea was to get it out of the way asap, then continue hunt for the actual problem. Solving the actual problem first would be much better, of course.

At any rate, an early return here is useful.

It is only an improvement if the arg iterator is called to compute HasRetBuffArg and nothing else. It is a regression for more typical use of arg iterator that needs most of the things computed.

True, but by "regression" you mean perf degradation? I don't see the fix introducing logical errors. This fix could be amended by narrowing the early return only to ELEMENT_TYPE_VALUETYPE and valueType.GetSize() <= ENREGISTERED_RETURNTYPE_INTEGER_MAXSIZE, otherwise calling ComputeReturnFlags() normally. But still, that's patching not solving the underlying issue :/

@jkotas
Copy link
Member

jkotas commented Feb 7, 2024

CLASS_LOAD_EXACTPARENTS

This is likely the problem. The stackwalking during GC should be only loading types up to CLASS_LOAD_EXACTPARENTS.

src/coreclr/vm/callingconvention.h:2043 ArgIteratorTemplate::ComputeReturnFlags()

I see this calls MethodTable::GetRiscV64PassStructInRegisterFlags. Are you able to tell the line number in MethodTable::GetRiscV64PassStructInRegisterFlags that triggers the bad type load? I think it is where the fix needs to be.

True, but by "regression" you mean perf degradation?

Yes.

@tomeksowi
Copy link
Contributor Author

tomeksowi commented Feb 7, 2024 •
edited
Loading

I see this calls MethodTable::GetRiscV64PassStructInRegisterFlags. Are you able to tell the line number in MethodTable::GetRiscV64PassStructInRegisterFlags that triggers the bad type load? I think it is where the fix needs to be.

It's GetNativeLayoutInfo(). The type which triggers the cascade is ReadOnlySpan<T>. Probably sth's wrong for generics, other non-generic types pass through the same path fine.

Nicer stack trace with context and some functions deoptimized 
SigPointer::GetTypeHandleThrowing(ModuleBase*, SigTypeContext const*, ClassLoader::LoadTypesFlag, ClassLoadLevel, int, Substitution const*, ZapSig::Context const*, MethodTable*, SigPointer::HandleRecursiveGenericsForFieldLayoutLoad*) const at /runtime/src/coreclr/vm/siginfo.cpp:1106
1104  : 
1105  :             void* buffer[128];
1106 >:             int n = backtrace(buffer, sizeof buffer / sizeof *buffer);
1107  :             backtrace_symbols_fd(buffer, n, 1);
1108  :             
ClassLoader::LoadTypeDefOrRefOrSpecThrowing(Module*, unsigned int, SigTypeContext const*, ClassLoader::NotFoundAction, ClassLoader::PermitUninstantiatedFlag, ClassLoader::LoadTypesFlag, ClassLoadLevel, int, Substitution const*, MethodTable*) at /runtime/src/coreclr/vm/clsload.cpp:1825
1823  :                                                           level, dropGenericArgumentLevel, pSubst, (const ZapSig::Context *)0, pMTInterfaceMapOwner);
1824  : #ifndef DACCESS_COMPILE
1825 >:         if ((fNotFoundAction == ThrowIfNotFound) && typeHnd.IsNull())
1826  :             pModule->GetAssembly()->ThrowTypeLoadException(pInternalImport, typeDefOrRefOrSpec,
1827  :                                                            IDS_CLASSLOAD_GENERAL);
MethodTableBuilder::LoadExactInterfaceMap(MethodTable*) at /runtime/src/coreclr/vm/methodtablebuilder.cpp:9386
9384  :         while ((hr = ie.Next()) == S_OK)
9385  :         {
9386 >:             MethodTable *pNewIntfMT = ClassLoader::LoadTypeDefOrRefOrSpecThrowing(pMT->GetModule(),
9387  :                                                                                 ie.CurrentToken(),
9388  :                                                                                 &typeContext,
ClassLoader::LoadExactParentAndInterfacesTransitively(MethodTable*) at /runtime/src/coreclr/vm/class.cpp:1093
1091  : 
1092  : #ifdef _DEBUG
1093 >:     if (g_pConfig->ShouldDumpOnClassLoad(pMT->GetDebugClassName()))
1094  :     {
1095  :         pMT->Debug_DumpInterfaceMap("Exact");
ClassLoader::LoadExactParents(MethodTable*) at /runtime/src/coreclr/vm/class.cpp:1199
1197  :     LoadExactParentAndInterfacesTransitively(pMT);
1198  : 
1199 >:     if (pMT->GetClass()->HasVTableMethodImpl())
1200  :     {
1201  :         MethodTableBuilder::CopyExactParentSlots(pMT);
ClassLoader::DoIncrementalLoad(TypeKey const*, TypeHandle, ClassLoadLevel) at /runtime/src/coreclr/vm/clsload.cpp:2648
2646  :     }
2647  : 
2648 >:     if (typeHnd.GetLoadLevel() >= CLASS_LOAD_EXACTPARENTS)
2649  :     {
2650  :         Notify(typeHnd);
ClassLoader::LoadTypeHandleForTypeKey_Body(TypeKey const*, TypeHandle, ClassLoadLevel) at /runtime/src/coreclr/vm/clsload.cpp:3280
3278  :         while (currentLevel < targetLevel)
3279  :         {
3280 >:             typeHnd = DoIncrementalLoad(pTypeKey, typeHnd, currentLevel);
3281  :             CONSISTENCY_CHECK(typeHnd.GetLoadLevel() > currentLevel);
3282  :             currentLevel = typeHnd.GetLoadLevel();
ClassLoader::LoadTypeHandleForTypeKey(TypeKey const*, TypeHandle, ClassLoadLevel, InstantiationContext const*) at /runtime/src/coreclr/vm/clsload.cpp:3012
3010  :     if (currentLevel < targetLevelUnderLock)
3011  :     {
3012 >:         typeHnd = LoadTypeHandleForTypeKey_Body(pTypeKey,
3013  :                                                 typeHnd,
3014  :                                                 targetLevelUnderLock);
ClassLoader::LoadTypeDefThrowing(Module*, unsigned int, ClassLoader::NotFoundAction, ClassLoader::PermitUninstantiatedFlag, unsigned int, ClassLoadLevel, Instantiation*) at /runtime/src/coreclr/vm/clsload.cpp:2001
1999  :             {
2000  :                 TypeKey typeKey(pModule, typeDef);
2001 >:                 typeHnd = pModule->GetClassLoader()->LoadTypeHandleForTypeKey(&typeKey,
2002  :                                                                               typeHnd,
2003  :                                                                               level);
ClassLoader::LoadTypeHandleThrowing(NameHandle*, ClassLoadLevel, Module*) at /runtime/src/coreclr/vm/clsload.cpp:0
1  : // Licensed to the .NET Foundation under one or more agreements.
2  : // The .NET Foundation licenses this file to you under the MIT license.
3  : //
4  : // File: clsload.cpp
5  : //
ClassLoader::LoadTypeHandleThrowIfFailed(NameHandle*, ClassLoadLevel, Module*) at /runtime/src/coreclr/vm/clsload.cpp:387
385  : 
386  :     // Lookup in the classes that this class loader knows about
387 >:     TypeHandle typeHnd = LoadTypeHandleThrowing(pName, level, pLookInThisModuleOnly);
388  : 
389  :     if(typeHnd.IsNull()) {
ClassLoader::LoadTypeByNameThrowing(Assembly*, NameHandle*, ClassLoader::NotFoundAction, ClassLoader::LoadTypesFlag, ClassLoadLevel) at /runtime/src/coreclr/vm/clsload.cpp:347
345  :     ClassLoader* classLoader = pAssembly->GetLoader();
346  :     if (fNotFound == ClassLoader::ThrowIfNotFound)
347 >:         RETURN classLoader->LoadTypeHandleThrowIfFailed(pNameHandle, level);
348  :     else
349  :         RETURN classLoader->LoadTypeHandleThrowing(pNameHandle, level);
CoreLibBinder::LookupClassLocal(BinderClassID) at /runtime/src/coreclr/vm/binder.cpp:71
69  :     {
70  :         NameHandle nameHandle = NameHandle(nameSpace, name);
71 >:         pMT = ClassLoader::LoadTypeByNameThrowing(GetModule()->GetAssembly(), &nameHandle).AsMethodTable();
72  :     }
73  :     else
CoreLibBinder::GetClass(BinderClassID) at /runtime/src/coreclr/vm/binder.h:342
340  :     PTR_MethodTable pMT = VolatileLoad(&((&g_CoreLib)->m_pClasses[id]));
341  :     if (pMT == NULL)
342 >:         return LookupClass(id);
343  :     return pMT;
344  : }
EEClassNativeLayoutInfo::CollectNativeLayoutFieldMetadataThrowing(MethodTable*) at /runtime/src/coreclr/vm/classlayoutinfo.cpp:1029
1027  :         // from the managed size and alignment.
1028  :         // Crossgen scenarios block Vector<T> from even being loaded, so only do this check when not in crossgen.
1029 >:         if (pMT->HasSameTypeDefAs(CoreLibBinder::GetClass(CLASS__VECTORT)))
1030  :         {
1031  :             pNativeLayoutInfo->m_size = pEEClassLayoutInfo->GetManagedSize();
EEClassNativeLayoutInfo::InitializeNativeLayoutFieldMetadataThrowing(MethodTable*) at /runtime/src/coreclr/vm/classlayoutinfo.cpp:844
842  :         if (pClass->GetNativeLayoutInfo() == nullptr)
843  :         {
844 >:             EEClassNativeLayoutInfo* pNativeLayoutInfo = CollectNativeLayoutFieldMetadataThrowing(pMT);
845  : #ifdef FEATURE_HFA
846  :             CheckForNativeHFA(pMT, pNativeLayoutInfo);
MethodTable::EnsureNativeLayoutInfoInitialized() at /runtime/src/coreclr/vm/methodtable.cpp:9289
9287  : #ifndef DACCESS_COMPILE
9288  :     EEClassNativeLayoutInfo::InitializeNativeLayoutFieldMetadataThrowing(this);
9289 >:     return this->GetClass()->GetNativeLayoutInfo();
9290  : #else
9291  :     DacNotImpl();
MethodTable::GetNativeLayoutInfo() at /runtime/src/coreclr/vm/methodtable.cpp:9274
9272  :         return pNativeLayoutInfo;
9273  :     }
9274 >:     return EnsureNativeLayoutInfoInitialized();
9275  : }
9276  : 
MethodTable::GetRiscV64PassStructInRegisterFlags(CORINFO_CLASS_STRUCT_*) at /runtime/src/coreclr/vm/methodtable.cpp:3729
3727  :         if (th.GetSize() <= 16 /*MAX_PASS_MULTIREG_BYTES*/)
3728  :         {
3729 >:             DWORD numIntroducedFields = pMethodTable->GetNativeLayoutInfo()->GetNumFields();
3730  :             FieldDesc *pFieldStart = nullptr;
3731  : 
ArgIteratorTemplate<ArgIteratorBase>::ComputeReturnFlags() at /runtime/src/coreclr/vm/callingconvention.h:2043
2041  : 
2042  :                 MethodTable *pMethodTable = thValueType.AsMethodTable();
2043 >:                 flags = (MethodTable::GetRiscV64PassStructInRegisterFlags((CORINFO_CLASS_HANDLE)pMethodTable) & 0xff) << RETURN_FP_SIZE_SHIFT;
2044  :                 break;
2045  :             }
ArgIteratorTemplate<ArgIteratorBase>::HasRetBuffArg() at /runtime/src/coreclr/vm/callingconvention.h:423
421  :         if (!(m_dwFlags & RETURN_FLAGS_COMPUTED))
422  :             ComputeReturnFlags();
423 >:         return (m_dwFlags & RETURN_HAS_RET_BUFFER);
424  :     }
425  : 
TransitionFrame::PromoteCallerStack(void (*)(Object**, ScanContext*, unsigned int), ScanContext*) at /runtime/src/coreclr/vm/frames.cpp:0
1  : // Licensed to the .NET Foundation under one or more agreements.
2  : // The .NET Foundation licenses this file to you under the MIT license.
3  : // FRAMES.CPP
4  : 
5  : 
GcStackCrawlCallBack(CrawlFrame*, void*) at /runtime/src/coreclr/vm/gcenv.ee.common.cpp:0
1  : // Licensed to the .NET Foundation under one or more agreements.
2  : // The .NET Foundation licenses this file to you under the MIT license.
3  : 
4  : #include "common.h"
5  : #include "gcenv.h"
Thread::MakeStackwalkerCallback(CrawlFrame*, StackWalkAction (*)(CrawlFrame*, void*), void*, unsigned int) at /runtime/src/coreclr/vm/stackwalk.cpp:847
845  :     CLEAR_THREAD_TYPE_STACKWALKER();
846  : 
847 >:     StackWalkAction swa = pCallback(pCF, (VOID*)pData);
848  : 
849  :     SET_THREAD_TYPE_STACKWALKER(this);
Thread::StackWalkFramesEx(REGDISPLAY*, StackWalkAction (*)(CrawlFrame*, void*), void*, unsigned int, Frame*) at /runtime/src/coreclr/vm/stackwalk.cpp:0
1  : // Licensed to the .NET Foundation under one or more agreements.
2  : // The .NET Foundation licenses this file to you under the MIT license.
3  : // STACKWALK.CPP
4  : 
5  : 
Thread::StackWalkFrames(StackWalkAction (*)(CrawlFrame*, void*), void*, unsigned int, Frame*) at /runtime/src/coreclr/vm/stackwalk.cpp:1010
1008  : #endif
1009  : 
1010 >:     return StackWalkFramesEx(&rd, pCallback, pData, flags, pStartFrame);
1011  : }
1012  : 
GetThreadNULLOk() at /runtime/src/coreclr/vm/threads.inl:37
35  : inline Thread* GetThreadNULLOk()
36  : {
37 >:     return gCurrentThreadInfo.m_pThread;
38  : }
39  : 
GCToEEInterface::GcScanRoots(void (*)(Object**, ScanContext*, unsigned int), int, int, ScanContext*) at /runtime/src/coreclr/vm/gcenv.ee.cpp:304
302  : #endif // FEATURE_EVENT_TRACE
303  :             ScanStackRoots(pThread, fn, sc);
304 >:             ScanTailCallArgBufferRoots(pThread, fn, sc);
305  : #ifdef FEATURE_EVENT_TRACE
306  :             sc->dwEtwRootKind = kEtwGCRootKindOther;
WKS::gc_heap::mark_phase(int) at /runtime/src/coreclr/gc/gc.cpp:29217
29215  :                                 condemned_gen_number, max_generation,
29216  :                                 &sc);
29217 >:         drain_mark_queue();
29218  :         fire_mark_event (ETW::GC_ROOT_STACK, current_promoted_bytes, last_promoted_bytes);
29219  : 
WKS::gc_heap::check_gen0_bricks() at /runtime/src/coreclr/gc/gc.cpp:12512
12510  : {
12511  : //#ifdef _DEBUG
12512 >:     if (gen0_bricks_cleared)
12513  :     {
12514  : #ifdef USE_REGIONS
WKS::gc_heap::garbage_collect(int) at /runtime/src/coreclr/gc/gc.cpp:24190
24188  :     }
24189  : #ifndef MULTIPLE_HEAPS
24190 >:     allocation_running_time = GCToOSInterface::GetLowPrecisionTimeStamp();
24191  :     allocation_running_amount = dd_new_allocation (dynamic_data_of (0));
24192  :     fgn_last_alloc = dd_new_allocation (dynamic_data_of (0));
WKS::GCHeap::GarbageCollectGeneration(unsigned int, gc_reason) at /runtime/src/coreclr/gc/gc.cpp:50292
50290  :         BEGIN_TIMING(gc_during_log);
50291  :         pGenGCHeap->garbage_collect (condemned_generation_number);
50292 >:         if (gc_heap::pm_trigger_full_gc)
50293  :         {
50294  :             pGenGCHeap->garbage_collect_pm_full_gc();
WKS::GCHeap::GarbageCollectTry(int, int, int) at /runtime/src/coreclr/gc/gc.cpp:0
1  : // Licensed to the .NET Foundation under one or more agreements.
2  : // The .NET Foundation licenses this file to you under the MIT license.
3  : 
4  : 
5  : //
WKS::GCHeap::StressHeap(gc_alloc_context*) at /runtime/src/coreclr/gc/gc.cpp:0
1  : // Licensed to the .NET Foundation under one or more agreements.
2  : // The .NET Foundation licenses this file to you under the MIT license.
3  : 
4  : 
5  : //
Thread::PerformPreemptiveGC() at /runtime/src/coreclr/vm/threadsuspend.cpp:0
1  : // Licensed to the .NET Foundation under one or more agreements.
2  : // The .NET Foundation licenses this file to you under the MIT license.
3  : //
4  : // threadsuspend.CPP
5  : //
Thread::RareEnablePreemptiveGC() at /runtime/src/coreclr/vm/threadsuspend.cpp:2376
2374  : #endif
2375  : 
2376 >:     STRESS_LOG1(LF_SYNC, LL_INFO100000, "RareEnablePreemptiveGC: entering. Thread state = %x\n", m_State.Load());
2377  :     if (!ThreadStore::HoldingThreadStore(this))
2378  :     {
GetThread() at /runtime/src/coreclr/vm/threads.inl:42
40  : inline Thread* GetThread()
41  : {
42 >:     Thread* pThread = gCurrentThreadInfo.m_pThread;
43  :     _ASSERTE(pThread);
44  :     return pThread;
PreStubWorker at /runtime/src/coreclr/vm/prestub.cpp:2424
2422  :         GCX_PREEMP_THREAD_EXISTS(CURRENT_THREAD);
2423  :         {
2424 >:             pbRetVal = pMD->DoPrestub(pDispatchingMT, CallerGCMode::Coop);
2425  :         }
2426  : 
?? at /runtime/src/coreclr/vm/riscv64/asmhelpers.S:379
377  :     addi  a0, sp, __PWTB_TransitionBlock        // pTransitionBlock
378  :     call  PreStubWorker
379 >:     addi  t4, a0, 0
380  : 
381  :     EPILOG_WITH_TRANSITION_BLOCK_TAILCALL

The offending class load happens couple calls down the stack in CoreLibBinder::GetClass(BinderClassID), called when loading CLASS__VECTORT for comparison.

EEClassNativeLayoutInfo::CollectNativeLayoutFieldMetadataThrowing(MethodTable*) at /runtime/src/coreclr/vm/classlayoutinfo.cpp:1029
1027  :         // from the managed size and alignment.
1028  :         // Crossgen scenarios block Vector<T> from even being loaded, so only do this check when not in crossgen.
1029 >:         if (pMT->HasSameTypeDefAs(CoreLibBinder::GetClass(CLASS__VECTORT)))
1030  :         {
1031  :             pNativeLayoutInfo->m_size = pEEClassLayoutInfo->GetManagedSize();

According to comment, I shouldn't even get here in crossgen (unless the comment doesn't refer also to crossgen2). Not sure where the flow should branch for crossgen, though. EDIT: the comment refers to crossgen1 and is a leftover from #ifdef CROSSGEN_COMPILE removed in #57697. So GetClass(CLASS_VECTOR*T) should work with CG2. I see it used to compare names but was changed, probably because comparing RIDs is faster. I'll try to find a way to do that check without a full load.

@jkotas
Copy link
Member

jkotas commented Feb 10, 2024 •
edited
Loading

In order to be usable for GC stackwalks, the ArgIterator is expected to be non-throwing and no GC triggering. The problem in the above stacktrace starts at InitializeNativeLayoutFieldMetadataThrowing. This method should not be called during GC stackwalk.

A possible fix may be to call EnsureNativeLayoutInfoInitialized at some point during struct type loading on RiscV, so that loaded struct types have this info intialized.

According to comment, I shouldn't even get here in crossgen (unless the comment doesn't refer also to crossgen2).

This is stale comment. You can delete it while you are on it.

@tomeksowi
Revert "[RISC-V] Skip ComputeReturnFlags in ArgIteratorTemplate::HasR…
ff49a20 
…etBuffArg"

This reverts commit c4f7e13.
@tomeksowi
[RISC-V] Call EnsureNativeLayoutInfoInitialized() for small structs d…
c1dd658 
…uring type load to avoid undue class loads from calling GetNativeLayoutInfo() in MethodTable::GetRiscV64PassStructInRegisterFlags during GC stackwalks
@tomeksowi
Remove stale comment
5e1d745
@tomeksowi
Copy link
Contributor Author

tomeksowi commented Feb 12, 2024 •
edited
Loading

A possible fix may be to call EnsureNativeLayoutInfoInitialized at some point during struct type loading on RiscV

Thanks a ton for your help, done.

@tomeksowi tomeksowi changed the title [RISC-V] Skip ComputeReturnFlags in ArgIteratorTemplate::HasRetBuffArg [RISC-V] Initialize native layout info for small structs during type load Feb 12, 2024
jkotas
jkotas reviewed Feb 12, 2024
View reviewed changes
src/coreclr/vm/class.cpp Outdated
@@ -1222,6 +1222,17 @@ void ClassLoader::LoadExactParents(MethodTable* pMT)
// We can now mark this type as having exact parents
pMT->SetHasExactParent();

#ifdef TARGET_RISCV64
if (pMT->HasLayout() &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to do this before SetHasExactParent call.

This is lock-free algorithm and SetHasExactParent acts as a barrier. If you do this after SetHasExactParent, it is possible for the other threads to run a lot further and hit the problem in theory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to do this before SetHasExactParent call.

I thought about initializing native layout info earlier but that failed on "Recursion in CLRException::GetThrowable" so I committed after SetHasExactParent.

But if you say it's worth it, it's caused by a infinite loop in EnsureNativeLayoutInfoInitialized() loading System.ReadOnlySpan`1[__Canon] and CoreLibBinder::GetClass(CLASS__INT128) alternately. Not sure why after INT128 it comes back to load ReadOnlySpan, I'll try to find a circuit breaker.

brucehoult
brucehoult reviewed Feb 29, 2024
View reviewed changes
src/coreclr/vm/class.cpp Outdated
{
// To calculate how a small struct is enregistered RISC-V needs native layout info.
// Initialize now so ArgIterator doesn't trigger undue class loads during GC stack walks.
pMT->EnsureNativeLayoutInfoInitialized();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is it that makes this necessary on RISC-V but not on anything else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RISC-V has more complicated calling convention than anything else. More information needs to be computed for RISC-V to interpret the calling convention. This information needs to be computed upfront - we are not able to compute it at the point where the calling convention is interpreted.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm ... I thought I was reasonably familiar with the ABI, and that it is pretty similar to A64. The only thing I'd regard as a little funky is structs with one int and one fp value being passed in the next int and the next fp register, if available. Otherwise ... big things are passed by reference, return values are passed the same as the first argument, big return values pass a pointer to a buf as the first int argument, bumping the rest along. All pretty standard stuff.

Is there some particular corner case that makes it more complex?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 500 lines compute the registers to use for a struct on RISC-V:

runtime/src/coreclr/vm/methodtable.cpp

Lines 3525 to 4027 in 0a5418d

int MethodTable::GetRiscV64PassStructInRegisterFlags(CORINFO_CLASS_HANDLE cls)
{
TypeHandle th(cls);
bool useNativeLayout = false;
int size = STRUCT_NO_FLOAT_FIELD;
MethodTable* pMethodTable = nullptr;
if (!th.IsTypeDesc())
{
pMethodTable = th.AsMethodTable();
if (pMethodTable->HasLayout())
{
useNativeLayout = true;
}
else if (th.GetSize() <= 16 /*MAX_PASS_MULTIREG_BYTES*/)
{
DWORD numIntroducedFields = pMethodTable->GetNumIntroducedInstanceFields();
if (numIntroducedFields == 1)
{
FieldDesc *pFieldStart = pMethodTable->GetApproxFieldDescListRaw();
CorElementType fieldType = pFieldStart[0].GetFieldType();
if (CorTypeInfo::IsPrimitiveType_NoThrow(fieldType))
{
if (fieldType == ELEMENT_TYPE_R4)
{
size = STRUCT_FLOAT_FIELD_ONLY_ONE;
}
else if (fieldType == ELEMENT_TYPE_R8)
{
size = STRUCT_FLOAT_FIELD_ONLY_ONE | STRUCT_FIRST_FIELD_SIZE_IS8;
}
}
else if (fieldType == ELEMENT_TYPE_VALUETYPE)
{
pMethodTable = pFieldStart->GetApproxFieldTypeHandleThrowing().GetMethodTable();
size = GetRiscV64PassStructInRegisterFlags((CORINFO_CLASS_HANDLE)pMethodTable);
}
}
else if (numIntroducedFields == 2)
{
FieldDesc *pFieldSecond;
FieldDesc *pFieldFirst = pMethodTable->GetApproxFieldDescListRaw();
if (pFieldFirst->GetOffset() == 0)
{
pFieldSecond = pFieldFirst + 1;
}
else
{
pFieldSecond = pFieldFirst;
pFieldFirst = pFieldFirst + 1;
}
assert(pFieldFirst->GetOffset() == 0);
if (pFieldFirst->GetSize() > 8)
{
goto _End_arg;
}
CorElementType fieldType = pFieldFirst[0].GetFieldType();
if (CorTypeInfo::IsPrimitiveType_NoThrow(fieldType))
{
if (fieldType == ELEMENT_TYPE_R4)
{
size = STRUCT_FLOAT_FIELD_FIRST;
}
else if (fieldType == ELEMENT_TYPE_R8)
{
size = STRUCT_FIRST_FIELD_DOUBLE;
}
else if (pFieldFirst[0].GetSize() == 8)
{
size = STRUCT_FIRST_FIELD_SIZE_IS8;
}
}
else if (fieldType == ELEMENT_TYPE_VALUETYPE)
{
pMethodTable = pFieldFirst->GetApproxFieldTypeHandleThrowing().GetMethodTable();
if (IsRiscV64OnlyOneField(pMethodTable))
{
size = GetRiscV64PassStructInRegisterFlags((CORINFO_CLASS_HANDLE)pMethodTable);
if ((size & STRUCT_FLOAT_FIELD_ONLY_ONE) != 0)
{
size = pFieldFirst[0].GetSize() == 8 ? STRUCT_FIRST_FIELD_DOUBLE : STRUCT_FLOAT_FIELD_FIRST;
}
else if (size == STRUCT_NO_FLOAT_FIELD)
{
size = pFieldFirst[0].GetSize() == 8 ? STRUCT_FIRST_FIELD_SIZE_IS8: 0;
}
else
{
size = STRUCT_NO_FLOAT_FIELD;
goto _End_arg;
}
}
else
{
size = STRUCT_NO_FLOAT_FIELD;
goto _End_arg;
}
}
else if (pFieldFirst[0].GetSize() == 8)
{
size = STRUCT_FIRST_FIELD_SIZE_IS8;
}
fieldType = pFieldSecond[0].GetFieldType();
if (pFieldSecond[0].GetSize() > 8)
{
size = STRUCT_NO_FLOAT_FIELD;
goto _End_arg;
}
else if (CorTypeInfo::IsPrimitiveType_NoThrow(fieldType))
{
if (fieldType == ELEMENT_TYPE_R4)
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND) : (size | STRUCT_FLOAT_FIELD_SECOND);
}
else if (fieldType == ELEMENT_TYPE_R8)
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND_8) : (size | STRUCT_SECOND_FIELD_DOUBLE);
}
else if ((size & STRUCT_FLOAT_FIELD_FIRST) == 0)
{
size = STRUCT_NO_FLOAT_FIELD;
}
else if (pFieldSecond[0].GetSize() == 8)
{
size |= STRUCT_SECOND_FIELD_SIZE_IS8;
}
}
else if (fieldType == ELEMENT_TYPE_VALUETYPE)
{
pMethodTable = pFieldSecond[0].GetApproxFieldTypeHandleThrowing().GetMethodTable();
if (IsRiscV64OnlyOneField(pMethodTable))
{
int size2 = GetRiscV64PassStructInRegisterFlags((CORINFO_CLASS_HANDLE)pMethodTable);
if ((size2 & STRUCT_FLOAT_FIELD_ONLY_ONE) != 0)
{
if (pFieldSecond[0].GetSize() == 8)
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND_8) : (size | STRUCT_SECOND_FIELD_DOUBLE);
}
else
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND) : (size | STRUCT_FLOAT_FIELD_SECOND);
}
}
else if ((size & STRUCT_FLOAT_FIELD_FIRST) == 0)
{
size = STRUCT_NO_FLOAT_FIELD;
}
else if (size2 == STRUCT_NO_FLOAT_FIELD)
{
size |= pFieldSecond[0].GetSize() == 8 ? STRUCT_SECOND_FIELD_SIZE_IS8 : 0;
}
else
{
size = STRUCT_NO_FLOAT_FIELD;
}
}
else
{
size = STRUCT_NO_FLOAT_FIELD;
}
}
else if ((size & STRUCT_FLOAT_FIELD_FIRST) == 0)
{
size = STRUCT_NO_FLOAT_FIELD;
}
else if (pFieldSecond[0].GetSize() == 8)
{
size |= STRUCT_SECOND_FIELD_SIZE_IS8;
}
}
goto _End_arg;
}
}
else
{
_ASSERTE(th.IsNativeValueType());
useNativeLayout = true;
pMethodTable = th.AsNativeValueType();
}
_ASSERTE(pMethodTable != nullptr);
if (useNativeLayout)
{
if (th.GetSize() <= 16 /*MAX_PASS_MULTIREG_BYTES*/)
{
DWORD numIntroducedFields = pMethodTable->GetNativeLayoutInfo()->GetNumFields();
FieldDesc *pFieldStart = nullptr;
if (numIntroducedFields == 1)
{
pFieldStart = pMethodTable->GetApproxFieldDescListRaw();
CorElementType fieldType = pFieldStart->GetFieldType();
// InlineArray types and fixed buffer types have implied repeated fields.
// Checking if a type is an InlineArray type is cheap, so we'll do that first.
bool hasImpliedRepeatedFields = HasImpliedRepeatedFields(pMethodTable);
if (hasImpliedRepeatedFields)
{
numIntroducedFields = pMethodTable->GetNumInstanceFieldBytes() / pFieldStart->GetSize();
if (numIntroducedFields > 2)
{
goto _End_arg;
}
if (fieldType == ELEMENT_TYPE_R4)
{
if (numIntroducedFields == 1)
{
size = STRUCT_FLOAT_FIELD_ONLY_ONE;
}
else if (numIntroducedFields == 2)
{
size = STRUCT_FLOAT_FIELD_ONLY_TWO;
}
goto _End_arg;
}
else if (fieldType == ELEMENT_TYPE_R8)
{
if (numIntroducedFields == 1)
{
size = STRUCT_FLOAT_FIELD_ONLY_ONE | STRUCT_FIRST_FIELD_SIZE_IS8;
}
else if (numIntroducedFields == 2)
{
size = STRUCT_FIELD_TWO_DOUBLES;
}
goto _End_arg;
}
}
if (CorTypeInfo::IsPrimitiveType_NoThrow(fieldType))
{
if (fieldType == ELEMENT_TYPE_R4)
{
size = STRUCT_FLOAT_FIELD_ONLY_ONE;
}
else if (fieldType == ELEMENT_TYPE_R8)
{
size = STRUCT_FLOAT_FIELD_ONLY_ONE | STRUCT_FIRST_FIELD_SIZE_IS8;
}
}
else if (fieldType == ELEMENT_TYPE_VALUETYPE)
{
const NativeFieldDescriptor *pNativeFieldDescs = pMethodTable->GetNativeLayoutInfo()->GetNativeFieldDescriptors();
NativeFieldCategory nfc = pNativeFieldDescs->GetCategory();
if (nfc == NativeFieldCategory::NESTED)
{
pMethodTable = pNativeFieldDescs->GetNestedNativeMethodTable();
size = GetRiscV64PassStructInRegisterFlags((CORINFO_CLASS_HANDLE)pMethodTable);
return size;
}
else if (nfc == NativeFieldCategory::FLOAT)
{
if (pFieldStart->GetSize() == 4)
{
size = STRUCT_FLOAT_FIELD_ONLY_ONE;
}
else if (pFieldStart->GetSize() == 8)
{
size = STRUCT_FLOAT_FIELD_ONLY_ONE | STRUCT_FIRST_FIELD_SIZE_IS8;
}
}
}
}
else if (numIntroducedFields == 2)
{
pFieldStart = pMethodTable->GetApproxFieldDescListRaw();
if (pFieldStart->GetSize() > 8)
{
goto _End_arg;
}
if (pFieldStart->GetOffset() || !pFieldStart[1].GetOffset() || (pFieldStart[0].GetSize() > pFieldStart[1].GetOffset()))
{
goto _End_arg;
}
CorElementType fieldType = pFieldStart[0].GetFieldType();
if (CorTypeInfo::IsPrimitiveType_NoThrow(fieldType))
{
if (fieldType == ELEMENT_TYPE_R4)
{
size = STRUCT_FLOAT_FIELD_FIRST;
}
else if (fieldType == ELEMENT_TYPE_R8)
{
size = STRUCT_FIRST_FIELD_DOUBLE;
}
else if (pFieldStart[0].GetSize() == 8)
{
size = STRUCT_FIRST_FIELD_SIZE_IS8;
}
fieldType = pFieldStart[1].GetFieldType();
if (CorTypeInfo::IsPrimitiveType_NoThrow(fieldType))
{
if (fieldType == ELEMENT_TYPE_R4)
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND) : (size | STRUCT_FLOAT_FIELD_SECOND);
}
else if (fieldType == ELEMENT_TYPE_R8)
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND_8) : (size | STRUCT_SECOND_FIELD_DOUBLE);
}
else if ((size & STRUCT_FLOAT_FIELD_FIRST) == 0)
{
size = STRUCT_NO_FLOAT_FIELD;
}
else if (pFieldStart[1].GetSize() == 8)
{
size |= STRUCT_SECOND_FIELD_SIZE_IS8;
}
goto _End_arg;
}
}
else if (fieldType == ELEMENT_TYPE_VALUETYPE)
{
const NativeFieldDescriptor *pNativeFieldDescs = pMethodTable->GetNativeLayoutInfo()->GetNativeFieldDescriptors();
NativeFieldCategory nfc = pNativeFieldDescs->GetCategory();
if (nfc == NativeFieldCategory::NESTED)
{
if (pNativeFieldDescs->GetNumElements() != 1)
{
size = STRUCT_NO_FLOAT_FIELD;
goto _End_arg;
}
MethodTable* pMethodTable2 = pNativeFieldDescs->GetNestedNativeMethodTable();
if (!IsRiscV64OnlyOneField(pMethodTable2))
{
size = STRUCT_NO_FLOAT_FIELD;
goto _End_arg;
}
size = GetRiscV64PassStructInRegisterFlags((CORINFO_CLASS_HANDLE)pMethodTable2);
if ((size & STRUCT_FLOAT_FIELD_ONLY_ONE) != 0)
{
if (pFieldStart->GetSize() == 8)
{
size = STRUCT_FIRST_FIELD_DOUBLE;
}
else
{
size = STRUCT_FLOAT_FIELD_FIRST;
}
}
else if (pFieldStart->GetSize() == 8)
{
size = STRUCT_FIRST_FIELD_SIZE_IS8;
}
else
{
size = STRUCT_NO_FLOAT_FIELD;
goto _End_arg;
}
}
else if (nfc == NativeFieldCategory::FLOAT)
{
if (pFieldStart[0].GetSize() == 4)
{
size = STRUCT_FLOAT_FIELD_FIRST;
}
else if (pFieldStart[0].GetSize() == 8)
{
_ASSERTE((pMethodTable->GetNativeSize() == 8) || (pMethodTable->GetNativeSize() == 16));
size = STRUCT_FIRST_FIELD_DOUBLE;
}
}
else if (pFieldStart[0].GetSize() == 8)
{
size = STRUCT_FIRST_FIELD_SIZE_IS8;
}
}
else if (pFieldStart[0].GetSize() == 8)
{
size = STRUCT_FIRST_FIELD_SIZE_IS8;
}
fieldType = pFieldStart[1].GetFieldType();
if (pFieldStart[1].GetSize() > 8)
{
size = STRUCT_NO_FLOAT_FIELD;
goto _End_arg;
}
else if (CorTypeInfo::IsPrimitiveType_NoThrow(fieldType))
{
if (fieldType == ELEMENT_TYPE_R4)
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND) : (size | STRUCT_FLOAT_FIELD_SECOND);
}
else if (fieldType == ELEMENT_TYPE_R8)
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND_8) : (size | STRUCT_SECOND_FIELD_DOUBLE);
}
else if ((size & STRUCT_FLOAT_FIELD_FIRST) == 0)
{
size = STRUCT_NO_FLOAT_FIELD;
}
else if (pFieldStart[1].GetSize() == 8)
{
size |= STRUCT_SECOND_FIELD_SIZE_IS8;
}
// Pass with two integer registers in `struct {int a, int b, float/double c}` cases
if ((size | STRUCT_FIRST_FIELD_SIZE_IS8 | STRUCT_FLOAT_FIELD_SECOND) == size)
{
size = STRUCT_NO_FLOAT_FIELD;
}
}
else if (fieldType == ELEMENT_TYPE_VALUETYPE)
{
const NativeFieldDescriptor *pNativeFieldDescs = pMethodTable->GetNativeLayoutInfo()->GetNativeFieldDescriptors();
NativeFieldCategory nfc = pNativeFieldDescs[1].GetCategory();
if (nfc == NativeFieldCategory::NESTED)
{
if (pNativeFieldDescs[1].GetNumElements() != 1)
{
size = STRUCT_NO_FLOAT_FIELD;
goto _End_arg;
}
MethodTable* pMethodTable2 = pNativeFieldDescs[1].GetNestedNativeMethodTable();
if (!IsRiscV64OnlyOneField(pMethodTable2))
{
size = STRUCT_NO_FLOAT_FIELD;
goto _End_arg;
}
if ((GetRiscV64PassStructInRegisterFlags((CORINFO_CLASS_HANDLE)pMethodTable2) & STRUCT_FLOAT_FIELD_ONLY_ONE) != 0)
{
if (pFieldStart[1].GetSize() == 4)
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND) : (size | STRUCT_FLOAT_FIELD_SECOND);
}
else if (pFieldStart[1].GetSize() == 8)
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND_8) : (size | STRUCT_SECOND_FIELD_DOUBLE);
}
}
else if ((size & STRUCT_FLOAT_FIELD_FIRST) == 0)
{
size = STRUCT_NO_FLOAT_FIELD;
}
else if (pFieldStart[1].GetSize() == 8)
{
size |= STRUCT_SECOND_FIELD_SIZE_IS8;
}
}
else if (nfc == NativeFieldCategory::FLOAT)
{
if (pFieldStart[1].GetSize() == 4)
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND) : (size | STRUCT_FLOAT_FIELD_SECOND);
}
else if (pFieldStart[1].GetSize() == 8)
{
size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND_8) : (size | STRUCT_SECOND_FIELD_DOUBLE);
}
}
else if ((size & STRUCT_FLOAT_FIELD_FIRST) == 0)
{
size = STRUCT_NO_FLOAT_FIELD;
}
else if (pFieldStart[1].GetSize() == 8)
{
size |= STRUCT_SECOND_FIELD_SIZE_IS8;
}
}
else if ((size & STRUCT_FLOAT_FIELD_FIRST) == 0)
{
size = STRUCT_NO_FLOAT_FIELD;
}
else if (pFieldStart[1].GetSize() == 8)
{
size |= STRUCT_SECOND_FIELD_SIZE_IS8;
}
}
}
}
_End_arg:
return size;
}
#endif
.

Copy link
Contributor Author

@tomeksowi tomeksowi Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, GetNativeLayoutInfo() in particular is causing problems as it can trigger a full class load (in CoreLibBinder::GetClass) here:

runtime/src/coreclr/vm/classlayoutinfo.cpp

Lines 1027 to 1041 in 0a5418d

if (pMT->HasSameTypeDefAs(CoreLibBinder::GetClass(CLASS__VECTORT)))
{
pNativeLayoutInfo->m_size = pEEClassLayoutInfo->GetManagedSize();
pNativeLayoutInfo->m_alignmentRequirement = pEEClassLayoutInfo->m_ManagedLargestAlignmentRequirementOfAllMembers;
}
else
if (pMT->HasSameTypeDefAs(CoreLibBinder::GetClass(CLASS__INT128)) ||
pMT->HasSameTypeDefAs(CoreLibBinder::GetClass(CLASS__UINT128)) ||
pMT->HasSameTypeDefAs(CoreLibBinder::GetClass(CLASS__VECTOR64T)) ||
pMT->HasSameTypeDefAs(CoreLibBinder::GetClass(CLASS__VECTOR128T)) ||
pMT->HasSameTypeDefAs(CoreLibBinder::GetClass(CLASS__VECTOR256T)) ||
pMT->HasSameTypeDefAs(CoreLibBinder::GetClass(CLASS__VECTOR512T)))
{
pNativeLayoutInfo->m_alignmentRequirement = pEEClassLayoutInfo->m_ManagedLargestAlignmentRequirementOfAllMembers;
}

I've been looking at calling EnsureEnsureNativeLayoutInfoInitialized() before SetHasExactParent() as suggested but then CoreLibBinder::GetClass(CLASS__INT128) triggers EnsureEnsureNativeLayoutInfoInitialized() for it which then tries to load Int128 (at this point not fully loaded yet), which fails on a deadlock check:

runtime/src/coreclr/vm/clsload.cpp

Lines 3181 to 3185 in 0a5418d

if (PendingTypeLoadHolder::CheckForDeadLockOnCurrentThread(pLoadingEntry))
{
// Attempting recursive load
ClassLoader::ThrowTypeLoadException(pTypeKey, IDS_CLASSLOAD_GENERAL);
}

I'll come back to it to figure out how to side-step this issue. It's a bit unfortunate EEClassNativeLayoutInfo::CollectNativeLayoutFieldMetadataThrowing() is doing a full class load, MethodTable::HasSameTypeDefAs() needs just the RID which I believe we get much earlier in the load sequence and the module which is a known global in CoreLibBinder.

BTW is the formula for RID fixed, i.e. given the namespace and the name for the few types enumerated, could it be calculated at compile time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RID formula is not fixed. The RID changes from build to build.

If all you need is the RID, you can load the Vector type explicitly upfront here:

runtime/src/coreclr/vm/appdomain.cpp

Line 1380 in 18b0816

g_pCastHelpers = CoreLibBinder::GetClass(CLASS__CASTHELPERS);

Copy link
Contributor Author

@tomeksowi tomeksowi Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all you need is the RID, you can load the Vector type explicitly upfront

Preloading classes for comparison gets rid of the direct problem in EEClassNativeLayoutInfo::CollectNativeLayoutFieldMetadataThrowing, thank you. I pushed WiP so far, however, I'm still hitting recursive class loads while constructing MarshalInfo here, which does a full load:

runtime/src/coreclr/vm/mlinfo.cpp

Lines 1735 to 1740 in db8f9eb

case ELEMENT_TYPE_OBJECT:
case ELEMENT_TYPE_STRING:
case ELEMENT_TYPE_CLASS:
case ELEMENT_TYPE_VAR:
{
TypeHandle sigTH = sig.GetTypeHandleThrowing(pModule, pTypeContext);

Not sure if lowering the class load level to CLASS_LOAD_APPROXPARENTS is acceptable here. The test passes but I fear the marshal info may be incorrect for generics as a result, need to confirm with CLR tests on marshalling. And there are other places in which MarshalInfo::MarshalInfo does a full class load.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the stack trace and the type that you are hitting this for?

Copy link
Contributor Author

@tomeksowi tomeksowi Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the stack trace and the type that you are hitting this for?

Stack trace for type ImportThunkKey, some line numbers are slightly off because I inserted deoptimizing attributes 
#0  ClassLoader::ThrowTypeLoadException (pKey=0x400180af28, resIDWhy=2148734242) at /runtime/src/coreclr/vm/clsload.cpp:737
#1  0x0000004001ed92aa in ClassLoader::LoadTypeHandleForTypeKey_Body (this=0x40001a97a0, pTypeKey=0x400180af28, typeHnd=..., targetLevel=CLASS_LOAD_EXACTPARENTS)
    at /runtime/src/coreclr/vm/clsload.cpp:3190
#2  0x0000004001ed4994 in ClassLoader::LoadTypeHandleForTypeKey (this=0x40001a97a0, pTypeKey=0x400180af28, typeHnd=..., targetLevel=CLASS_LOAD_EXACTPARENTS, pInstContext=0x0)
    at /runtime/src/coreclr/vm/clsload.cpp:3011
#3  0x0000004001ed60d0 in ClassLoader::LoadTypeDefThrowing (pModule=0x3f833bf130, typeDef=33555137, fNotFoundAction=ClassLoader::ThrowIfNotFound,
    fUninstantiated=ClassLoader::FailIfUninstDefOrRef, tokenNotToLoad=0, level=CLASS_LOAD_EXACTPARENTS, pTargetInstantiation=0x0) at /runtime/src/coreclr/vm/clsload.cpp:1998
#4  0x0000004001ed6e1e in ClassLoader::LoadTypeDefOrRefThrowing (pModule=0x3f833bf130, typeDefOrRef=33555137, fNotFoundAction=ClassLoader::ThrowIfNotFound,
    fUninstantiated=ClassLoader::FailIfUninstDefOrRef, tokenNotToLoad=0, level=CLASS_LOAD_EXACTPARENTS) at /runtime/src/coreclr/vm/clsload.cpp:2167
#5  0x0000004001fa8dda in SigPointer::GetTypeHandleThrowing (this=0x400180c500, pModule=0x3f833bf130, pTypeContext=0x400180c980, fLoadTypes=ClassLoader::LoadTypes,
    level=CLASS_LOAD_EXACTPARENTS, dropGenericArgumentLevel=0, pSubst=0x0, pZapSigContext=0x0, pMTInterfaceMapOwner=0x0, pRecursiveFieldGenericHandling=0x0)
    at /runtime/src/coreclr/vm/siginfo.cpp:1641
#6  0x0000004001fa88b2 in SigPointer::GetTypeHandleThrowing (this=0x400180c9c0, pModule=0x3f833bf130, pTypeContext=0x400180c980, fLoadTypes=ClassLoader::LoadTypes,
    level=CLASS_LOAD_EXACTPARENTS, dropGenericArgumentLevel=0, pSubst=0x0, pZapSigContext=0x0, pMTInterfaceMapOwner=0x0, pRecursiveFieldGenericHandling=0x0)
    at /runtime/src/coreclr/vm/siginfo.cpp:1505
#7  0x0000004001f0ea02 in MetaSig::GetLastTypeHandleThrowing (this=0x400180c978, fLoadTypes=ClassLoader::LoadTypes, level=<optimized out>, dropGenericArgumentLevel=<optimized out>)
    at /runtime/src/coreclr/vm/siginfo.hpp:917
#8  FieldDesc::GetFieldTypeHandleThrowing (this=<optimized out>, level=29345424, dropGenericArgumentLevel=2048) at /runtime/src/coreclr/vm/field.cpp:166
#9  0x0000004001f763e8 in MethodTable::DoFullyLoad (this=0x3f8432e568, pVisited=0x400180ceb8, level=CLASS_DEPENDENCIES_LOADED, pPending=0x0, pfBailed=0x400180cedc, pInstContext=0x0)
    at /runtime/src/coreclr/vm/methodtable.cpp:5197
#10 0x0000004001f7631e in MethodTable::DoFullyLoad (this=0x3f8432edb0, pVisited=0x400180d188, level=CLASS_DEPENDENCIES_LOADED, pPending=0x0, pfBailed=0x400180d1ac, pInstContext=0x0)
    at /runtime/src/coreclr/vm/methodtable.cpp:5176
#11 0x0000004001f7625a in MethodTable::DoFullyLoad (this=0x3f843b9e90, pVisited=0x0, level=CLASS_DEPENDENCIES_LOADED, pPending=0x0, pfBailed=0x400180d2dc, pInstContext=0x0)
    at /runtime/src/coreclr/vm/methodtable.cpp:5157
#12 0x0000004001ed9cce in PushFinalLevels (typeHnd=..., targetLevel=CLASS_LOADED, pInstContext=0x0) at /runtime/src/coreclr/vm/clsload.cpp:2941
#13 0x0000004001ed49f2 in ClassLoader::LoadTypeHandleForTypeKey (this=0x40001a97a0, pTypeKey=0x400180d3b8, typeHnd=..., targetLevel=CLASS_LOADED, pInstContext=0x0)
    at /runtime/src/coreclr/vm/clsload.cpp:3018
#14 0x0000004001ed60d0 in ClassLoader::LoadTypeDefThrowing (pModule=0x3f833bf130, typeDef=33554907, fNotFoundAction=ClassLoader::ThrowIfNotFound,
    fUninstantiated=ClassLoader::FailIfUninstDefOrRef, tokenNotToLoad=0, level=CLASS_LOADED, pTargetInstantiation=0x0) at /runtime/src/coreclr/vm/clsload.cpp:1998
#15 0x0000004001ed6e1e in ClassLoader::LoadTypeDefOrRefThrowing (pModule=0x3f833bf130, typeDefOrRef=33554907, fNotFoundAction=ClassLoader::ThrowIfNotFound,
    fUninstantiated=ClassLoader::FailIfUninstDefOrRef, tokenNotToLoad=0, level=CLASS_LOADED) at /runtime/src/coreclr/vm/clsload.cpp:2167
#16 0x0000004001fa8dda in SigPointer::GetTypeHandleThrowing (this=0x400180ef50, pModule=0x3f833bf130, pTypeContext=0x400180f2e8, fLoadTypes=ClassLoader::LoadTypes,
    level=CLASS_LOADED, dropGenericArgumentLevel=0, pSubst=0x0, pZapSigContext=0x0, pMTInterfaceMapOwner=0x0, pRecursiveFieldGenericHandling=0x0)
    at /runtime/src/coreclr/vm/siginfo.cpp:1641
#17 0x00000040020c34c2 in MarshalInfo::MarshalInfo (this=0x400180ef60, pModule=0x3f833bf130, sig=..., pTypeContext=0x400180f2e8, token=67112859,
    ms=MarshalInfo::MARSHAL_SCENARIO_FIELD, nlType=nltAnsi, nlFlags=nlfNone, isParam=0, paramidx=0, numArgs=0, BestFit=0, ThrowOnUnmappableChar=0, fEmitsIL=0, pMD=0x0,
    fLoadCustomMarshal=0, pDebugName=0x6091d48ff9 "ContainingImportSection", pDebugClassName=0x6091d552be "ImportThunkKey", argidx=4294967295)
    at /runtime/src/coreclr/vm/mlinfo.cpp:1742
#18 0x0000004002061abe in ParseNativeType (pModule=0x3f833bf130, sig=..., pFD=0x3f8432d1c8, flags=ParseNativeTypeFlags::IsAnsi, pNFD=0x608c029d40, pTypeContext=0x400180f2e8,
    szNamespace=0x6091d32afc "", szClassName=0x6091d552be "ImportThunkKey", szFieldName=0x6091d48ff9 "ContainingImportSection") at /runtime/src/coreclr/vm/fieldmarshaler.cpp:56
#19 0x0000004001ed1bae in (anonymous namespace)::ParseFieldNativeTypes (pInternalImport=0x40001b1e70, cl=33555137, fieldDescs=..., pModule=0x3f833bf130,
    nativeTypeFlags=ParseNativeTypeFlags::IsAnsi, pTypeContext=0x400180f2e8, pFieldInfoArrayOut=0x608c029d28, szNamespace=0x6091d32afc "", szName=0x6091d552be "ImportThunkKey")
    at /runtime/src/coreclr/vm/classlayoutinfo.cpp:442
#20 0x0000004001ed0d88 in EEClassNativeLayoutInfo::CollectNativeLayoutFieldMetadataThrowing (pMT=0x3f8432d430) at /runtime/src/coreclr/vm/classlayoutinfo.cpp:923
#21 0x0000004001ed05bc in EEClassNativeLayoutInfo::InitializeNativeLayoutFieldMetadataThrowing (pMT=0x3f8432d430) at /runtime/src/coreclr/vm/classlayoutinfo.cpp:844
#22 0x0000004001f7f4b0 in MethodTable::EnsureNativeLayoutInfoInitialized (this=0x3f8432d430) at /runtime/src/coreclr/vm/methodtable.cpp:9280
#23 0x0000004001ec7986 in ClassLoader::LoadExactParents (pMT=0x3f8432d430) at /runtime/src/coreclr/vm/class.cpp:1231
#24 0x0000004001ed7ec0 in ClassLoader::DoIncrementalLoad (pTypeKey=0x400180fc58, typeHnd=..., currentLevel=CLASS_LOAD_APPROXPARENTS) at /runtime/src/coreclr/vm/clsload.cpp:2634
#25 0x0000004001ed96a6 in ClassLoader::LoadTypeHandleForTypeKey_Body (this=0x40001a97a0, pTypeKey=0x400180fc58, typeHnd=..., targetLevel=CLASS_LOAD_EXACTPARENTS)
    at /runtime/src/coreclr/vm/clsload.cpp:3283
#26 0x0000004001ed4994 in ClassLoader::LoadTypeHandleForTypeKey (this=0x40001a97a0, pTypeKey=0x400180fc58, typeHnd=..., targetLevel=CLASS_LOAD_EXACTPARENTS, pInstContext=0x0)
    at /runtime/src/coreclr/vm/clsload.cpp:3011
#27 0x0000004001ed60d0 in ClassLoader::LoadTypeDefThrowing (pModule=0x3f833bf130, typeDef=33555137, fNotFoundAction=ClassLoader::ThrowIfNotFound,
    fUninstantiated=ClassLoader::FailIfUninstDefOrRef, tokenNotToLoad=0, level=CLASS_LOAD_EXACTPARENTS, pTargetInstantiation=0x0) at /runtime/src/coreclr/vm/clsload.cpp:1998
#28 0x0000004001ed6e1e in ClassLoader::LoadTypeDefOrRefThrowing (pModule=0x3f833bf130, typeDefOrRef=33555137, fNotFoundAction=ClassLoader::ThrowIfNotFound,
    fUninstantiated=ClassLoader::FailIfUninstDefOrRef, tokenNotToLoad=0, level=CLASS_LOAD_EXACTPARENTS) at /runtime/src/coreclr/vm/clsload.cpp:2167
#29 0x0000004001fa8dda in SigPointer::GetTypeHandleThrowing (this=0x4001811230, pModule=0x3f833bf130, pTypeContext=0x40018116b0, fLoadTypes=ClassLoader::LoadTypes,
    level=CLASS_LOAD_EXACTPARENTS, dropGenericArgumentLevel=0, pSubst=0x0, pZapSigContext=0x0, pMTInterfaceMapOwner=0x0, pRecursiveFieldGenericHandling=0x0)
    at /runtime/src/coreclr/vm/siginfo.cpp:1641
#30 0x0000004001fa88b2 in SigPointer::GetTypeHandleThrowing (this=0x40018116f0, pModule=0x3f833bf130, pTypeContext=0x40018116b0, fLoadTypes=ClassLoader::LoadTypes,
    level=CLASS_LOAD_EXACTPARENTS, dropGenericArgumentLevel=0, pSubst=0x0, pZapSigContext=0x0, pMTInterfaceMapOwner=0x0, pRecursiveFieldGenericHandling=0x0)
    at /runtime/src/coreclr/vm/siginfo.cpp:1505
#31 0x0000004001f0ea02 in MetaSig::GetLastTypeHandleThrowing (this=0x40018116a8, fLoadTypes=ClassLoader::LoadTypes, level=<optimized out>, dropGenericArgumentLevel=<optimized out>)
    at /runtime/src/coreclr/vm/siginfo.hpp:917
#32 FieldDesc::GetFieldTypeHandleThrowing (this=<optimized out>, level=29345424, dropGenericArgumentLevel=2048) at /runtime/src/coreclr/vm/field.cpp:166
#33 0x0000004001f763e8 in MethodTable::DoFullyLoad (this=0x3f8432e568, pVisited=0x4001811be8, level=CLASS_DEPENDENCIES_LOADED, pPending=0x0, pfBailed=0x4001811c0c, pInstContext=0x0)
    at /runtime/src/coreclr/vm/methodtable.cpp:5197
#34 0x0000004001f7631e in MethodTable::DoFullyLoad (this=0x3f8432edb0, pVisited=0x4001811eb8, level=CLASS_DEPENDENCIES_LOADED, pPending=0x0, pfBailed=0x4001811edc, pInstContext=0x0)
--Type <RET> for more, q to quit, c to continue without paging--c
    at /runtime/src/coreclr/vm/methodtable.cpp:5176
#35 0x0000004001f7625a in MethodTable::DoFullyLoad (this=0x3f8432e968, pVisited=0x4001812188, level=CLASS_DEPENDENCIES_LOADED, pPending=0x0, pfBailed=0x40018121ac, pInstContext=0x0) at /runtime/src/coreclr/vm/methodtable.cpp:5157
#36 0x0000004001f761d0 in MethodTable::DoFullyLoad (this=0x3f84327c00, pVisited=0x4001812458, level=CLASS_DEPENDENCIES_LOADED, pPending=0x0, pfBailed=0x400181247c, pInstContext=0x0) at /runtime/src/coreclr/vm/methodtable.cpp:5138
#37 0x0000004001f761d0 in MethodTable::DoFullyLoad (this=0x3f843280d0, pVisited=0x4001812728, level=CLASS_DEPENDENCIES_LOADED, pPending=0x0, pfBailed=0x400181274c, pInstContext=0x0) at /runtime/src/coreclr/vm/methodtable.cpp:5138
#38 0x0000004001f761d0 in MethodTable::DoFullyLoad (this=0x3f84328ac8, pVisited=0x0, level=CLASS_DEPENDENCIES_LOADED, pPending=0x0, pfBailed=0x400181287c, pInstContext=0x0) at /runtime/src/coreclr/vm/methodtable.cpp:5138
#39 0x0000004001ed9cce in PushFinalLevels (typeHnd=..., targetLevel=CLASS_LOADED, pInstContext=0x0) at /runtime/src/coreclr/vm/clsload.cpp:2941
#40 0x0000004001ed49f2 in ClassLoader::LoadTypeHandleForTypeKey (this=0x40001a97a0, pTypeKey=0x4001812958, typeHnd=..., targetLevel=CLASS_LOADED, pInstContext=0x0) at /runtime/src/coreclr/vm/clsload.cpp:3018
#41 0x0000004001ed60d0 in ClassLoader::LoadTypeDefThrowing (pModule=0x3f833bf130, typeDef=33554886, fNotFoundAction=ClassLoader::ThrowIfNotFound, fUninstantiated=ClassLoader::FailIfUninstDefOrRef, tokenNotToLoad=0, level=CLASS_LOADED, pTargetInstantiation=0x0) at /runtime/src/coreclr/vm/clsload.cpp:1998
#42 0x0000004001ed6e1e in ClassLoader::LoadTypeDefOrRefThrowing (pModule=0x3f833bf130, typeDefOrRef=33554886, fNotFoundAction=ClassLoader::ThrowIfNotFound, fUninstantiated=ClassLoader::FailIfUninstDefOrRef, tokenNotToLoad=0, level=CLASS_LOADED) at /runtime/src/coreclr/vm/clsload.cpp:2167
#43 0x0000004001fa8dda in SigPointer::GetTypeHandleThrowing (this=0x40018137a0, pModule=0x3f833bf130, pTypeContext=0x4001813778, fLoadTypes=ClassLoader::LoadTypes, level=CLASS_LOADED, dropGenericArgumentLevel=0, pSubst=0x0, pZapSigContext=0x0, pMTInterfaceMapOwner=0x0, pRecursiveFieldGenericHandling=0x0) at /runtime/src/coreclr/vm/siginfo.cpp:1641
#44 0x0000004001f48a52 in CEEInfo::getArgClass (this=<optimized out>, sig=0x4001814420, args=<optimized out>) at /runtime/src/coreclr/vm/jitinterface.cpp:9693
#45 0x00000060890d1354 in Compiler::lvaInitTypeRef (this=0x400044cf48) at /runtime/src/coreclr/jit/lclvars.cpp:301
#46 0x0000006088fe425a in Compiler::compCompileHelper (this=0x400044cf48, classPtr=0x3f833bf130, compHnd=0x4001814528, methodInfo=0x4001814380, methodCodePtr=0x40018141a8, methodCodeSize=0x400181435c, compileFlags=0x40018141c8) at /runtime/src/coreclr/jit/compiler.cpp:6945
#47 0x0000006088fe310a in Compiler::compCompile(CORINFO_MODULE_STRUCT_*, void**, unsigned int*, JitFlags*)::$_0::operator()(Compiler::compCompile(CORINFO_MODULE_STRUCT_*, void**, unsigned int*, JitFlags*)::__JITParam*) const (this=<optimized out>, __JITpParam=<optimized out>) at /runtime/src/coreclr/jit/compiler.cpp:6355
#48 Compiler::compCompile (this=0x400044cf48, classPtr=0x3f833bf130, methodCodePtr=0x40018141a8, methodCodeSize=0x400181435c, compileFlags=0x40018141c8) at /runtime/src/coreclr/jit/compiler.cpp:6374
#49 0x0000006088fe5954 in jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_0::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const::{lambda(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_0::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const::__JITParam*)#1}::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_0::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const::__JITParam*) const (this=<optimized out>, __JITpParam=<optimized out>) at /runtime/src/coreclr/jit/compiler.cpp:7864
#50 jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_0::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const (__JITpParam=<optimized out>, this=<optimized out>) at /runtime/src/coreclr/jit/compiler.cpp:7889
#51 jitNativeCode (methodHnd=0x3f840de6b8, classPtr=0x3f833bf130, compHnd=0x4001814528, methodInfo=0x4001814380, methodCodePtr=0x40018141a8, methodCodeSize=0x400181435c, compileFlags=0x40018141c8, inlineInfoPtr=0x0) at /runtime/src/coreclr/jit/compiler.cpp:7891
#52 0x0000006088fee534 in CILJit::compileMethod (this=<optimized out>, compHnd=0x4001814528, methodInfo=0x4001814380, flags=<optimized out>, entryAddress=0x4001814ed0, nativeSizeOfCode=0x400181435c) at /runtime/src/coreclr/jit/ee_il_dll.cpp:289
#53 0x0000004001f4f726 in invokeCompileMethodHelper (jitMgr=0x40000b1220, comp=0x4001814528, info=0x4001814380, jitFlags=..., nativeEntry=0x4001814ed0, nativeSizeOfCode=0x400181435c) at /runtime/src/coreclr/vm/jitinterface.cpp:12520
#54 0x0000004001f4f894 in invokeCompileMethod (jitMgr=0x400180af28, comp=0xffffffff80131522, info=0xa6af15a049a54a00, jitFlags=..., nativeEntry=0x4001bfc690, nativeSizeOfCode=0x800) at /runtime/src/coreclr/vm/jitinterface.cpp:12583
#55 0x0000004001f5070e in UnsafeJitFunction (config=<optimized out>, ILHeader=<optimized out>, pJitFlags=0x4001814fe0, pSizeOfCode=0x40018150bc) at /runtime/src/coreclr/vm/jitinterface.cpp:13028
#56 0x0000004001f97b4c in MethodDesc::JitCompileCodeLocked (this=0x3f840de6b8, pConfig=0x40018153f0, pilHeader=0x4001815080, pEntry=0x608c0260e0, pSizeOfCode=0x40018150bc) at /runtime/src/coreclr/vm/prestub.cpp:939
#57 0x0000004001f9742a in MethodDesc::JitCompileCodeLockedEventWrapper (this=0x3f840de6b8, pConfig=0x40018153f0, pEntry=0x608c0260e0) at /runtime/src/coreclr/vm/prestub.cpp:820
#58 0x0000004001f96ae4 in MethodDesc::JitCompileCode (this=0x3f840de6b8, pConfig=0x40018153f0) at /runtime/src/coreclr/vm/prestub.cpp:707
#59 0x0000004001f96226 in MethodDesc::PrepareILBasedCode (this=0x3f840de6b8, pConfig=0x40018153f0) at /runtime/src/coreclr/vm/prestub.cpp:433
#60 0x0000004001ee02c2 in CodeVersionManager::PublishVersionableCodeIfNecessary (this=0x40000ad2d0, pMethodDesc=0x3f840de6b8, callerGCMode=CallerGCMode::Coop, doBackpatchRef=0x40018154e8, doFullBackpatchRef=<optimized out>) at /runtime/src/coreclr/vm/codeversion.cpp:1734
#61 0x0000004001f9b830 in MethodDesc::DoPrestub (this=0x3f840de6b8, pDispatchingMT=0x3f840de738, callerGCMode=CallerGCMode::Coop) at /runtime/src/coreclr/vm/prestub.cpp:2595
#62 0x0000004001f9afee in PreStubWorker (pTransitionBlock=<optimized out>, pMD=0x3f840de6b8) at /runtime/src/coreclr/vm/prestub.cpp:2424
#63 0x000000400221a49a in ThePreStub () at /runtime/src/coreclr/vm/riscv64/asmhelpers.S:378
#64 0x0000003f83f08c68 in ?? ()

@clamp03 clamp03 mentioned this pull request Mar 8, 2024
Merged
@tomeksowi
WiP: move EnsureNativeLayoutInfoInitialized() before SetHasExactParent()
0292f48 
SetHasExactParent() is supposed to act like a barrier to ensure the native layout is loaded before other threads use it.
@build-analysis build-analysis bot mentioned this pull request Mar 8, 2024
Open
3 tasks
@jkotas
Copy link
Member

jkotas commented Mar 9, 2024 •
edited
Loading

Not sure if lowering the class load level to CLASS_LOAD_APPROXPARENTS is acceptable here.

I do not think it is acceptable. It is likely that the existing callers of this method expect the types to be loaded.

The crux of the problem seems to be that the RISCV managed->managed calling convention took a dependency on the interop marshalling info. The interop marshalling info is meant to be only used for marshalling used by managed/unmanaged interop that is a layer above the type loader. If you run an empty program, you should not see EEClassNativeLayoutInfo::CollectNativeLayoutFieldMetadataThrowing called at all or for just a few types. Using interop marshalling info for the basic managed->managed creates cycle between the two layers.

What would it take to break the cycle and stop depending on interop marshalling info for the managed->managed RISCV calling convention?

@tomeksowi
Copy link
Contributor Author

What would it take to break the cycle and stop depending on interop marshalling info for the managed->managed RISCV calling convention?

This part of MethodTable::GetRiscV64PassStructInRegisterFlags would need reworking, it falls back to GetNativeLayoutInfo().

runtime/src/coreclr/vm/methodtable.cpp

Lines 3536 to 3539 in 9c828a1

if (pMethodTable->HasLayout())
{
useNativeLayout = true;
}

I think it's possible to calculate the struct flags based on MethodTable::GetApproxFieldDescListRaw(). If so, even some of the interop marshalling could be covered that way too, as long as the layout is blittable.

That would be a bit more impactful than a simple bugfix I hoped for but may be worth it if it solves the problem once and for all.

@tomeksowi
Remove initializing native layout info for small structs during type …
3d730d2 
…load

Managed->managed calling convention should not depend on native info which is used for interop marshalling with unmanaged code which is above the type loader.
@tomeksowi
Don't use native layout in GetRiscV64PassStructInRegisterFlags unless…
d543f74 
… we're calculating flags for a native value type
@tomeksowi
Copy link
Contributor Author

I think it's possible to calculate the struct flags based on MethodTable::GetApproxFieldDescListRaw()

Pushed WiP, looks promising. Still need run more tests because it's used in so many places..

@tomeksowi tomeksowi changed the title [RISC-V] Initialize native layout info for small structs during type load [RISC-V] Avoid using native layout info to calculate register flags for small structs where possible Mar 13, 2024
@build-analysis build-analysis bot mentioned this pull request Mar 14, 2024
Closed
@tomeksowi
Copy link
Contributor Author

OK, CLR tests are looking good so far, I re-ran them with #99589 to check if the tests it unblocks don't fail with this PR (they don't).

@tomeksowi tomeksowi requested a review from jkotas March 14, 2024 15:09
@jkotas
Copy link
Member

jkotas commented Mar 14, 2024

LGTM. @dotnet/samsung Could you please signoff as well?

@jkotas
Copy link
Member

jkotas commented Mar 14, 2024

cc @dotnet/loongarch64-contrib The same bug exists in the Loongarch copy of this method. Should the same fix be made there?

In fact, IsLoongArch64OnlyOneField and IsRiscV64OnlyOneField methods are exactly same and there is nothing architecture specific in their implementation. Should this be de-duplicated into one platform-neutral IsOnlyOneField method?

sirntar
sirntar approved these changes Mar 15, 2024
View reviewed changes
Copy link
Member

@sirntar sirntar left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I think you should check whether a similar problem occurs on the LA64 @shushanhf

@jkotas jkotas merged commit da2cb96 into dotnet:main Mar 15, 2024
108 of 111 checks passed
@tomeksowi
Copy link
Contributor Author

Thank you for all the help @jkotas

@shushanhf
Copy link
Contributor

cc @dotnet/loongarch64-contrib The same bug exists in the Loongarch copy of this method. Should the same fix be made there?

In fact, IsLoongArch64OnlyOneField and IsRiscV64OnlyOneField methods are exactly same and there is nothing architecture specific in their implementation. Should this be de-duplicated into one platform-neutral IsOnlyOneField method?

Thanks very much~

@LuckyXu-HF LuckyXu-HF mentioned this pull request Mar 16, 2024
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yurai007 yurai007 yurai007 left review comments

@brucehoult brucehoult brucehoult left review comments

@sirntar sirntar sirntar approved these changes

@jkotas jkotas jkotas approved these changes

Assignees

@tomeksowi tomeksowi

Labels
arch-riscv Related to the RISC-V architecture area-VM-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tomeksowi @jkotas @shushanhf @brucehoult @yurai007 @sirntar @clamp03