Skip to content

Commit

Permalink
Fix DEBUG/non-DEBUG SuperPMI asm diffs (#76470)
Browse files Browse the repository at this point in the history
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 #76347
  • Loading branch information
BruceForstall authored Oct 1, 2022
1 parent 26a91ad commit aea856f
Show file tree
Hide file tree
Showing 7 changed files with 35 additions and 5 deletions.
7 changes: 7 additions & 0 deletions src/coreclr/scripts/superpmi.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ MethodContext::MethodContext()

cr = new CompileResult();
index = -1;
ignoreStoredConfig = false;
isReadyToRunCompilation = ReadyToRunCompilation::Uninitialized;
}

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down
11 changes: 6 additions & 5 deletions src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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();
Expand All @@ -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);
Expand Down Expand Up @@ -879,6 +879,7 @@ class MethodContext
CompileResult* cr;
CompileResult* originalCR;
int index;
bool ignoreStoredConfig;

private:
bool IsEnvironmentHeaderEqual(const Environment& prevEnv);
Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/tools/superpmi/superpmi/commandline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/tools/superpmi/superpmi/commandline.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/tools/superpmi/superpmi/parallelsuperpmi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/tools/superpmi/superpmi/superpmi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
Expand Down

0 comments on commit aea856f

Please sign in to comment.