From 99ebea09e15f163bd8f64686b645312bfc8c47e2 Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Fri, 30 Sep 2022 14:57:14 -0700 Subject: [PATCH] Fix DEBUG/non-DEBUG SuperPMI asm diffs Recorded SPMI method contexts include configuration environment variables such as `COMPlus_JITMinOpts` that are replayed. However, when doing asmdiffs replays to compare a Release to a Checked compiler (non-DEBUG to DEBUG), there may be codegen-altering configuration variables such as JitStress that are only read and interpreted by the DEBUG compiler. This leads to asm diffs. Introduce a `-ignoreStoredConfig` argument to superpmi.exe, and use it in superpmi.py when doing Checked/Release asm diffs, that pretends there are no stored config variables. This assumes that the stored config variables only alter JIT behavior but that they JIT will succeed with or without them. This is also slightly more than necessary: if there is a config variable that the Release compiler knows about, we won't use that, either. However, we have no easy way (currently) to distinguish which variables are DEBUG and which are both DEBUG and non-DEBUG available. Contributes to https://github.com/dotnet/runtime/issues/76347 --- src/coreclr/scripts/superpmi.py | 7 +++++++ .../tools/superpmi/superpmi-shared/methodcontext.cpp | 7 +++++++ .../tools/superpmi/superpmi-shared/methodcontext.h | 11 ++++++----- src/coreclr/tools/superpmi/superpmi/commandline.cpp | 7 +++++++ src/coreclr/tools/superpmi/superpmi/commandline.h | 2 ++ .../tools/superpmi/superpmi/parallelsuperpmi.cpp | 1 + src/coreclr/tools/superpmi/superpmi/superpmi.cpp | 5 +++++ 7 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index ae45443620cd6c..d4a386333a634d 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1608,6 +1608,13 @@ def replay_with_asm_diffs(self): if self.coreclr_args.error_limit is not None: flags += ["-failureLimit", self.coreclr_args.error_limit] + if self.coreclr_args.diff_with_release: + # If one of the JITs is Release, ignore all the stored configuration variables. + # This isn't necessary if both are Release builds (and, in fact, it can clear variables + # that both Release compilers can interpret). However, we rarely or never compare + # two Release compilers, so this is safest. + flags += [ "-ignoreStoredConfig" ] + # Change the working directory to the Core_Root we will call SuperPMI from. # This is done to allow libcoredistools to be loaded correctly on unix # as the loadlibrary path will be relative to the current directory. diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp index 23c52a3ebaec5a..2faea5a996e80b 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp @@ -30,6 +30,7 @@ MethodContext::MethodContext() cr = new CompileResult(); index = -1; + ignoreStoredConfig = false; isReadyToRunCompilation = ReadyToRunCompilation::Uninitialized; } @@ -6903,6 +6904,9 @@ void MethodContext::dmpGetIntConfigValue(const Agnostic_ConfigIntInfo& key, int int MethodContext::repGetIntConfigValue(const WCHAR* name, int defaultValue) { + if (ignoreStoredConfig) + return defaultValue; + if (GetIntConfigValue == nullptr) return defaultValue; @@ -6955,6 +6959,9 @@ void MethodContext::dmpGetStringConfigValue(DWORD nameIndex, DWORD resultIndex) const WCHAR* MethodContext::repGetStringConfigValue(const WCHAR* name) { + if (ignoreStoredConfig) + return nullptr; + if (GetStringConfigValue == nullptr) return nullptr; diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h index f9769119e20b5d..354cdb8213966e 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h @@ -77,9 +77,6 @@ class MethodContext MethodContext(); private: - MethodContext(HANDLE hFile); - MethodContext(unsigned char* buff, unsigned int totalLen); - void MethodInitHelper(unsigned char* buff, unsigned int totalLen); void MethodInitHelperFile(HANDLE hFile); @@ -94,6 +91,11 @@ class MethodContext ~MethodContext(); void Destroy(); + void setIgnoreStoredConfig() + { + ignoreStoredConfig = true; + } + bool Equal(MethodContext* other); unsigned int saveToFile(HANDLE hFile); unsigned int calculateFileSize(); @@ -114,8 +116,6 @@ class MethodContext void recGlobalContext(const MethodContext& other); - void dmpEnvironment(DWORD key, const Agnostic_Environment& value); - void recCompileMethod(CORINFO_METHOD_INFO* info, unsigned flags, CORINFO_OS os); void dmpCompileMethod(DWORD key, const Agnostic_CompileMethod& value); void repCompileMethod(CORINFO_METHOD_INFO* info, unsigned* flags, CORINFO_OS* os); @@ -879,6 +879,7 @@ class MethodContext CompileResult* cr; CompileResult* originalCR; int index; + bool ignoreStoredConfig; private: bool IsEnvironmentHeaderEqual(const Environment& prevEnv); diff --git a/src/coreclr/tools/superpmi/superpmi/commandline.cpp b/src/coreclr/tools/superpmi/superpmi/commandline.cpp index ca407889e87235..e60a24a7a4f4e5 100644 --- a/src/coreclr/tools/superpmi/superpmi/commandline.cpp +++ b/src/coreclr/tools/superpmi/superpmi/commandline.cpp @@ -39,6 +39,9 @@ void CommandLine::DumpHelp(const char* program) printf(" -box\n"); printf(" Break on exception thrown, such as for missing data during replay\n"); printf("\n"); + printf(" -ignoreStoredConfig\n"); + printf(" On replay, ignore any stored configuration variables. Useful for Checked/Release asm diffs.\n"); + printf("\n"); printf(" -v[erbosity] messagetypes\n"); printf(" Controls which types of messages SuperPMI logs. Specify a string of\n"); printf(" characters representing message categories to enable, where:\n"); @@ -350,6 +353,10 @@ bool CommandLine::Parse(int argc, char* argv[], /* OUT */ Options* o) { o->breakOnException = true; } + else if ((_strnicmp(&argv[i][1], "ignoreStoredConfig", 18) == 0)) + { + o->ignoreStoredConfig = true; + } else if ((_strnicmp(&argv[i][1], "verbosity", argLen) == 0)) { if (++i >= argc) diff --git a/src/coreclr/tools/superpmi/superpmi/commandline.h b/src/coreclr/tools/superpmi/superpmi/commandline.h index e801f8cb4f752d..f0dc09bd1617f3 100644 --- a/src/coreclr/tools/superpmi/superpmi/commandline.h +++ b/src/coreclr/tools/superpmi/superpmi/commandline.h @@ -22,6 +22,7 @@ class CommandLine , breakOnError(false) , breakOnAssert(false) , breakOnException(false) + , ignoreStoredConfig(false) , applyDiff(false) , parallel(false) #if !defined(USE_MSVCDIS) && defined(USE_COREDISTOOLS) @@ -59,6 +60,7 @@ class CommandLine bool breakOnError; bool breakOnAssert; bool breakOnException; + bool ignoreStoredConfig; bool applyDiff; bool parallel; // User specified to use /parallel mode. bool useCoreDisTools; // Use CoreDisTools library instead of Msvcdis diff --git a/src/coreclr/tools/superpmi/superpmi/parallelsuperpmi.cpp b/src/coreclr/tools/superpmi/superpmi/parallelsuperpmi.cpp index 610ccdf732b73e..228fea9393412c 100644 --- a/src/coreclr/tools/superpmi/superpmi/parallelsuperpmi.cpp +++ b/src/coreclr/tools/superpmi/superpmi/parallelsuperpmi.cpp @@ -487,6 +487,7 @@ char* ConstructChildProcessArgs(const CommandLine::Options& o) ADDARG_BOOL(o.breakOnError, "-boe"); ADDARG_BOOL(o.breakOnAssert, "-boa"); ADDARG_BOOL(o.breakOnException, "-box"); + ADDARG_BOOL(o.ignoreStoredConfig, "-ignoreStoredConfig"); ADDARG_BOOL(o.applyDiff, "-applyDiff"); ADDARG_STRING(o.reproName, "-reproName"); ADDARG_STRING(o.writeLogFile, "-writeLogFile"); diff --git a/src/coreclr/tools/superpmi/superpmi/superpmi.cpp b/src/coreclr/tools/superpmi/superpmi/superpmi.cpp index 7875d4ca111199..c0d64df1acc208 100644 --- a/src/coreclr/tools/superpmi/superpmi/superpmi.cpp +++ b/src/coreclr/tools/superpmi/superpmi/superpmi.cpp @@ -328,6 +328,11 @@ int __cdecl main(int argc, char* argv[]) return (int)SpmiResult::GeneralFailure; } + if (o.ignoreStoredConfig) + { + mc->setIgnoreStoredConfig(); + } + if (reader->IsMethodExcluded(mc)) { excludedCount++;