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.

Sign up for GitHub

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

Change SuperPMI collection to not use altjit mechanism #44834

Merged
merged 3 commits into from
Nov 18, 2020
Merged

Change SuperPMI collection to not use altjit mechanism #44834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8a75cc0
Change SuperPMI collection to not use altjit mechanism
BruceForstall Nov 17, 2020
f09c981
Enable `COMPlus_JitName` for crossgen as well
BruceForstall Nov 17, 2020
d26220f
Fix issue with empty collection args
BruceForstall Nov 17, 2020
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
118 changes: 72 additions & 46 deletions src/coreclr/scripts/superpmi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@

superpmi_collect_help = """\
Command to run SuperPMI collect over. Note that there cannot be any dotnet CLI commands
invoked inside this command, as they will fail due to the shim altjit being set.
invoked inside this command, as they will fail due to the shim JIT being set.
"""

replay_mch_files_help = """\
Expand Down Expand Up @@ -750,9 +750,7 @@ def __collect_mc_files__(self):
env_copy = os.environ.copy()
env_copy["SuperPMIShimLogPath"] = self.temp_location
env_copy["SuperPMIShimPath"] = self.jit_path
env_copy["COMPlus_AltJit"] = "*"
env_copy["COMPlus_AltJitNgen"] = "*"
env_copy["COMPlus_AltJitName"] = self.collection_shim_name
env_copy["COMPlus_JitName"] = self.collection_shim_name
env_copy["COMPlus_EnableExtraSuperPmiQueries"] = "1"
env_copy["COMPlus_TieredCompilation"] = "0"

Expand All @@ -764,9 +762,7 @@ def __collect_mc_files__(self):
logging.debug("")
print_platform_specific_environment_vars(logging.DEBUG, self.coreclr_args, "SuperPMIShimLogPath", self.temp_location)
print_platform_specific_environment_vars(logging.DEBUG, self.coreclr_args, "SuperPMIShimPath", self.jit_path)
print_platform_specific_environment_vars(logging.DEBUG, self.coreclr_args, "COMPlus_AltJit", "*")
print_platform_specific_environment_vars(logging.DEBUG, self.coreclr_args, "COMPlus_AltJitNgen", "*")
print_platform_specific_environment_vars(logging.DEBUG, self.coreclr_args, "COMPlus_AltJitName", self.collection_shim_name)
print_platform_specific_environment_vars(logging.DEBUG, self.coreclr_args, "COMPlus_JitName", self.collection_shim_name)
print_platform_specific_environment_vars(logging.DEBUG, self.coreclr_args, "COMPlus_EnableExtraSuperPmiQueries", "1")
print_platform_specific_environment_vars(logging.DEBUG, self.coreclr_args, "COMPlus_TieredCompilation", "0")
if self.coreclr_args.use_zapdisable:
Expand Down Expand Up @@ -1072,36 +1068,48 @@ def replay(self):
logging.debug("Temp Location: %s", temp_location)
logging.debug("")

flags = [
# `repro_flags` are the subset of flags we tell the user to pass to superpmi when reproducing
# a failure. This won't include things like "-p" for parallelism or "-r" to create a repro .mc file.
repro_flags = []

common_flags = [
"-v", "ew", # only display errors and warnings
"-r", os.path.join(temp_location, "repro") # Repro name, create .mc repro files
]

altjit_string = "*" if self.coreclr_args.altjit else ""
altjit_replay_flags = [
"-jitoption", "force", "AltJit=" + altjit_string,
"-jitoption", "force", "AltJitNgen=" + altjit_string,
"-jitoption", "force", "EnableExtraSuperPmiQueries=0"
]
flags += altjit_replay_flags
if self.coreclr_args.altjit:
repro_flags += [
"-jitoption", "force", "AltJit=*",
"-jitoption", "force", "AltJitNgen=*"
]
if self.coreclr_args.arch == "arm":
repro_flags += [ "-target", "arm" ]
elif self.coreclr_args.arch == "arm64":
repro_flags += [ "-target", "arm64" ]

if not self.coreclr_args.sequential:
flags += [ "-p" ]
common_flags += [ "-p" ]

if self.coreclr_args.break_on_assert:
flags += [ "-boa" ]
common_flags += [ "-boa" ]

if self.coreclr_args.break_on_error:
flags += [ "-boe" ]
common_flags += [ "-boe" ]

if self.coreclr_args.spmi_log_file is not None:
flags += [ "-w", self.coreclr_args.spmi_log_file ]
common_flags += [ "-w", self.coreclr_args.spmi_log_file ]

# TEMPORARY: when we have collections that are done using COMPlus_JitName
# instead of COMPlus_AltJit, we can remove these.
if not self.coreclr_args.altjit:
repro_flags += [
"-jitoption", "force", "AltJit=",
"-jitoption", "force", "AltJitNgen="
]
repro_flags += [ "-jitoption", "force", "EnableExtraSuperPmiQueries=0" ]
# END TEMPORARY

if self.coreclr_args.altjit:
if self.coreclr_args.arch == "arm":
flags += [ "-target", "arm" ]
elif self.coreclr_args.arch == "arm64":
flags += [ "-target", "arm64" ]
common_flags += repro_flags

# For each MCH file that we are going to replay, do the replay and replay post-processing.
#
Expand All @@ -1117,6 +1125,8 @@ def replay(self):

logging.info("Running SuperPMI replay of %s", mch_file)

flags = common_flags

fail_mcl_file = os.path.join(temp_location, os.path.basename(mch_file) + "_fail.mcl")
flags += [
"-f", fail_mcl_file, # Failing mc List
Expand All @@ -1136,7 +1146,7 @@ def replay(self):
if return_code == 0:
logging.warning("Warning: SuperPMI returned a zero exit code, but generated a non-zero-sized mcl file")
print_fail_mcl_file_method_numbers(fail_mcl_file)
repro_base_command_line = "{} {} {}".format(self.superpmi_path, " ".join(altjit_replay_flags), self.jit_path)
repro_base_command_line = "{} {} {}".format(self.superpmi_path, " ".join(repro_flags), self.jit_path)
save_repro_mc_files(temp_location, self.coreclr_args, repro_base_command_line)

if not self.coreclr_args.skip_cleanup:
Expand Down Expand Up @@ -1238,22 +1248,44 @@ def replay_with_asm_diffs(self):
"COMPlus_JitDump": "*",
"COMPlus_NgenDump": "*" }

altjit_string = "*" if self.coreclr_args.altjit else ""
altjit_asm_diffs_flags = []
altjit_replay_flags = []

if self.coreclr_args.altjit:
target_flags = []
if self.coreclr_args.arch == "arm":
target_flags += [ "-target", "arm" ]
elif self.coreclr_args.arch == "arm64":
target_flags += [ "-target", "arm64" ]

altjit_asm_diffs_flags = target_flags + [
"-jitoption", "force", "AltJit=*",
"-jitoption", "force", "AltJitNgen=*",
"-jit2option", "force", "AltJit=*",
"-jit2option", "force", "AltJitNgen=*"
]

altjit_asm_diffs_flags = [
"-jitoption", "force", "AltJit=" + altjit_string,
"-jitoption", "force", "AltJitNgen=" + altjit_string,
"-jitoption", "force", "EnableExtraSuperPmiQueries=0",
"-jit2option", "force", "AltJit=" + altjit_string,
"-jit2option", "force", "AltJitNgen=" + altjit_string,
"-jit2option", "force", "EnableExtraSuperPmiQueries=0"
]
altjit_replay_flags = target_flags + [
"-jitoption", "force", "AltJit=*",
"-jitoption", "force", "AltJitNgen=*"
]

altjit_replay_flags = [
"-jitoption", "force", "AltJit=" + altjit_string,
"-jitoption", "force", "AltJitNgen=" + altjit_string,
"-jitoption", "force", "EnableExtraSuperPmiQueries=0"
]
# TEMPORARY: when we have collections that are done using COMPlus_JitName
# instead of COMPlus_AltJit, we can remove these.
if not self.coreclr_args.altjit:
altjit_asm_diffs_flags = [
"-jitoption", "force", "AltJit=",
"-jitoption", "force", "AltJitNgen=",
"-jit2option", "force", "AltJit=",
"-jit2option", "force", "AltJitNgen="
]
altjit_replay_flags = [
"-jitoption", "force", "AltJit=",
"-jitoption", "force", "AltJitNgen="
]
altjit_asm_diffs_flags += [ "-jitoption", "force", "EnableExtraSuperPmiQueries=0" ]
altjit_replay_flags += [ "-jitoption", "force", "EnableExtraSuperPmiQueries=0" ]
# END TEMPORARY

# Keep track if any MCH file replay had asm diffs
files_with_asm_diffs = []
Expand Down Expand Up @@ -1304,12 +1336,6 @@ def replay_with_asm_diffs(self):
if self.coreclr_args.spmi_log_file is not None:
flags += [ "-w", self.coreclr_args.spmi_log_file ]

if self.coreclr_args.altjit:
if self.coreclr_args.arch == "arm":
flags += [ "-target", "arm" ]
elif self.coreclr_args.arch == "arm64":
flags += [ "-target", "arm64" ]

# 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 Expand Up @@ -2699,7 +2725,7 @@ def verify_replay_common_args():
"collection_args",
lambda unused: True,
"Unable to set collection_args",
modify_arg=lambda collection_args: collection_args.split(" ") if collection_args is not None else collection_args)
modify_arg=lambda collection_args: collection_args.split(" ") if collection_args is not None else [])

coreclr_args.verify(args,
"pmi",
Expand Down
8 changes: 2 additions & 6 deletions src/coreclr/src/ToolBox/superpmi/readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,7 @@ Set the following environment variables:
```
SuperPMIShimLogPath=<full path to an existing, empty temporary directory>
SuperPMIShimPath=<full path to clrjit.dll, the "standalone" JIT>
COMPlus_AltJit=*
COMPlus_AltJitNgen=*
COMPlus_AltJitName=superpmi-shim-collector.dll
COMPlus_JitName=superpmi-shim-collector.dll
```

for example, on Windows:
Expand All @@ -132,9 +130,7 @@ for example, on Windows:
mkdir f:\spmi\temp
set SuperPMIShimLogPath=f:\spmi\temp
set SuperPMIShimPath=f:\gh\runtime\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root\clrjit.dll
set COMPlus_AltJit=*
set COMPlus_AltJitNgen=*
set COMPlus_AltJitName=superpmi-shim-collector.dll
set COMPlus_JitName=superpmi-shim-collector.dll
```

(On Linux, use `libclrjit.so` and `libsuperpmi-shim-collector.so`.
Expand Down
31 changes: 29 additions & 2 deletions src/coreclr/src/ToolBox/superpmi/superpmi-shim-collector/jithost.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,33 @@

JitHost* g_ourJitHost;

// RecordVariable: return `true` if the given COMPlus variable `key` should be recorded
// in the method context.
bool RecordVariable(const WCHAR* key)
{
// Special-case: we don't want to store some COMPlus variables during
// collections that we don't want to see on replay:
// COMPlus_JitName -- used to get the VM to load the SuperPMI collection shim
// without requiring the shim to overwrite the original JIT.
// This JIT doesn't care about this on SuperPMI replay, but
// we don't need to waste the space in the MC file storing it.
// COMPlus_AltJitName -- if collecting with an altjit, this is set. The JIT doesn't
// use this on replay, but it doesn't need to be stored.
// COMPlus_EnableExtraSuperPmiQueries -- used to force the JIT to ask additional
// questions during SuperPMI collection. We don't want to store
// this variable because we don't want to replay using it.

if ((_wcsicmp(key, W("JitName")) == 0) ||
(_wcsicmp(key, W("AltJitName")) == 0) ||
(_wcsicmp(key, W("EnableExtraSuperPmiQueries")) == 0))
{
return false;
}

// By default, we record everything.
return true;
}

JitHost::JitHost(ICorJitHost* wrappedHost, MethodContext* methodContext) : wrappedHost(wrappedHost), mc(methodContext)
{
}
Expand Down Expand Up @@ -37,7 +64,7 @@ int JitHost::getIntConfigValue(const WCHAR* key, int defaultValue)
// The JIT eagerly asks about every config value. If we store all these
// queries, it takes almost half the MC file space. So only store the
// non-default answers.
if (result != defaultValue)
if (RecordVariable(key) && (result != defaultValue))
{
mc->recGetIntConfigValue(key, defaultValue, result);
}
Expand All @@ -50,7 +77,7 @@ const WCHAR* JitHost::getStringConfigValue(const WCHAR* key)
const WCHAR* result = wrappedHost->getStringConfigValue(key);

// Don't store null returns, which is the default
if (result != nullptr)
if (RecordVariable(key) && (result != nullptr))
{
mc->recGetStringConfigValue(key, result);
}
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/src/ToolBox/superpmi/superpmi/jitinstance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ JitInstance* JitInstance::InitJit(char* nameOfJit,
jit->options = options;

// The flag to cause the JIT to be invoked as an altjit is stored in the jit flags, not in
// the environment. If the user uses the "-jitoption force" flag to force AltJit off (it was
// probably on during collection), or to force it on, then propagate that to the jit flags.
// the environment. If the user uses the "-jitoption force" flag to force AltJit off
// or to force it on, then propagate that to the jit flags.
jit->forceClearAltJitFlag = false;
jit->forceSetAltJitFlag = false;
const WCHAR* altJitFlag = jit->getForceOption(W("AltJit"));
Expand Down
89 changes: 69 additions & 20 deletions src/coreclr/src/vm/codeman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1557,6 +1557,39 @@ struct JIT_LOAD_DATA
// Here's the global data for JIT load and initialization state.
JIT_LOAD_DATA g_JitLoadData;

// Validate that the name used to load the JIT is just a simple file name
// and does not contain something that could be used in a non-qualified path.
// For example, using the string "..\..\..\myjit.dll" we might attempt to
// load a JIT from the root of the drive.
//
// The minimal set of characters that we must check for and exclude are:
// '\\' - (backslash)
// '/' - (forward slash)
// ':' - (colon)
//
// Returns false if we find any of these characters in 'pwzJitName'
// Returns true if we reach the null terminator without encountering
// any of these characters.
//
static bool ValidateJitName(LPCWSTR pwzJitName)
{
LPCWSTR pCurChar = pwzJitName;
wchar_t curChar;
do {
curChar = *pCurChar;
if ((curChar == '\\') || (curChar == '/') || (curChar == ':'))
{
// Return false if we find any of these character in 'pwzJitName'
return false;
}
pCurChar++;
} while (curChar != 0);

// Return true; we have reached the null terminator
//
return true;
}

// LoadAndInitializeJIT: load the JIT dll into the process, and initialize it (call the UtilCode initialization function,
// check the JIT-EE interface GUID, etc.)
//
Expand Down Expand Up @@ -1589,34 +1622,40 @@ static void LoadAndInitializeJIT(LPCWSTR pwzJitName, OUT HINSTANCE* phJit, OUT I
*phJit = NULL;
*ppICorJitCompiler = NULL;

HRESULT hr = E_FAIL;

PathString CoreClrFolderHolder;
bool havePath = false;

if (GetClrModulePathName(CoreClrFolderHolder))
if (pwzJitName == nullptr)
{
// Load JIT from next to CoreCLR binary
havePath = true;
pJitLoadData->jld_hr = E_FAIL;
LOG((LF_JIT, LL_FATALERROR, "LoadAndInitializeJIT: pwzJitName is null"));
return;
}

if (havePath && !CoreClrFolderHolder.IsEmpty())
HRESULT hr = E_FAIL;

if (ValidateJitName(pwzJitName))
{
SString::Iterator iter = CoreClrFolderHolder.End();
BOOL findSep = CoreClrFolderHolder.FindBack(iter, DIRECTORY_SEPARATOR_CHAR_W);
if (findSep)
// Load JIT from next to CoreCLR binary
PathString CoreClrFolderHolder;
if (GetClrModulePathName(CoreClrFolderHolder) && !CoreClrFolderHolder.IsEmpty())
{
SString sJitName(pwzJitName);
CoreClrFolderHolder.Replace(iter + 1, CoreClrFolderHolder.End() - (iter + 1), sJitName);

*phJit = CLRLoadLibrary(CoreClrFolderHolder.GetUnicode());
if (*phJit != NULL)
SString::Iterator iter = CoreClrFolderHolder.End();
BOOL findSep = CoreClrFolderHolder.FindBack(iter, DIRECTORY_SEPARATOR_CHAR_W);
if (findSep)
{
hr = S_OK;
SString sJitName(pwzJitName);
CoreClrFolderHolder.Replace(iter + 1, CoreClrFolderHolder.End() - (iter + 1), sJitName);

*phJit = CLRLoadLibrary(CoreClrFolderHolder.GetUnicode());
if (*phJit != NULL)
{
hr = S_OK;
}
}
}
}

else
{
LOG((LF_JIT, LL_FATALERROR, "LoadAndInitializeJIT: invalid characters in %S\n", pwzJitName));
}

if (SUCCEEDED(hr))
{
Expand Down Expand Up @@ -4367,7 +4406,17 @@ LPCWSTR ExecutionManager::GetJitName()
{
STANDARD_VM_CONTRACT;

return MAKEDLLNAME_W(W("clrjit"));
LPCWSTR pwzJitName = NULL;

// Try to obtain a name for the jit library from the env. variable
IfFailThrow(CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_JitName, const_cast<LPWSTR *>(&pwzJitName)));

if (NULL == pwzJitName)
{
pwzJitName = MAKEDLLNAME_W(W("clrjit"));
}

return pwzJitName;
}
#endif // !FEATURE_MERGE_JIT_AND_ENGINE

Expand Down
Loading