Skip to content

Commit 53bcbeb

Browse files
Fix issues with JitDumpFg (#68006)
1. Fix potential race condition flagged by an automated tool: use fopen with "wx" instead of "r" followed by "a+". 2. While in the code, I noticed it wasn't consistent about file naming, and failed on many different function names, so I fixed those issues as well.
1 parent 23c22ce commit 53bcbeb

File tree

1 file changed

+48
-35
lines changed

1 file changed

+48
-35
lines changed

src/coreclr/jit/fgdiagnostic.cpp

Lines changed: 48 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -217,9 +217,11 @@ struct escapeMapping_t
217217
// clang-format off
218218
static escapeMapping_t s_EscapeFileMapping[] =
219219
{
220-
{':', "="},
221-
{'<', "["},
222-
{'>', "]"},
220+
{' ', "_"},
221+
{':', "_"},
222+
{',', "_"},
223+
{'<', "~lt~"},
224+
{'>', "~gt~"},
223225
{';', "~semi~"},
224226
{'|', "~bar~"},
225227
{'&', "~amp~"},
@@ -431,7 +433,7 @@ void Compiler::fgDumpTree(FILE* fgxFile, GenTree* const tree)
431433
// Return Value:
432434
// Opens a file to which a flowgraph can be dumped, whose name is based on the current
433435
// config vales.
434-
436+
//
435437
FILE* Compiler::fgOpenFlowGraphFile(bool* wbDontClose, Phases phase, PhasePosition pos, LPCWSTR type)
436438
{
437439
FILE* fgxFile;
@@ -441,7 +443,6 @@ FILE* Compiler::fgOpenFlowGraphFile(bool* wbDontClose, Phases phase, PhasePositi
441443
LPCWSTR filename = nullptr;
442444
LPCWSTR pathname = nullptr;
443445
const char* escapedString;
444-
bool createDuplicateFgxFiles = true;
445446

446447
if (fgBBcount <= 1)
447448
{
@@ -525,7 +526,6 @@ FILE* Compiler::fgOpenFlowGraphFile(bool* wbDontClose, Phases phase, PhasePositi
525526
{
526527
if (fgFirstBB->hasProfileWeight())
527528
{
528-
createDuplicateFgxFiles = true;
529529
goto ONE_FILE_PER_METHOD;
530530
}
531531
else
@@ -536,9 +536,7 @@ FILE* Compiler::fgOpenFlowGraphFile(bool* wbDontClose, Phases phase, PhasePositi
536536
if (wcscmp(filename, W("hot")) == 0)
537537
{
538538
if (info.compMethodInfo->regionKind == CORINFO_REGION_HOT)
539-
540539
{
541-
createDuplicateFgxFiles = true;
542540
goto ONE_FILE_PER_METHOD;
543541
}
544542
else
@@ -550,7 +548,6 @@ FILE* Compiler::fgOpenFlowGraphFile(bool* wbDontClose, Phases phase, PhasePositi
550548
{
551549
if (info.compMethodInfo->regionKind == CORINFO_REGION_COLD)
552550
{
553-
createDuplicateFgxFiles = true;
554551
goto ONE_FILE_PER_METHOD;
555552
}
556553
else
@@ -562,7 +559,6 @@ FILE* Compiler::fgOpenFlowGraphFile(bool* wbDontClose, Phases phase, PhasePositi
562559
{
563560
if (info.compMethodInfo->regionKind == CORINFO_REGION_JIT)
564561
{
565-
createDuplicateFgxFiles = true;
566562
goto ONE_FILE_PER_METHOD;
567563
}
568564
else
@@ -572,15 +568,38 @@ FILE* Compiler::fgOpenFlowGraphFile(bool* wbDontClose, Phases phase, PhasePositi
572568
}
573569
else if (wcscmp(filename, W("all")) == 0)
574570
{
575-
createDuplicateFgxFiles = true;
576571

577572
ONE_FILE_PER_METHOD:;
578573

579-
escapedString = fgProcessEscapes(info.compFullName, s_EscapeFileMapping);
574+
#define FILENAME_PATTERN W("%S-%S-%s-%S.%s")
575+
#define FILENAME_PATTERN_WITH_NUMBER W("%S-%S-%s-%S~%d.%s")
576+
577+
const size_t MaxFileNameLength = MAX_PATH_FNAME - 20 /* give us some extra buffer */;
578+
579+
escapedString = fgProcessEscapes(info.compFullName, s_EscapeFileMapping);
580+
size_t escapedStringLen = strlen(escapedString);
581+
582+
static const char* phasePositionStrings[] = {"pre", "post"};
583+
assert((unsigned)pos < ArrLen(phasePositionStrings));
584+
const char* phasePositionString = phasePositionStrings[(unsigned)pos];
585+
const size_t phasePositionStringLen = strlen(phasePositionString);
586+
const char* tierName = compGetTieringName(true);
587+
size_t wCharCount = escapedStringLen + 1 + strlen(phasePositionString) + 1 + wcslen(phaseName) + 1 +
588+
strlen(tierName) + strlen("~999") + 1 + wcslen(type) + 1;
589+
590+
if (wCharCount > MaxFileNameLength)
591+
{
592+
// Crop the escapedString.
593+
wCharCount -= escapedStringLen;
594+
size_t newEscapedStringLen = MaxFileNameLength - wCharCount;
595+
char* newEscapedString = getAllocator(CMK_DebugOnly).allocate<char>(newEscapedStringLen + 1);
596+
strncpy_s(newEscapedString, newEscapedStringLen + 1, escapedString, newEscapedStringLen);
597+
newEscapedString[newEscapedStringLen] = '\0';
598+
escapedString = newEscapedString;
599+
escapedStringLen = newEscapedStringLen;
600+
wCharCount += escapedStringLen;
601+
}
580602

581-
const char* tierName = compGetTieringName(true);
582-
size_t wCharCount =
583-
strlen(escapedString) + wcslen(phaseName) + 1 + strlen("~999") + wcslen(type) + strlen(tierName) + 1;
584603
if (pathname != nullptr)
585604
{
586605
wCharCount += wcslen(pathname) + 1;
@@ -589,48 +608,42 @@ FILE* Compiler::fgOpenFlowGraphFile(bool* wbDontClose, Phases phase, PhasePositi
589608

590609
if (pathname != nullptr)
591610
{
592-
swprintf_s((LPWSTR)filename, wCharCount, W("%s\\%S-%s-%S.%s"), pathname, escapedString, phaseName, tierName,
593-
type);
611+
swprintf_s((LPWSTR)filename, wCharCount, W("%s\\") FILENAME_PATTERN, pathname, escapedString,
612+
phasePositionString, phaseName, tierName, type);
594613
}
595614
else
596615
{
597-
swprintf_s((LPWSTR)filename, wCharCount, W("%S.%s"), escapedString, type);
616+
swprintf_s((LPWSTR)filename, wCharCount, FILENAME_PATTERN, escapedString, phasePositionString, phaseName,
617+
tierName, type);
598618
}
599-
fgxFile = _wfopen(filename, W("r")); // Check if this file already exists
600-
if (fgxFile != nullptr)
619+
fgxFile = _wfopen(filename, W("wx")); // Open the file for writing only only if it doesn't already exist
620+
if (fgxFile == nullptr)
601621
{
602-
// For Generic methods we will have both hot and cold versions
603-
if (createDuplicateFgxFiles == false)
604-
{
605-
fclose(fgxFile);
606-
return nullptr;
607-
}
608-
// Yes, this filename already exists, so create a different one by appending ~2, ~3, etc...
622+
// This filename already exists, so create a different one by appending ~2, ~3, etc...
609623
for (int i = 2; i < 1000; i++)
610624
{
611-
fclose(fgxFile);
612625
if (pathname != nullptr)
613626
{
614-
swprintf_s((LPWSTR)filename, wCharCount, W("%s\\%S~%d.%s"), pathname, escapedString, i, type);
627+
swprintf_s((LPWSTR)filename, wCharCount, W("%s\\") FILENAME_PATTERN_WITH_NUMBER, pathname,
628+
escapedString, phasePositionString, phaseName, tierName, i, type);
615629
}
616630
else
617631
{
618-
swprintf_s((LPWSTR)filename, wCharCount, W("%S~%d.%s"), escapedString, i, type);
632+
swprintf_s((LPWSTR)filename, wCharCount, FILENAME_PATTERN_WITH_NUMBER, escapedString,
633+
phasePositionString, phaseName, tierName, i, type);
619634
}
620-
fgxFile = _wfopen(filename, W("r")); // Check if this file exists
621-
if (fgxFile == nullptr)
635+
fgxFile = _wfopen(filename, W("wx")); // Open the file for writing only only if it doesn't already exist
636+
if (fgxFile != nullptr)
622637
{
623638
break;
624639
}
625640
}
626641
// If we have already created 1000 files with this name then just fail
627-
if (fgxFile != nullptr)
642+
if (fgxFile == nullptr)
628643
{
629-
fclose(fgxFile);
630644
return nullptr;
631645
}
632646
}
633-
fgxFile = _wfopen(filename, W("a+"));
634647
*wbDontClose = false;
635648
}
636649
else if (wcscmp(filename, W("stdout")) == 0)

0 commit comments

Comments
 (0)