Fix SuperPMI collect with getIntConfigValue (#53442)

* Fix SuperPMI collect with getIntConfigValue

PR #52427 introduced a per-compilation call to getIntConfigValue
on "SuperPMIMethodContextNumber". This pseudo-config should not
be collected. It exposed a pre-existing multi-threading issue in
the superpmi collector shim. I got rid of the per-compilation
override of the jithost MethodContext, which is problematic, and
currently unneeded, but documented the considerations around collecting
per-compilation data.

Fixes #53440

* Update src/coreclr/ToolBox/superpmi/superpmi-shim-collector/jithost.cpp

Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>

Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>

3 files changed, 23 insertions, 17 deletions

diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shim-collector/icorjitcompiler.cpp b/src/coreclr/ToolBox/superpmi/superpmi-shim-collector/icorjitcompiler.cpp
index 69b5b117727..da110199d86 100644
--- a/src/coreclr/ToolBox/superpmi/superpmi-shim-collector/icorjitcompiler.cpp
+++ b/src/coreclr/ToolBox/superpmi/superpmi-shim-collector/icorjitcompiler.cpp
@@ -21,11 +21,6 @@ CorJitResult interceptor_ICJC::compileMethod(ICorJitInfo*                comp,
     our_ICorJitInfo.original_ICorJitInfo = comp;
 
     auto* mc = new MethodContext();
-    if (g_ourJitHost != nullptr)
-    {
-        g_ourJitHost->setMethodContext(mc);
-    }
-
     our_ICorJitInfo.mc = mc;
     our_ICorJitInfo.mc->cr->recProcessName(GetCommandLineA());
 
@@ -74,11 +69,6 @@ CorJitResult interceptor_ICJC::compileMethod(ICorJitInfo*                comp,
 
     delete mc;
 
-    if (g_ourJitHost != nullptr)
-    {
-        g_ourJitHost->setMethodContext(g_globalContext);
-    }
-
     return temp;
 }
 
diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shim-collector/jithost.cpp b/src/coreclr/ToolBox/superpmi/superpmi-shim-collector/jithost.cpp
index e77203b0368..e243306a1ec 100644
--- a/src/coreclr/ToolBox/superpmi/superpmi-shim-collector/jithost.cpp
+++ b/src/coreclr/ToolBox/superpmi/superpmi-shim-collector/jithost.cpp
@@ -6,6 +6,20 @@
 #include "spmiutil.h"
 #include "jithost.h"
 
+// There is a single JitHost object created during the one-time initialization of the JIT (function jitStartup),
+// and shared amongst all subsequent compilations. Any calls to the getIntConfigValue/getStringConfigValue
+// APIs get recorded in a single, global MethodContext/CompileResult, and are copied to the
+// per-compilation MethodContext in the shim implementation of compileMethod (using recGlobalContext()).
+// This works because the JIT eagerly asks for all config values once, in the one-time jitStartup
+// function. If the JIT were to ask for config values later, during the per-compilation phase,
+// they would get recorded here in the global MethodContext, and copied to all subsequent
+// compilation MethodContexts. This would be incorrect. A solution would be to use a per-compilation
+// MethodContext in addition to the global MethodContext, but we have to allow for multi-threading. That is,
+// there could be multiple JIT compilations happening concurrently, so we can't just replace the global
+// MethodContext with a per-compilation MethodContext. Perhaps per-compilation MethodContext could be
+// stored in a map from OS thread id to MethodContext, and looked up here based on thread id. The host APIs
+// have no per-compilation knowledge.
+
 JitHost* g_ourJitHost;
 
 // RecordVariable: return `true` if the given COMPlus variable `key` should be recorded
@@ -39,11 +53,6 @@ JitHost::JitHost(ICorJitHost* wrappedHost, MethodContext* methodContext) : wrapp
 {
 }
 
-void JitHost::setMethodContext(MethodContext* methodContext)
-{
-    this->mc = methodContext;
-}
-
 void* JitHost::allocateMemory(size_t size)
 {
     return wrappedHost->allocateMemory(size);
@@ -56,6 +65,15 @@ void JitHost::freeMemory(void* block)
 
 int JitHost::getIntConfigValue(const WCHAR* key, int defaultValue)
 {
+    // Special-case handling: don't collect this pseudo-variable, and don't
+    // even record that it was called (since it would get recorded into the
+    // global state). (See the superpmi.exe tool implementation of JitHost::getIntConfigValue()
+    // for the special-case implementation of this.)
+    if (wcscmp(key, W("SuperPMIMethodContextNumber")) == 0)
+    {
+        return defaultValue;
+    }
+
     mc->cr->AddCall("getIntConfigValue");
     int result = wrappedHost->getIntConfigValue(key, defaultValue);
 
diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shim-collector/jithost.h b/src/coreclr/ToolBox/superpmi/superpmi-shim-collector/jithost.h
index 76a06f15261..a282f409600 100644
--- a/src/coreclr/ToolBox/superpmi/superpmi-shim-collector/jithost.h
+++ b/src/coreclr/ToolBox/superpmi/superpmi-shim-collector/jithost.h
@@ -9,8 +9,6 @@ class JitHost : public ICorJitHost
 public:
     JitHost(ICorJitHost* wrappedHost, MethodContext* methodContext);
 
-    void setMethodContext(MethodContext* methodContext);
-
 #include "icorjithostimpl.h"
 
 private: