From 9cc2520ace2503068559c88ef73c3c5fd7214bc9 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 11 Mar 2021 15:55:05 -0800 Subject: [PATCH 1/5] Allow jit to disregard PGO; PGO changes for SPMI and MCS * COMPlus_JitDisablePGO will now block the jit from using profile data. Useful for investigations and (someday) bug triage. * `-jitoption` now works for `superpmi.py asmdiffs`. It only applies to the base jit. * Limit SPMI file size path component to 50 chars, so that SPMI can handle being run in directories with long paths we can't control (as happens with tools like `crank`). * Enhance `mcs -jitflags` output to describe how many method contexts have PGO data, and which kinds of data they hold. Useful for validating that an SPMI collection done via dynamic PGO has actually collected an interesting set of jit requests. --- .../ToolBox/superpmi/mcs/verbjitflags.cpp | 20 ++++++++++ .../superpmi-shared/methodcontext.cpp | 38 +++++++++++++++++++ .../superpmi/superpmi-shared/methodcontext.h | 11 ++++++ .../superpmi-shared/spmidumphelper.cpp | 6 +++ .../superpmi/superpmi-shared/spmiutil.cpp | 2 +- src/coreclr/jit/codegencommon.cpp | 4 +- src/coreclr/jit/compiler.cpp | 33 ++++++++++------ src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/jitconfigvalues.h | 3 ++ src/coreclr/scripts/superpmi.py | 12 ++++-- 10 files changed, 113 insertions(+), 18 deletions(-) diff --git a/src/coreclr/ToolBox/superpmi/mcs/verbjitflags.cpp b/src/coreclr/ToolBox/superpmi/mcs/verbjitflags.cpp index f5f9d7786a745a..a40707846887de 100644 --- a/src/coreclr/ToolBox/superpmi/mcs/verbjitflags.cpp +++ b/src/coreclr/ToolBox/superpmi/mcs/verbjitflags.cpp @@ -24,6 +24,26 @@ int verbJitFlags::DoWork(const char* nameOfInput) mc->repGetJitFlags(&corJitFlags, sizeof(corJitFlags)); unsigned long long rawFlags = corJitFlags.GetFlagsRaw(); + // We co-opt unused flag bits to note if there's pgo data, + // and if so, what kind + // + bool hasEdgeProfile = false; + bool hasClassProfile = false; + if (mc->hasPgoData(hasEdgeProfile, hasClassProfile)) + { + rawFlags |= 1ULL << (EXTRA_JIT_FLAGS::HAS_PGO); + + if (hasEdgeProfile) + { + rawFlags |= 1ULL << (EXTRA_JIT_FLAGS::HAS_EDGE_PROFILE); + } + + if (hasClassProfile) + { + rawFlags |= 1ULL << (EXTRA_JIT_FLAGS::HAS_CLASS_PROFILE); + } + } + int index = flagMap.GetIndex(rawFlags); if (index == -1) { diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.cpp b/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.cpp index 0a7da7e4b56c2b..51faa23a60702b 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.cpp @@ -6765,6 +6765,44 @@ int MethodContext::dumpMD5HashToBuffer(BYTE* pBuffer, int bufLen, char* hash, in return m_hash.HashBuffer(pBuffer, bufLen, hash, hashLen); } +bool MethodContext::hasPgoData(bool& hasEdgeProfile, bool& hasClassProfile) +{ + hasEdgeProfile = false; + hasClassProfile = false; + + // Obtain the Method Info structure for this method + CORINFO_METHOD_INFO info; + unsigned flags = 0; + repCompileMethod(&info, &flags); + + if ((GetPgoInstrumentationResults != nullptr) && + (GetPgoInstrumentationResults->GetIndex(CastHandle(info.ftn)) != -1)) + { + ICorJitInfo::PgoInstrumentationSchema* schema = nullptr; + UINT32 schemaCount = 0; + BYTE* schemaData = nullptr; + HRESULT pgoHR = repGetPgoInstrumentationResults(info.ftn, &schema, &schemaCount, &schemaData); + + if (SUCCEEDED(pgoHR)) + { + for (UINT32 i = 0; i < schemaCount; i++) + { + hasEdgeProfile |= (schema[i].InstrumentationKind == ICorJitInfo::PgoInstrumentationKind::EdgeIntCount); + hasClassProfile |= (schema[i].InstrumentationKind == ICorJitInfo::PgoInstrumentationKind::TypeHandleHistogramCount); + + if (hasEdgeProfile && hasClassProfile) + { + break; + } + } + + return true; + } + } + + return false; +} + MethodContext::Environment MethodContext::cloneEnvironment() { MethodContext::Environment env; diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h b/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h index 4851d369669537..23781e66c13b9d 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h +++ b/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h @@ -43,6 +43,15 @@ const char* toString(CorInfoType cit); #define METHOD_IDENTITY_INFO_SIZE 0x10000 // We assume that the METHOD_IDENTITY_INFO_SIZE will not exceed 64KB +// Special "jit flags" for noting some method context features + +enum EXTRA_JIT_FLAGS +{ + HAS_PGO = 63, + HAS_EDGE_PROFILE = 62, + HAS_CLASS_PROFILE = 61 +}; + class MethodContext { public: @@ -82,6 +91,8 @@ class MethodContext int dumpMethodIdentityInfoToBuffer(char* buff, int len, bool ignoreMethodName = false, CORINFO_METHOD_INFO* optInfo = nullptr, unsigned optFlags = 0); int dumpMethodMD5HashToBuffer(char* buff, int len, bool ignoreMethodName = false, CORINFO_METHOD_INFO* optInfo = nullptr, unsigned optFlags = 0); + bool hasPgoData(bool& hasEdgeProfile, bool& hasClassProfile); + void recGlobalContext(const MethodContext& other); void dmpEnvironment(DWORD key, const Agnostic_Environment& value); diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shared/spmidumphelper.cpp b/src/coreclr/ToolBox/superpmi/superpmi-shared/spmidumphelper.cpp index 3530d369507ecd..da63826e750b6f 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi-shared/spmidumphelper.cpp +++ b/src/coreclr/ToolBox/superpmi/superpmi-shared/spmidumphelper.cpp @@ -279,6 +279,12 @@ std::string SpmiDumpHelper::DumpJitFlags(unsigned long long flags) AddFlag(NO_INLINING); + // "Extra jit flag" support + // + AddFlagNumeric(HAS_PGO, EXTRA_JIT_FLAGS::HAS_PGO); + AddFlagNumeric(HAS_EDGE_PROFILE, EXTRA_JIT_FLAGS::HAS_EDGE_PROFILE); + AddFlagNumeric(HAS_CLASS_PROFILE, EXTRA_JIT_FLAGS::HAS_CLASS_PROFILE); + #undef AddFlag #undef AddFlagNumeric diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp b/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp index 0d52ae35ed1ab1..1854d95ca7eb87 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp +++ b/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp @@ -159,7 +159,7 @@ WCHAR* GetResultFileName(const WCHAR* folderPath, const WCHAR* fileName, const W const size_t folderPathLength = wcslen(folderPath); const size_t fileNameLength = wcslen(fileName); const size_t extensionLength = wcslen(extension); - const size_t maxPathLength = MAX_PATH - 50; // subtract 50 because excel doesn't like paths longer then 230. + const size_t maxPathLength = 50; const size_t randomStringLength = 8; size_t fullPathLength = folderPathLength + 1 + extensionLength; diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 75f038612b3c2c..aefc5006b8f4bf 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -2207,9 +2207,9 @@ void CodeGen::genGenerateMachineCode() compiler->fgHaveValidEdgeWeights ? "valid" : "invalid", compiler->fgCalledCount); } - if (compiler->fgProfileData_ILSizeMismatch) + if (compiler->fgPgoFailReason != nullptr) { - printf("; discarded IBC profile data due to mismatch in ILSize\n"); + printf("; %s\n", compiler->fgPgoFailReason); } if (compiler->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_ALT_JIT)) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 7a8fb916abfc18..aa8027e39827df 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -2876,11 +2876,12 @@ void Compiler::compInitOptions(JitFlags* jitFlags) // Profile data // - fgPgoSchema = nullptr; - fgPgoData = nullptr; - fgPgoSchemaCount = 0; - fgPgoQueryResult = E_FAIL; - fgProfileData_ILSizeMismatch = false; + fgPgoSchema = nullptr; + fgPgoData = nullptr; + fgPgoSchemaCount = 0; + fgPgoQueryResult = E_FAIL; + fgPgoFailReason = nullptr; + if (jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT)) { fgPgoQueryResult = info.compCompHnd->getPgoInstrumentationResults(info.compMethodHnd, &fgPgoSchema, @@ -2892,12 +2893,22 @@ void Compiler::compInitOptions(JitFlags* jitFlags) // // We will discard the IBC data in this case // - if (FAILED(fgPgoQueryResult) && (fgPgoSchema != nullptr)) + if (FAILED(fgPgoQueryResult)) + { + fgPgoFailReason = (fgPgoSchema != nullptr) ? "No matching PGO data" : "No PGO data"; + fgPgoData = nullptr; + fgPgoSchema = nullptr; + } + // Optionally, discard the profile data. + // + else if (JitConfig.JitDisablePGO() == 1) { - fgProfileData_ILSizeMismatch = true; - fgPgoData = nullptr; - fgPgoSchema = nullptr; + fgPgoFailReason = "PGO data available, but JitDisablePGO == 1"; + fgPgoQueryResult = E_FAIL; + fgPgoData = nullptr; + fgPgoSchema = nullptr; } + #ifdef DEBUG // A successful result implies a non-NULL fgPgoSchema // @@ -3389,9 +3400,9 @@ void Compiler::compInitOptions(JitFlags* jitFlags) printf("OPTIONS: optimized using profile data\n"); } - if (fgProfileData_ILSizeMismatch) + if (fgPgoFailReason != nullptr) { - printf("OPTIONS: discarded IBC profile data due to mismatch in ILSize\n"); + printf("OPTIONS: %s\n", fgPgoFailReason); } if (jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT)) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 64635e15cb2960..15cef06c532a5c 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5584,7 +5584,7 @@ class Compiler void fgIncorporateEdgeCounts(); public: - bool fgProfileData_ILSizeMismatch; + const char* fgPgoFailReason; ICorJitInfo::PgoInstrumentationSchema* fgPgoSchema; BYTE* fgPgoData; UINT32 fgPgoSchemaCount; diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 478aa4d8d0bd87..c54e1a7f6fadb1 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -458,6 +458,9 @@ CONFIG_INTEGER(JitMinimalPrejitProfiling, W("JitMinimalPrejitProfiling"), 0) CONFIG_INTEGER(JitClassProfiling, W("JitClassProfiling"), 1) CONFIG_INTEGER(JitEdgeProfiling, W("JitEdgeProfiling"), 1) +// Profile consumption options +CONFIG_INTEGER(JitDisablePGO, W("JitDisablePGO"), 0) // Ignore pgo data + // Control when Virtual Calls are expanded CONFIG_INTEGER(JitExpandCallsEarly, W("JitExpandCallsEarly"), 1) // Expand Call targets early (in the global morph // phase) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 6909171afa31f5..737960dd69ce56 100755 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1722,6 +1722,11 @@ def replay_with_asm_diffs(self): altjit_asm_diffs_flags = target_flags altjit_replay_flags = target_flags + option_flags = [] + if self.coreclr_args.jitoption: + for o in self.coreclr_args.jitoption: + option_flags += "-jitoption", o + if self.coreclr_args.altjit: altjit_asm_diffs_flags += [ "-jitoption", "force", "AltJit=*", @@ -1771,6 +1776,7 @@ def replay_with_asm_diffs(self): "-r", os.path.join(temp_location, "repro") # Repro name, create .mc repro files ] flags += altjit_asm_diffs_flags + flags += option_flags if not self.coreclr_args.sequential: flags += [ "-p" ] @@ -1859,7 +1865,7 @@ async def create_replay_artifacts(print_prefix, item, self, mch_file, env_vars, # as the LoadLibrary path will be relative to the current directory. with ChangeDir(self.coreclr_args.core_root): - async def create_one_artifact(jit_path: str, location: str) -> str: + async def create_one_artifact(jit_path: str, location: str, flags: list[str]) -> str: command = [self.superpmi_path] + flags + [jit_path, mch_file] item_path = os.path.join(location, "{}{}".format(item, extension)) with open(item_path, 'w') as file_handle: @@ -1872,8 +1878,8 @@ async def create_one_artifact(jit_path: str, location: str) -> str: return generated_txt # Generate diff and base JIT dumps - base_txt = await create_one_artifact(self.base_jit_path, base_location) - diff_txt = await create_one_artifact(self.diff_jit_path, diff_location) + base_txt = await create_one_artifact(self.base_jit_path, base_location, flags + option_flags) + diff_txt = await create_one_artifact(self.diff_jit_path, diff_location, flags) if base_txt != diff_txt: jit_differences_queue.put_nowait(item) From 46f4e369adf043b41bbc245f1976b3456d8e272e Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 13 Mar 2021 08:35:54 -0800 Subject: [PATCH 2/5] review feedback; also disable GDV if we're disabling PGO --- .../superpmi/superpmi-shared/methodcontext.h | 6 ++++ src/coreclr/jit/compiler.cpp | 4 +-- src/coreclr/jit/importer.cpp | 9 +++++ src/coreclr/scripts/superpmi.py | 33 +++++++++++++++---- 4 files changed, 44 insertions(+), 8 deletions(-) diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h b/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h index 23781e66c13b9d..053e3424311790 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h +++ b/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h @@ -52,6 +52,12 @@ enum EXTRA_JIT_FLAGS HAS_CLASS_PROFILE = 61 }; +// Asserts to catch changes in corjit flags definitions. + +static_assert(EXTRA_JIT_FLAGS::HAS_PGO == CORJIT_FLAGS::CorJitFlag::CORJIT_FLAG_UNUSED36, "Jit Flags Mismatch"); +static_assert(EXTRA_JIT_FLAGS::HAS_EDGE_PROFILE == CORJIT_FLAGS::CorJitFlag::CORJIT_FLAG_UNUSED35, "Jit Flags Mismatch"); +static_assert(EXTRA_JIT_FLAGS::HAS_CLASS_PROFILE == CORJIT_FLAGS::CorJitFlag::CORJIT_FLAG_UNUSED34, "Jit Flags Mismatch"); + class MethodContext { public: diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index aa8027e39827df..ab111747a54b49 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -2901,9 +2901,9 @@ void Compiler::compInitOptions(JitFlags* jitFlags) } // Optionally, discard the profile data. // - else if (JitConfig.JitDisablePGO() == 1) + else if (JitConfig.JitDisablePGO() != 0) { - fgPgoFailReason = "PGO data available, but JitDisablePGO == 1"; + fgPgoFailReason = "PGO data available, but JitDisablePGO != 0"; fgPgoQueryResult = E_FAIL; fgPgoData = nullptr; fgPgoSchema = nullptr; diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 0b083746808c43..6dffb4458d3359 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -21504,6 +21504,15 @@ void Compiler::considerGuardedDevirtualization( JITDUMP("Considering guarded devirtualization\n"); + // We currently only get likely class guesses when there is PGO data. So if we've disabled + // PGO, just bail out. + + if (JitConfig.JitDisablePGO() != 0) + { + JITDUMP("Not guessing for class; pgo disabled\n"); + return; + } + // See if there's a likely guess for the class. // const unsigned likelihoodThreshold = isInterface ? 25 : 30; diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 737960dd69ce56..3f0da667005e52 100755 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -294,6 +294,8 @@ asm_diff_parser.add_argument("--diff_jit_dump", action="store_true", help="Generate JitDump output for diffs. Default: only generate asm, not JitDump.") asm_diff_parser.add_argument("-temp_dir", help="Specify a temporary directory used for a previous ASM diffs run (for which --skip_cleanup was used) to view the results. The replay command is skipped.") asm_diff_parser.add_argument("--gcinfo", action="store_true", help="Include GC info in disassembly (sets COMPlus_JitGCDump/COMPlus_NgenGCDump; requires instructions to be prefixed by offsets).") +asm_diff_parser.add_argument("-base_jit_option", action="append", help="Option to pass to the baselne JIT. Format is key=value, where key is the option name without leading COMPlus_...") +asm_diff_parser.add_argument("-diff_jit_option", action="append", help="Option to pass to the diff JIT. Format is key=value, where key is the option name without leading COMPlus_...") # subparser for upload upload_parser = subparsers.add_parser("upload", description=upload_description, parents=[core_root_parser, target_parser]) @@ -1722,10 +1724,18 @@ def replay_with_asm_diffs(self): altjit_asm_diffs_flags = target_flags altjit_replay_flags = target_flags - option_flags = [] if self.coreclr_args.jitoption: - for o in self.coreclr_args.jitoption: - option_flags += "-jitoption", o + logging.warning("Ignoring -jitoption; use -base_jit_option or -diff_jit_option instead"); + + base_option_flags = [] + if self.coreclr_args.base_jit_option: + for o in self.coreclr_args.base_jit_option: + base_option_flags += "-jitoption", o + + diff_option_flags = [] + if self.coreclr_args.diff_jit_option: + for o in self.coreclr_args.diff_jit_option: + diff_option_flags += "-jitoption2", o if self.coreclr_args.altjit: altjit_asm_diffs_flags += [ @@ -1776,7 +1786,8 @@ def replay_with_asm_diffs(self): "-r", os.path.join(temp_location, "repro") # Repro name, create .mc repro files ] flags += altjit_asm_diffs_flags - flags += option_flags + flags += base_option_flags + flags += diff_option_flags if not self.coreclr_args.sequential: flags += [ "-p" ] @@ -1878,8 +1889,8 @@ async def create_one_artifact(jit_path: str, location: str, flags: list[str]) -> return generated_txt # Generate diff and base JIT dumps - base_txt = await create_one_artifact(self.base_jit_path, base_location, flags + option_flags) - diff_txt = await create_one_artifact(self.diff_jit_path, diff_location, flags) + base_txt = await create_one_artifact(self.base_jit_path, base_location, flags + base_option_flags) + diff_txt = await create_one_artifact(self.diff_jit_path, diff_location, flags + diff_option_flags) if base_txt != diff_txt: jit_differences_queue.put_nowait(item) @@ -3437,6 +3448,16 @@ def verify_replay_common_args(): lambda unused: True, "Unable to set diff_jit_dump.") + coreclr_args.verify(args, + "base_jit_option", + lambda unused: True, + "Unable to set base_jit_option.") + + coreclr_args.verify(args, + "diff_jit_option", + lambda unused: True, + "Unable to set diff_jit_option.") + process_base_jit_path_arg(coreclr_args) jit_in_product_location = False From e3b2c06d9b6440a9b4ab5bf1c093f2d73ecd9324 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 13 Mar 2021 09:17:37 -0800 Subject: [PATCH 3/5] fix build issue with clang --- .../ToolBox/superpmi/superpmi-shared/methodcontext.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h b/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h index 053e3424311790..d71f8d35f3b332 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h +++ b/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h @@ -54,9 +54,9 @@ enum EXTRA_JIT_FLAGS // Asserts to catch changes in corjit flags definitions. -static_assert(EXTRA_JIT_FLAGS::HAS_PGO == CORJIT_FLAGS::CorJitFlag::CORJIT_FLAG_UNUSED36, "Jit Flags Mismatch"); -static_assert(EXTRA_JIT_FLAGS::HAS_EDGE_PROFILE == CORJIT_FLAGS::CorJitFlag::CORJIT_FLAG_UNUSED35, "Jit Flags Mismatch"); -static_assert(EXTRA_JIT_FLAGS::HAS_CLASS_PROFILE == CORJIT_FLAGS::CorJitFlag::CORJIT_FLAG_UNUSED34, "Jit Flags Mismatch"); +static_assert((int)EXTRA_JIT_FLAGS::HAS_PGO == (int)CORJIT_FLAGS::CorJitFlag::CORJIT_FLAG_UNUSED36, "Jit Flags Mismatch"); +static_assert((int)EXTRA_JIT_FLAGS::HAS_EDGE_PROFILE == (int)CORJIT_FLAGS::CorJitFlag::CORJIT_FLAG_UNUSED35, "Jit Flags Mismatch"); +static_assert((int)EXTRA_JIT_FLAGS::HAS_CLASS_PROFILE == (int)CORJIT_FLAGS::CorJitFlag::CORJIT_FLAG_UNUSED34, "Jit Flags Mismatch"); class MethodContext { From 05501b663a1bc1aefda279d97f716b0782d377ac Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 15 Mar 2021 11:58:08 -0700 Subject: [PATCH 4/5] rework options handling for asmdiffs --- src/coreclr/scripts/superpmi.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 3f0da667005e52..f6a37a0e841a09 100755 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1731,11 +1731,14 @@ def replay_with_asm_diffs(self): if self.coreclr_args.base_jit_option: for o in self.coreclr_args.base_jit_option: base_option_flags += "-jitoption", o + base_option_flags_for_diff_artifact = base_option_flags diff_option_flags = [] + diff_option_flags_for_diff_artifact = [] if self.coreclr_args.diff_jit_option: for o in self.coreclr_args.diff_jit_option: - diff_option_flags += "-jitoption2", o + diff_option_flags += "-jit2option", o + diff_option_flags_for_diff_artifact += "-jitoption", o if self.coreclr_args.altjit: altjit_asm_diffs_flags += [ @@ -1889,8 +1892,8 @@ async def create_one_artifact(jit_path: str, location: str, flags: list[str]) -> return generated_txt # Generate diff and base JIT dumps - base_txt = await create_one_artifact(self.base_jit_path, base_location, flags + base_option_flags) - diff_txt = await create_one_artifact(self.diff_jit_path, diff_location, flags + diff_option_flags) + base_txt = await create_one_artifact(self.base_jit_path, base_location, flags + base_option_flags_for_diff_artifact) + diff_txt = await create_one_artifact(self.diff_jit_path, diff_location, flags + diff_option_flags_for_diff_artifact) if base_txt != diff_txt: jit_differences_queue.put_nowait(item) From 2c6a24bd19a757ae073d5045db4ef7666e702c86 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 16 Mar 2021 20:10:04 -0700 Subject: [PATCH 5/5] revise file name creation --- .../superpmi/superpmi-shared/spmiutil.cpp | 92 +++++++++++-------- 1 file changed, 56 insertions(+), 36 deletions(-) diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp b/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp index 1854d95ca7eb87..efbe6f13e1cd17 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp +++ b/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp @@ -156,58 +156,76 @@ void ReplaceIllegalCharacters(WCHAR* fileName) // All lengths in this function exclude the terminal NULL. WCHAR* GetResultFileName(const WCHAR* folderPath, const WCHAR* fileName, const WCHAR* extension) { - const size_t folderPathLength = wcslen(folderPath); - const size_t fileNameLength = wcslen(fileName); const size_t extensionLength = wcslen(extension); - const size_t maxPathLength = 50; + const size_t fileNameLength = wcslen(fileName); const size_t randomStringLength = 8; + const size_t maxPathLength = MAX_PATH - 50; + bool appendRandomString = false; - size_t fullPathLength = folderPathLength + 1 + extensionLength; - bool appendRandomString = false; + // See how long the folder part is, and start building the file path with the folder part. + // + WCHAR* fullPath = new WCHAR[MAX_PATH]; + fullPath[0] = W('\0'); + const size_t folderPathLength = GetFullPathNameW(folderPath, MAX_PATH, (LPWSTR)fullPath, NULL); - if (fileNameLength > 0) - { - fullPathLength += fileNameLength; - } - else + if (folderPathLength == 0) { - fullPathLength += randomStringLength; - appendRandomString = true; + LogError("GetResultFileName - can't resolve folder path '%ws'", folderPath); + return nullptr; } - size_t charsToDelete = 0; + // Account for the folder, directory separator and extension. + // + size_t fullPathLength = folderPathLength + 1 + extensionLength; - if (fullPathLength > maxPathLength) + // If we won't have room for a minimal file name part, bail. + // + if ((fullPathLength + randomStringLength) > maxPathLength) { - // The path name is too long; creating the file will fail. This can happen because we use the command line, - // which for ngen includes lots of environment variables, for example. - // Shorten the file name and add a random string to the end to avoid collisions. + LogError("GetResultFileName - folder path '%ws' length + minimal file name exceeds limit %d", fullPath, maxPathLength); + return nullptr; + } - charsToDelete = fullPathLength - maxPathLength + randomStringLength; + // Now figure out the file name part. + // + const size_t maxFileNameLength = maxPathLength - fullPathLength; + size_t usableFileNameLength = 0; - if (fileNameLength >= charsToDelete) - { - appendRandomString = true; - fullPathLength = maxPathLength; - } - else - { - LogError("GetResultFileName - path to the output file is too long '%ws\\%ws.%ws(%d)'", folderPath, fileName, extension, fullPathLength); - return nullptr; - } + if (fileNameLength == 0) + { + // No file name provided. Use random string. + // + fullPathLength += randomStringLength; + appendRandomString = true; + } + else if (fileNameLength < maxFileNameLength) + { + // Reasonable length file name, use as is. + // + usableFileNameLength = fileNameLength; + fullPathLength += fileNameLength; + appendRandomString = false; + } + else + { + // Overly long file name, truncate and add random string. + // + usableFileNameLength = maxFileNameLength - randomStringLength; + fullPathLength += maxFileNameLength; + appendRandomString = true; } - WCHAR* fullPath = new WCHAR[fullPathLength + 1]; - fullPath[0] = W('\0'); - wcsncat_s(fullPath, fullPathLength + 1, folderPath, folderPathLength); + // Append the file name part + // wcsncat_s(fullPath, fullPathLength + 1, DIRECTORY_SEPARATOR_STR_W, 1); + wcsncat_s(fullPath, fullPathLength + 1, fileName, usableFileNameLength); - if (fileNameLength > charsToDelete) - { - wcsncat_s(fullPath, fullPathLength + 1, fileName, fileNameLength - charsToDelete); - ReplaceIllegalCharacters(fullPath + folderPathLength + 1); - } + // Clean up anything in the file part that can't be in a file name. + // + ReplaceIllegalCharacters(fullPath + folderPathLength + 1); + // Append random string, if we're using it. + // if (appendRandomString) { unsigned randomNumber = 0; @@ -223,6 +241,8 @@ WCHAR* GetResultFileName(const WCHAR* folderPath, const WCHAR* fileName, const W wcsncat_s(fullPath, fullPathLength + 1, randomString, randomStringLength); } + // Append extension + // wcsncat_s(fullPath, fullPathLength + 1, extension, extensionLength); return fullPath;