Fix generic lookups and thunks (#129825)

Add support for generic lookups to Wasm R2R code by following the
DynamicHelper pattern we use on other platforms. This looks like a quite
suitable implementation for Wasm, so I expect that this will remain for
the long term.

Add support for WebAssembly .S files, as it was difficult to implement
the helpers in inline assembly due to clang limitations.
Tweak Crossgen2 into generating uses of the DynamicHelper logic.

Also add support for precomputed thunks for various runtime helpers
which were missing them, and add an assert that will fire even in
interpreted runs for missing cases.

Fixes (most) of #129622
Fixes #129821

---------

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Author: davidwrighton

eng/native/configurecompiler.cmake

@@ -1133,7 +1133,14 @@ if (CLR_CMAKE_HOST_WIN32)
         message(FATAL_ERROR "MC not found")
     endif()
 
-elseif (NOT CLR_CMAKE_HOST_BROWSER AND NOT CLR_CMAKE_HOST_WASI)
+elseif (CLR_CMAKE_HOST_BROWSER OR CLR_CMAKE_HOST_WASI)
+    # The wasm toolchains (emscripten / wasi-sdk) use clang, which can assemble
+    # preprocessed (.S) wasm assembly files directly.
+    set (CMAKE_ASM_COMPILER_VERSION "${CMAKE_C_COMPILER_VERSION}")
+
+    enable_language(ASM)
+
+else()
     # This is a workaround for upstream issue: https://gitlab.kitware.com/cmake/cmake/-/issues/22995.
     #
     # In Clang.cmake, the decision to use single or double hyphen for target and gcc-toolchain

src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelayLoadHelperImport.cs

@@ -41,15 +41,7 @@ public DelayLoadHelperImport(
             _useJumpableStub = useJumpableStub;
             if (factory.Target.Architecture == TargetArchitecture.Wasm32)
             {
-                if (instanceSignature is GenericLookupSignature)
-                {
-                    // Generic lookups are resolved via eager fixups and don't need import thunks
-                    _delayLoadHelper = null;
-                }
-                else
-                {
-                    _delayLoadHelper = factory.WasmImportThunkPortableEntrypoint(this);
-                }
+                _delayLoadHelper = factory.WasmImportThunkPortableEntrypoint(this);
             }
             else
             {

src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmImportThunkPortableEntrypoint.cs

@@ -57,14 +57,14 @@ public override int CompareToImpl(ISortableNode other, CompilerComparer comparer
 
         private static readonly WasmSignature _genericLookupSignature32Bit = new WasmSignature(
             new WasmFuncType(
-                new WasmResultType(new[] { WasmValueType.I32, WasmValueType.I32 }),
+                new WasmResultType(new[] { WasmValueType.I32, WasmValueType.I32, WasmValueType.I32 }),
                 new WasmResultType(new[] { WasmValueType.I32 })),
-            "iii");
+            "iip");
         private static readonly WasmSignature _genericLookupSignature64Bit = new WasmSignature(
             new WasmFuncType(
-                new WasmResultType(new[] { WasmValueType.I64, WasmValueType.I64 }),
+                new WasmResultType(new[] { WasmValueType.I64, WasmValueType.I64, WasmValueType.I64 }),
                 new WasmResultType(new[] { WasmValueType.I64 })),
-            "lll");
+            "llp");
 
         public override ObjectData GetData(NodeFactory factory, System.Boolean relocsOnly = false)
         {

src/coreclr/vm/CMakeLists.txt

@@ -943,12 +943,17 @@ elseif(CLR_CMAKE_TARGET_ARCH_RISCV64)
 elseif(CLR_CMAKE_TARGET_ARCH_WASM)
     set(VM_HEADERS_WKS_ARCH_ASM
         ${ARCH_SOURCES_DIR}/entrypoints.h
+        ${ARCH_SOURCES_DIR}/asmconstants.h
+    )
+    set(VM_SOURCES_WKS_ARCH_ASM
+        ${ARCH_SOURCES_DIR}/dynamichelpers.S
     )
     set(VM_SOURCES_WKS_ARCH
         ${RUNTIME_DIR}/${ARCH_SOURCES_DIR}/writebarriers.cpp
         ${ARCH_SOURCES_DIR}/calldescrworkerwasm.cpp
         ${ARCH_SOURCES_DIR}/profiler.cpp
         ${ARCH_SOURCES_DIR}/helpers.cpp
+        ${ARCH_SOURCES_DIR}/dynamichelpers.cpp
         exceptionhandling.cpp
         gcinfodecoder.cpp
     )

src/coreclr/vm/cgensys.h

@@ -68,7 +68,11 @@ extern "C" PCODE STDCALL DelayLoad_MethodCall(TransitionBlock* pTransitionBlock,
 extern "C" void STDCALL DelayLoad_MethodCall();
 #endif
 
+#ifdef TARGET_WASM
+extern "C" SIZE_T STDCALL DelayLoad_Helper(TransitionBlock* pTransitionBlock, READYTORUN_IMPORT_THUNK_PORTABLE_ENTRYPOINT* pImportThunkEntry, uint8_t *moduleBase, int32_t rvaOfModuleFixup);
+#else
 extern "C" void STDCALL DelayLoad_Helper();
+#endif
 extern "C" void STDCALL DelayLoad_Helper_Obj();
 extern "C" void STDCALL DelayLoad_Helper_ObjObj();
 #endif

src/coreclr/vm/jitinterface.cpp

@@ -11021,6 +11021,9 @@ void CEECodeGenInfo::getHelperFtn(CorInfoHelpFunc    ftnNum,               /* IN
     {
         helperMD = GetMethodDescForILBasedDynamicJitHelper(dynamicFtnNum);
         _ASSERTE(PortableEntryPoint::GetMethodDesc((PCODE)targetAddr) == helperMD);
+#ifdef FEATURE_READYTORUN
+        _ASSERTE(PortableEntryPoint::GetActualCode((PCODE)targetAddr) != NULL);
+#endif
     }
 
 #else // !FEATURE_PORTABLE_ENTRYPOINTS

src/coreclr/vm/prestub.cpp

@@ -2742,11 +2742,15 @@ EXTERN_C PCODE STDCALL ExternalMethodFixupWorker(TransitionBlock * pTransitionBl
         PTR_READYTORUN_IMPORT_SECTION pImportSection;
         if (sectionIndex != (DWORD)-1)
         {
+            // On some platforms (everywhere except wasm) we can get the section index from the callsite,
+            // so we don't have to search for it.
             pImportSection = pModule->GetImportSectionFromIndex(sectionIndex);
             _ASSERTE(pImportSection == pModule->GetImportSectionForRVA(rva));
         }
         else
         {
+            // On some platforms (currently only wasm) we would need to bloat the R2R binary a bit to store
+            // the section index, so we search for it instead.
             pImportSection = pModule->GetImportSectionForRVA(rva);
         }
         _ASSERTE(pImportSection != NULL);
@@ -3362,7 +3366,20 @@ PCODE DynamicHelperFixup(TransitionBlock * pTransitionBlock, TADDR * pCell, DWOR
 
     RVA rva = pNativeImage->GetDataRva((TADDR)pCell);
 
-    PTR_READYTORUN_IMPORT_SECTION pImportSection = pModule->GetImportSectionFromIndex(sectionIndex);
+    PTR_READYTORUN_IMPORT_SECTION pImportSection;
+    if (sectionIndex != (DWORD)-1)
+    {
+        // On some platforms (everywhere except wasm) we can get the section index from the callsite,
+        // so we don't have to search for it.
+        pImportSection = pModule->GetImportSectionFromIndex(sectionIndex);
+    }
+    else
+    {
+        // On some platforms (currently only wasm) we would need to bloat the R2R binary a bit to store
+        // the section index, so we search for it instead.
+        pImportSection = pModule->GetImportSectionForRVA(rva);
+    }
+
     _ASSERTE(pImportSection == pModule->GetImportSectionForRVA(rva));
 
     _ASSERTE(pImportSection->EntrySize == sizeof(TADDR));
@@ -3393,6 +3410,7 @@ PCODE DynamicHelperFixup(TransitionBlock * pTransitionBlock, TADDR * pCell, DWOR
 
     switch (kind)
     {
+#ifndef TARGET_WASM
     case READYTORUN_FIXUP_NewObject:
         th = ZapSig::DecodeType(pModule, pInfoModule, pBlob);
         th.AsMethodTable()->EnsureInstanceActive();
@@ -3444,7 +3462,7 @@ PCODE DynamicHelperFixup(TransitionBlock * pTransitionBlock, TADDR * pCell, DWOR
             pMD->EnsureActive();
         }
         break;
-
+#endif // !TARGET_WASM
     case READYTORUN_FIXUP_ThisObjDictionaryLookup:
     case READYTORUN_FIXUP_TypeDictionaryLookup:
     case READYTORUN_FIXUP_MethodDictionaryLookup:
@@ -3465,6 +3483,7 @@ PCODE DynamicHelperFixup(TransitionBlock * pTransitionBlock, TADDR * pCell, DWOR
         {
             switch (kind)
             {
+#ifndef TARGET_WASM
             case READYTORUN_FIXUP_IsInstanceOf:
             case READYTORUN_FIXUP_ChkCast:
                 {
@@ -3554,7 +3573,7 @@ PCODE DynamicHelperFixup(TransitionBlock * pTransitionBlock, TADDR * pCell, DWOR
                     }
                 }
                 break;
-
+#endif // !TARGET_WASM
             default:
                 UNREACHABLE();
             }
@@ -3578,6 +3597,7 @@ PCODE DynamicHelperFixup(TransitionBlock * pTransitionBlock, TADDR * pCell, DWOR
     {
         switch (kind)
         {
+#ifndef TARGET_WASM
         case READYTORUN_FIXUP_NewObject:
             {
                 bool fHasSideEffectsUnused;
@@ -3651,7 +3671,7 @@ PCODE DynamicHelperFixup(TransitionBlock * pTransitionBlock, TADDR * pCell, DWOR
                 }
             }
             break;
-
+#endif // !TARGET_WASM
         case READYTORUN_FIXUP_ThisObjDictionaryLookup:
         case READYTORUN_FIXUP_TypeDictionaryLookup:
         case READYTORUN_FIXUP_MethodDictionaryLookup:

src/coreclr/vm/readytoruninfo.cpp

@@ -2752,9 +2752,16 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator,
             else
             {
                 _ASSERTE(pLookup->sizeOffset == CORINFO_NO_SIZE_CHECK);
+                // SecondIndir is in bytes, but actual indirections into the table are always pointer aligned. 
+                // A value of 0 indicates that the second indirection is into the first generic dictionary of
+                // the type, which is the most common access pattern for generics. For Dictionary<TKey,TValue>,
+                // a SecondIndir of 0, and a LastIndir of 0 would indicate the MethodTable pointer of TKey,
+                // and if LastIndir was sizeof(TADDR) it would access the MethodTable pointer of TValue and so on.
                 if ((dictLookupData.SecondIndir == 0) && (dictLookupData.LastIndir <= sizeof(TADDR) * 3))
                 {
                     needsDictLookupData = false;
+                    // Since LastIndir is in bytes, but actual indirections into the table are always pointer
+                    // aligned, we can divide by sizeof(TADDR) to compute the possible cases here.
                     switch (dictLookupData.LastIndir / sizeof(TADDR))
                     {
                         case 0:
@@ -2784,6 +2791,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator,
             _ASSERTE(helperAddress == g_pMethodWithSlotAndModule);
             _ASSERTE(pLookup->offsets[0] == offsetof(InstantiatedMethodDesc, m_pPerInstInfo));
             dictLookupData.LastIndir = (UINT32)pLookup->offsets[1];
+            _ASSERTE(dictLookupData.SecondIndir == 0); // There are only 2 indirections, so there is no "SecondIndir" value to set, and it should be 0.
             if (pLookup->testForNull && pLookup->sizeOffset != CORINFO_NO_SIZE_CHECK)
             {
                 helper = (PCODE)DynamicHelper_GenericDictionaryLookup_Method_SizeCheck_TestForNull;
@@ -2797,9 +2805,10 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator,
             else
             {
                 _ASSERTE(pLookup->sizeOffset == CORINFO_NO_SIZE_CHECK);
-                if ((dictLookupData.SecondIndir == 0) && (dictLookupData.LastIndir <= sizeof(TADDR) * 3))
+                if (dictLookupData.LastIndir <= sizeof(TADDR) * 3)
                 {
                     needsDictLookupData = false;
+                    // Since LastIndir is in bytes, but actual indirections into the table are always pointer aligned, we can divide by sizeof(TADDR) to compute the possible cases here.
                     switch (dictLookupData.LastIndir / sizeof(TADDR))
                     {
                         case 0:

src/coreclr/vm/readytoruninfo.h

@@ -479,6 +479,14 @@ struct GenericDictionaryDynamicHelperStubData
     GenericHandleArgs *HandleArgs;
 };
 
+#ifdef FEATURE_PORTABLE_ENTRYPOINTS
+struct GenericDictionaryDynamicHelperStubData_PortableEntryPoint
+{
+    PCODE HelperFunctionTableIndex;
+    GenericDictionaryDynamicHelperStubData stubData;
+};
+#endif
+
 class ReadyToRunLoadedImage
 {
     TADDR m_pImageBase;

src/coreclr/vm/wasm/asmconstants.h

@@ -6,5 +6,55 @@
 // Be sure to rebuild clr/src/vm/ceemain.cpp after changing this file, to
 // ensure that the constants match the expected C/C++ values
 
+#ifndef ASMCONSTANTS_C_ASSERT
+#define ASMCONSTANTS_C_ASSERT(cond)
+#endif
+
+#ifndef ASMCONSTANTS_RUNTIME_ASSERT
+#define ASMCONSTANTS_RUNTIME_ASSERT(cond)
+#endif
+
+// Some constants are different in _DEBUG builds.  This macro factors out ifdefs from below.
+#ifdef _DEBUG
+#define DBG_FRE(dbg,fre) dbg
+#else
+#define DBG_FRE(dbg,fre) fre
+#endif
+
 #define DynamicHelperFrameFlags_ObjectArg   1
 #define DynamicHelperFrameFlags_ObjectArg2  2
+
+#define OFFSETOF__MethodTable__m_pPerInstInfo   DBG_FRE(0x24, 0x20)
+ASMCONSTANTS_C_ASSERT(OFFSETOF__MethodTable__m_pPerInstInfo
+                    == offsetof(MethodTable, m_pPerInstInfo));
+
+#define OFFSETOF__InstantiatedMethodDesc__m_pPerInstInfo   DBG_FRE(0x28, 0x14)
+ASMCONSTANTS_C_ASSERT(OFFSETOF__InstantiatedMethodDesc__m_pPerInstInfo
+                    == offsetof(InstantiatedMethodDesc, m_pPerInstInfo));
+
+#define OFFSETOF__GenericDictionaryDynamicHelperStubData_PortableEntryPoint__stubData 0x04
+ASMCONSTANTS_C_ASSERT(OFFSETOF__GenericDictionaryDynamicHelperStubData_PortableEntryPoint__stubData
+                    == offsetof(GenericDictionaryDynamicHelperStubData_PortableEntryPoint, stubData));
+
+#define OFFSETOF__GenericDictionaryDynamicHelperStubData_PortableEntryPoint__SecondIndir   0x4
+ASMCONSTANTS_C_ASSERT(OFFSETOF__GenericDictionaryDynamicHelperStubData_PortableEntryPoint__SecondIndir
+                    == offsetof(GenericDictionaryDynamicHelperStubData, SecondIndir) + offsetof(GenericDictionaryDynamicHelperStubData_PortableEntryPoint, stubData));
+
+#define OFFSETOF__GenericDictionaryDynamicHelperStubData_PortableEntryPoint__LastIndir     0x8
+ASMCONSTANTS_C_ASSERT(OFFSETOF__GenericDictionaryDynamicHelperStubData_PortableEntryPoint__LastIndir
+                    == offsetof(GenericDictionaryDynamicHelperStubData, LastIndir) + offsetof(GenericDictionaryDynamicHelperStubData_PortableEntryPoint, stubData));
+
+#define OFFSETOF__GenericDictionaryDynamicHelperStubData_PortableEntryPoint__SizeOffset    0xC
+ASMCONSTANTS_C_ASSERT(OFFSETOF__GenericDictionaryDynamicHelperStubData_PortableEntryPoint__SizeOffset
+                    == offsetof(GenericDictionaryDynamicHelperStubData, SizeOffset) + offsetof(GenericDictionaryDynamicHelperStubData_PortableEntryPoint, stubData));
+
+#define OFFSETOF__GenericDictionaryDynamicHelperStubData_PortableEntryPoint__SlotOffset    0x10
+ASMCONSTANTS_C_ASSERT(OFFSETOF__GenericDictionaryDynamicHelperStubData_PortableEntryPoint__SlotOffset
+                    == offsetof(GenericDictionaryDynamicHelperStubData, SlotOffset) + offsetof(GenericDictionaryDynamicHelperStubData_PortableEntryPoint, stubData));
+
+#define OFFSETOF__GenericDictionaryDynamicHelperStubData_PortableEntryPoint__HandleArgs    0x14
+ASMCONSTANTS_C_ASSERT(OFFSETOF__GenericDictionaryDynamicHelperStubData_PortableEntryPoint__HandleArgs
+                    == offsetof(GenericDictionaryDynamicHelperStubData, HandleArgs) + offsetof(GenericDictionaryDynamicHelperStubData_PortableEntryPoint, stubData));
+
+#undef ASMCONSTANTS_RUNTIME_ASSERT
+#undef ASMCONSTANTS_C_ASSERT