Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DEBUG/non-DEBUG SuperPMI asm diffs #76470

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of moving asmdiffs to run on Release instead of Checked.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When doing so, you'll need to audit usage of diff_with_release. It was introduced for use with Checked/Release diffs (so is not completely accurately named) and also before Release builds had any JitDisasm support (JitDiffableDasm still isn't in Release), so I think it kind of mixes concepts a bit.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (ignoreStoredConfig)
if (ignoreStoredConfig || (GetIntConfigValue == nullptr))
return defaultValue;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually prefer keeping these cases as separate statements.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (ignoreStoredConfig)
if (ignoreStoredConfig || (GetStringConfigValue == nullptr))
return nullptr;

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