Support control flow guard on x86 Windows

[MichalStrehovsky]

The CFG test is disabled in #99372 as x86 Windows Native AOT was getting enabled.

Not sure where this ranks priority wise. We could just document this as not supported on x86 and wait for feedback. It's an option.

[jakobbotsch]

The main problem is the following:

https://github.com/dotnet/runtime/blob/d94257383e271f2358b83e13bd2c64084dea8c57/src/coreclr/jit/compiler.h#L9689-L9694

As I understand it, NAOT on x86 does not use this scheme and instead passes the indirection cell as an argument, in which case it could presumably just be enabled.

[filipnavara]

Simply enabling it runs into some JIT asserts. It would likely be easy to fix but it's not that trivial.

[jakobbotsch]

What asserts?

[filipnavara]

I'm in process of collecting the asserts / JIT dumps for all the tests that fail on win-x86. I'll post the details for this one once I am done.

[filipnavara]

Assert:

ILC: D:\runtime\src\coreclr\jit\lowerxarch.cpp:6647
ILC: Assertion failed 'ctrlExpr->isIndir()' in 'System.Collections.ObjectModel.ReadOnlyCollection`1[System.__Canon]:System.Collections.Generic.IList<T>.get_Item(int):System.__Canon:this' during 'Lowering nodeinfo' (IL size 13; hash 0xb8516b28; FullOpts)

JIT dump: ControlFlowGuard.txt

[filipnavara]

Presumably this would make it working:

diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp
index adc70840874..6bf0aa2254a 100644
--- a/src/coreclr/jit/codegenxarch.cpp
+++ b/src/coreclr/jit/codegenxarch.cpp
@@ -6241,7 +6241,7 @@ void CodeGen::genCallInstruction(GenTreeCall* call X86_ARG(target_ssize_t stackA
     if (target != nullptr)
     {
 #ifdef TARGET_X86
-        if (call->IsVirtualStub() && (call->gtCallType == CT_INDIRECT))
+        if (call->IsVirtualStub() && (call->gtCallType == CT_INDIRECT) && !compiler->IsTargetAbi(CORINFO_NATIVEAOT_ABI))
         {
             // On x86, we need to generate a very specific pattern for indirect VSD calls:
             //
diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h
index 048d6eced96..3c568afd746 100644
--- a/src/coreclr/jit/compiler.h
+++ b/src/coreclr/jit/compiler.h
@@ -9667,7 +9667,7 @@ public:
         // Check if the compilation is control-flow guard enabled.
         bool IsCFGEnabled() const
         {
-#if defined(TARGET_ARM64) || defined(TARGET_AMD64)
+#if defined(TARGET_ARM64) || defined(TARGET_AMD64) || defined(TARGET_X86)
             // On these platforms we assume the register that the target is
             // passed in is preserved by the validator and take care to get the
             // target from the register for the call (even in debug mode).
diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp
index 811657c9a52..0ee3dc73cda 100644
--- a/src/coreclr/jit/lowerxarch.cpp
+++ b/src/coreclr/jit/lowerxarch.cpp
@@ -6642,7 +6642,7 @@ void Lowering::ContainCheckCallOperands(GenTreeCall* call)
         //
         // Where EAX is also used as an argument to the stub dispatch helper. Make
         // sure that the call target address is computed into EAX in this case.
-        if (call->IsVirtualStub() && (call->gtCallType == CT_INDIRECT))
+        if (call->IsVirtualStub() && (call->gtCallType == CT_INDIRECT) && !comp->IsTargetAbi(CORINFO_NATIVEAOT_ABI))
         {
             assert(ctrlExpr->isIndir());
             MakeSrcContained(call, ctrlExpr);
diff --git a/src/coreclr/jit/lsraxarch.cpp b/src/coreclr/jit/lsraxarch.cpp
index 9853a91118f..616a6df23a8 100644
--- a/src/coreclr/jit/lsraxarch.cpp
+++ b/src/coreclr/jit/lsraxarch.cpp
@@ -1328,7 +1328,7 @@ int LinearScan::BuildCall(GenTreeCall* call)
             ctrlExprCandidates = RBM_INT_CALLEE_TRASH;
         }
 #ifdef TARGET_X86
-        else if (call->IsVirtualStub() && (call->gtCallType == CT_INDIRECT))
+        else if (call->IsVirtualStub() && (call->gtCallType == CT_INDIRECT) && !compiler->IsTargetAbi(CORINFO_NATIVEAOT_ABI))
         {
             // On x86, we need to generate a very specific pattern for indirect VSD calls:
             //
diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp
index 92dece70960..60f0f03daca 100644
--- a/src/coreclr/jit/morph.cpp
+++ b/src/coreclr/jit/morph.cpp
@@ -2055,7 +2055,7 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call
     // If/when we change that, the following code needs to be changed to correctly support the (TBD) managed calling
     // convention for x86/SSE.

-    addStubCellArg = call->gtCallType != CT_INDIRECT && comp->IsTargetAbi(CORINFO_NATIVEAOT_ABI);
+    addStubCellArg = comp->IsTargetAbi(CORINFO_NATIVEAOT_ABI);
 #endif

     // We are allowed to have a ret buffer argument combined
--- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs
+++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs
@@ -831,7 +831,8 @@ public ISortableSymbolNode ExternVariable(string name)

         public ISortableSymbolNode ExternIndirectSymbol(string name)
         {
-            return _externIndirectSymbols.GetOrAdd(name);
+            string mangledName = NameMangler.NodeMangler.ExternVariable(name);
+            return _externIndirectSymbols.GetOrAdd(mangledName);
         }

         private NodeCache<PInvokeModuleData, PInvokeModuleFixupNode> _pInvokeModuleFixups;
diff --git a/src/tests/nativeaot/SmokeTests/ControlFlowGuard/ControlFlowGuard.csproj b/src/tests/nativeaot/SmokeTests/ControlFlowGuard/ControlFlowGuard.csproj
index 1fd766631b4..fd963421b87 100644
--- a/src/tests/nativeaot/SmokeTests/ControlFlowGuard/ControlFlowGuard.csproj
+++ b/src/tests/nativeaot/SmokeTests/ControlFlowGuard/ControlFlowGuard.csproj
@@ -6,7 +6,7 @@
     <ControlFlowGuard>Guard</ControlFlowGuard>
     <Optimize>true</Optimize>
     <!-- x86 support tracked at https://github.com/dotnet/runtime/issues/99516 -->
-    <CLRTestTargetUnsupported Condition="'$(TargetsWindows)' != 'true' or '$(TargetArchitecture)' == 'x86' ">true</CLRTestTargetUnsupported>
+    <CLRTestTargetUnsupported Condition="'$(TargetsWindows)' != 'true'">true</CLRTestTargetUnsupported>
   </PropertyGroup>
   <ItemGroup>
     <Compile Include="ControlFlowGuard.cs" />

I'd rather wait for the test infrastructure to be in place to submit it as PR. We also need to add check to keep it disabled for regular CoreCLR.

[MichalStrehovsky]

We also need to add check to keep it disabled for regular CoreCLR.

Our general testing strategy assumes we do all codegen stress testing on top of JIT based CoreCLR (we have CI legs with control flow guard codegen with CoreCLR-JIT for x64 and arm64).

NativeAOT doesn't do much JIT testing (and we only run one test with CFG on). We might need to rethink our testing strategy if x86 will not be covered in the CoreCLR-JIT CFG legs. (I.e. might be better to leave this not working than to just enable for NAOT.) Ultimately, the call on this is on the codegen team.

[filipnavara]

Half of the diff is general optimization to not emit unnecessary NOPs and to correctly decorate indirect symbols (only used by CFG). I'll submit that sooner or later anyway.

I'll let others decide about the one #if and testing to actually enable the CFG code paths.

[jakobbotsch]

I think we should just keep it disabled unless we get an actual request for the functionality. As @MichalStrehovsky mentions we would need to expand jit-cfg to cover this configuration as well (potentially that's just adding the leg, but who knows what unforeseen work that brings with it...).

[MichalPetryka]

I think we should just keep it disabled unless we get an actual request for the functionality.

I agree with this but if @filipnavara has patches that make it work, they should probably still be merged so that enabling it is easier in the future.