Skip to content

Commit

Permalink
Fix PMI non-determinism and a potential deadlock. (#252)
Browse files Browse the repository at this point in the history 
PMI in DRIVEALL mode invokes subprocesses in PREPALL mode
in order to be able to recover from errors. We had COMPlus environment
variables (including COMPlus_JitStdOutFile) set for both the parent and
child processes so the jit emitted disassembly, etc. to the same file.
The parent process was still active when the child was running (e.g.,
it called p.StandardOutput.ReadToEnd()). That resulted in duplicate
methods (especially from System.Private.Corelib) in the dasm file and
non-deterministic output interleaving.

The fix is not to set the COMPlus environment variables  in jit-dasm-pmi
when invoking pmi but instead pass them in PMIENV environment variable
as semicolon-separated name=value pairs and set the actual COMPlus
environment variables only on child processes.

I also fixed a potential deadlock in output/error redirection: at least
one of the redirected steams has to be read asynchronously.
See https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.process.standardoutput
for details.
  • Loading branch information
@erozenfeld
erozenfeld authored Feb 26, 2020
1 parent 0d7710a commit e209e16
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 23 deletions.
61 changes: 41 additions & 20 deletions src/jit-dasm-pmi/jit-dasm-pmi.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
using System.Collections.Generic;
using System.Reflection;
using System.Linq;
using System.Text;
using Microsoft.DotNet.Cli.Utils;
using Microsoft.DotNet.Tools.Common;

Expand Down Expand Up @@ -467,6 +468,22 @@ void AddEnvironmentVariable(string varName, string varValue)
Console.WriteLine("Setting: {0}={1}", varName, varValue);
}
}

StringBuilder pmiEnv = new StringBuilder();
// Append environment variable to the string that will be used as a value of PMIENV environment
// variable.
void AppendEnvironmentVariableToPmiEnv(string varName, string varValue)
{
if (pmiEnv.Length > 0)
{
pmiEnv.Append(";");
}
pmiEnv.Append(varName + "=" + varValue);
if (this.verbose)
{
Console.WriteLine("Appending: {0}={1} to PMIENV", varName, varValue);
}
}

try
{
Expand All @@ -485,53 +502,53 @@ void AddEnvironmentVariable(string varName, string varValue)
if (envVar.IndexOf("COMPlus_") == 0)
{
string value = Environment.GetEnvironmentVariable(envVar);
AddEnvironmentVariable(envVar, value);
AppendEnvironmentVariableToPmiEnv(envVar, value);
}
}

// Set up environment do PMI based disasm.
AddEnvironmentVariable("COMPlus_JitDisasm", "*");
AddEnvironmentVariable("COMPlus_JitDisasmAssemblies", Path.GetFileNameWithoutExtension(assembly.Name));
AddEnvironmentVariable("COMPlus_JitUnwindDump", "*");
AddEnvironmentVariable("COMPlus_JitEHDump", "*");
AddEnvironmentVariable("COMPlus_JitDiffableDasm", "1");
AddEnvironmentVariable("COMPlus_ReadyToRun", "0");
AddEnvironmentVariable("COMPlus_ZapDisable", "1");
AddEnvironmentVariable("COMPlus_JitEnableNoWayAssert", "1"); // Force noway_assert to generate assert (not fall back to MinOpts).
AddEnvironmentVariable("COMPlus_JitNoForceFallback", "1"); // Don't stress noway fallback path.
AddEnvironmentVariable("COMPlus_JitRequired", "1"); // Force NO_WAY to generate assert. Also generates assert for BADCODE/BADCODE3.
AppendEnvironmentVariableToPmiEnv("COMPlus_JitDisasm", "*");
AppendEnvironmentVariableToPmiEnv("COMPlus_JitDisasmAssemblies", Path.GetFileNameWithoutExtension(assembly.Name));
AppendEnvironmentVariableToPmiEnv("COMPlus_JitUnwindDump", "*");
AppendEnvironmentVariableToPmiEnv("COMPlus_JitEHDump", "*");
AppendEnvironmentVariableToPmiEnv("COMPlus_JitDiffableDasm", "1");
AppendEnvironmentVariableToPmiEnv("COMPlus_ReadyToRun", "0");
AppendEnvironmentVariableToPmiEnv("COMPlus_ZapDisable", "1");
AppendEnvironmentVariableToPmiEnv("COMPlus_JitEnableNoWayAssert", "1"); // Force noway_assert to generate assert (not fall back to MinOpts).
AppendEnvironmentVariableToPmiEnv("COMPlus_JitNoForceFallback", "1"); // Don't stress noway fallback path.
AppendEnvironmentVariableToPmiEnv("COMPlus_JitRequired", "1"); // Force NO_WAY to generate assert. Also generates assert for BADCODE/BADCODE3.

// We likely don't want tiering enabled, but allow it, if user wants tier0 codegen
AddEnvironmentVariable("COMPlus_TieredCompilation", _config.Tier0 ? "1" : "0");
AppendEnvironmentVariableToPmiEnv("COMPlus_TieredCompilation", _config.Tier0 ? "1" : "0");

if (_config.Tier0)
{
// jit all methods at tier0
AddEnvironmentVariable("COMPlus_TC_QuickJitForLoops", "1");
AppendEnvironmentVariableToPmiEnv("COMPlus_TC_QuickJitForLoops", "1");
// don't promote any method to tier1
AddEnvironmentVariable("COMPlus_TC_CallCounting", "0");
AppendEnvironmentVariableToPmiEnv("COMPlus_TC_CallCounting", "0");
}

if (this.doGCDump)
{
AddEnvironmentVariable("COMPlus_JitGCDump", "*");
AppendEnvironmentVariableToPmiEnv("COMPlus_JitGCDump", "*");
}

if (this.doDebugDump)
{
AddEnvironmentVariable("COMPlus_JitDebugDump", "*");
AppendEnvironmentVariableToPmiEnv("COMPlus_JitDebugDump", "*");
}

if (this._altjit != null)
{
AddEnvironmentVariable("COMPlus_AltJit", "*");
AddEnvironmentVariable("COMPlus_AltJitName", _altjit);
AppendEnvironmentVariableToPmiEnv("COMPlus_AltJit", "*");
AppendEnvironmentVariableToPmiEnv("COMPlus_AltJitName", _altjit);

// If this looks like a cross-targeting altjit, fix the SIMD size.
// Here's one place where rationalized jit naming would be nice.
if (_altjit.IndexOf("nonjit") > 0)
{
AddEnvironmentVariable("COMPlus_SIMD16ByteOnly", "1");
AppendEnvironmentVariableToPmiEnv("COMPlus_SIMD16ByteOnly", "1");
}
}

Expand All @@ -554,7 +571,9 @@ void AddEnvironmentVariable(string varName, string varValue)

PathUtility.EnsureParentDirectoryExists(dasmPath);

generateCmd.EnvironmentVariable("COMPlus_JitStdOutFile", dasmPath);
AppendEnvironmentVariableToPmiEnv("COMPlus_JitStdOutFile", dasmPath);

AddEnvironmentVariable("PMIENV", pmiEnv.ToString());

// Redirect stdout/stderr to log file and run command.
using (var outputStreamWriter = File.CreateText(logPath))
Expand Down Expand Up @@ -596,6 +615,8 @@ void AddEnvironmentVariable(string varName, string varValue)
}
else
{
AddEnvironmentVariable("PMIENV", pmiEnv.ToString());

// By default forward to output to stdout/stderr.
generateCmd.ForwardStdOut();
generateCmd.ForwardStdErr();
Expand Down
14 changes: 12 additions & 2 deletions src/pmi/PMIDriver.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.IO;
using System.Management;
using System.Text;
using System.Text.RegularExpressions;

// Drive PMI through jitting all the methods in an assembly.
//
Expand All @@ -22,7 +23,7 @@ class PMIDriver
const int PREPALL_TIMEOUT = 1800000; //30 minutes
const int PREPALL_MAX_RETRY_COUNT = 5;

public static int Drive(string assemblyName, bool verbose)
public static int Drive(string assemblyName, bool verbose, Dictionary<string, string> environment)
{
string assemblyPath = Path.GetFullPath(assemblyName);
string assemblyPathAsFile = Util.MapPathToFileName(assemblyPath);
Expand All @@ -41,6 +42,7 @@ public static int Drive(string assemblyName, bool verbose)
PrepAllInfo pi = new PrepAllInfo();
pi.assemblyName = assemblyName;
pi.methodToPrep = 0;
pi.environment = environment;

bool isFileCompleted = false;
int prepallRetryCount = 0;
Expand Down Expand Up @@ -165,6 +167,13 @@ private static PrepAllResult Compute(PrepAllInfo pi)
p.StartInfo.UseShellExecute = false;
p.StartInfo.RedirectStandardOutput = true;
p.StartInfo.RedirectStandardError = true;
p.ErrorDataReceived += new DataReceivedEventHandler((sender, e) =>
{ szError += e.Data; });

foreach (var pair in pi.environment)
{
p.StartInfo.EnvironmentVariables[pair.Key] = pair.Value;
}

// Fetch our command line. Split off the arguments.
#if NETCOREAPP
Expand All @@ -186,8 +195,8 @@ private static PrepAllResult Compute(PrepAllInfo pi)
Console.WriteLine($"DRIVEALL: Invoking {driverName} {newCommandLine}");

p.Start();
p.BeginErrorReadLine();
szOutput = p.StandardOutput.ReadToEnd();
szError = p.StandardError.ReadToEnd();

if (!p.WaitForExit(PREPALL_TIMEOUT))
{
Expand Down Expand Up @@ -311,6 +320,7 @@ struct PrepAllInfo
{
public string assemblyName;
public int methodToPrep;
public Dictionary<string, string> environment;

public override string ToString()
{
Expand Down
25 changes: 24 additions & 1 deletion src/pmi/pmi.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1313,6 +1313,9 @@ private static int Usage()
+ " for example: " + exeName + " PrepAll-Quiet-Time PATH_TO_ASSEMBLY\n"
+ "\n"
+ "Environment variable PMIPATH is a semicolon-separated list of paths used to find dependent assemblies.\n"
+ "Environment variable PMIENV is a semicolon-separated list of name=value pairs used to set the environment"
+ " when running pmi in a subprocess. This only affects DRIVEALL mode.\n"
+ " "
#if NETCOREAPP
+ "\n"
+ "PATH_TO_ASSEMBLY can optionally be a directory; if so the tool will process all .exe and .dll files\n"
Expand Down Expand Up @@ -1390,7 +1393,27 @@ public static int Main(string[] args)

if (rootCommand == "DRIVEALL")
{
return PMIDriver.PMIDriver.Drive(assemblyName, verbose);
string pmiEnv = Environment.GetEnvironmentVariable("PMIENV");
Dictionary<string, string> environment = new Dictionary<string, string>();
if ((pmiEnv != null) && (pmiEnv.Length != 0))
{
Regex rx = new Regex(@"^(?:(?<name>[\w_]+)=(?<value>[^;]+);)*(?<lastname>[\w_]+)=(?<lastvalue>[^$]+)$");
MatchCollection matches = rx.Matches(pmiEnv);
if (matches.Count != 1)
{
Console.WriteLine("ERROR: PMIENV format is incorrect");
return Usage();
}
Match match = matches[0];
GroupCollection groups = match.Groups;
for (int i = 0; i < groups["name"].Captures.Count; ++i)
{
environment[groups["name"].Captures[i].Value] = groups["value"].Captures[i].Value;
}
environment[groups["lastname"].Captures[0].Value] = groups["lastvalue"].Captures[0].Value;
}

return PMIDriver.PMIDriver.Drive(assemblyName, verbose, environment);
}

v = new Counter();
Expand Down

0 comments on commit e209e16

Please sign in to comment.