Skip to content

Commit

Permalink
Emitted pdb path should respect PathMap
Browse files Browse the repository at this point in the history
The PDB path which is written into the PE file should respect the
`/pathmap` option passed to the compiler.  This is necessary to ensure
that PEs can be deterministic when built from different source paths.

The feature flag `pdb-path-determinism` is being kept for the time
being.  Even though it's a feature flag it still seems inappropriate to
break customers in a point release.  Deferring the removal until the
next major release to give customers time to react.  This issue tracks
removing the flag:

#19592

closes #9813
  • Loading branch information
jaredpar committed May 18, 2017
1 parent 8bc010c commit b66f68c
Show file tree
Hide file tree
Showing 10 changed files with 206 additions and 67 deletions.
2 changes: 1 addition & 1 deletion build/scripts/test-determinism.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ function Run-Build() {
}

Write-Host "Building the Solution"
Exec-Command $msbuild "/nologo /v:m /nodeReuse:false /m /p:DebugDeterminism=true /p:BootstrapBuildPath=$script:bootstrapDir /p:Features=`"debug-determinism;pdb-path-determinism`" /p:UseRoslynAnalyzers=false $pathMapBuildOption $sln"
Exec-Command $msbuild "/nologo /v:m /nodeReuse:false /m /p:DebugDeterminism=true /p:BootstrapBuildPath=$script:bootstrapDir /p:Features=`"debug-determinism`" /p:UseRoslynAnalyzers=false $pathMapBuildOption $sln"
}
finally {
Pop-Location
Expand Down
79 changes: 78 additions & 1 deletion src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public class CommandLineTests : CSharpTestBase
private static readonly string s_defaultSdkDirectory = RuntimeEnvironment.GetRuntimeDirectory();
private static readonly string s_compilerVersion = typeof(Csc).GetTypeInfo().Assembly.GetCustomAttribute<AssemblyFileVersionAttribute>().Version;
private static readonly string s_compilerShortCommitHash =
CommonCompiler.ExtractShortCommitHash(typeof(CSharpCompiler).GetTypeInfo().Assembly.GetCustomAttribute<CommitHashAttribute>().Hash);
CommonCompiler.ExtractShortCommitHash(typeof(CSharpCompiler).GetTypeInfo().Assembly.GetCustomAttribute<CommitHashAttribute>()?.Hash);

private readonly string _baseDirectory = TempRoot.Root;

Expand Down Expand Up @@ -9030,6 +9030,7 @@ public void ParseSeparatedPaths_QuotedComma()
paths);
}

[CompilerTrait(CompilerFeature.Determinism)]
[Fact]
public void PathMapParser()
{
Expand Down Expand Up @@ -9063,6 +9064,82 @@ public void PathMapParser()
Assert.Equal((int)ErrorCode.ERR_InvalidPathMap, parsedArgs.Errors[0].Code);
}

[Fact]
[CompilerTrait(CompilerFeature.Determinism)]
public void PathMapPdbParser()
{
var dir = Path.Combine(_baseDirectory, "a");
var parsedArgs = DefaultParse(new[] { $@"/pathmap:{dir}=b:\", "a.cs", @"/pdb:a\data.pdb", "/debug:full" }, _baseDirectory);
parsedArgs.Errors.Verify();
Assert.Equal(Path.Combine(dir, @"data.pdb"), parsedArgs.PdbPath);

// This value is calculate during Emit phases and should be null even in the face of a pathmap targeting it.
Assert.Null(parsedArgs.EmitOptions.PdbFilePath);
}

[Fact]
[CompilerTrait(CompilerFeature.Determinism)]
public void PathMapPdbEmit()
{
void AssertPdbEmit(TempDirectory dir, string pdbPath, string pePdbPath, params string[] extraArgs)
{
var source = @"class Program { static void Main() { } }";
var src = dir.CreateFile("a.cs").WriteAllText(source);
var defaultArgs = new[] { "/nologo", "a.cs", "/out:a.exe", "/debug:full", $"/pdb:{pdbPath}" };
var isDeterministic = extraArgs.Contains("/deterministic");
var args = defaultArgs.Concat(extraArgs).ToArray();
var outWriter = new StringWriter(CultureInfo.InvariantCulture);

var csc = new MockCSharpCompiler(null, dir.Path, args);
int exitCode = csc.Run(outWriter);
Assert.Equal(0, exitCode);

var exePath = Path.Combine(dir.Path, "a.exe");
Assert.True(File.Exists(exePath));
Assert.True(File.Exists(pdbPath));
using (var peStream = File.OpenRead(exePath))
{
PdbValidation.ValidateDebugDirectory(peStream, null, pePdbPath, isDeterministic);
}
}

// Case with no mappings
using (var dir = new DisposableDirectory(Temp))
{
var pdbPath = Path.Combine(dir.Path, "a.pdb");
AssertPdbEmit(dir, pdbPath, pdbPath);
}

// Simple mapping
using (var dir = new DisposableDirectory(Temp))
{
var pdbPath = Path.Combine(dir.Path, "a.pdb");
AssertPdbEmit(dir, pdbPath, @"q:\a.pdb", $@"/pathmap:{dir.Path}=q:\");
}

// Simple mapping deterministic
using (var dir = new DisposableDirectory(Temp))
{
var pdbPath = Path.Combine(dir.Path, "a.pdb");
AssertPdbEmit(dir, pdbPath, @"q:\a.pdb", $@"/pathmap:{dir.Path}=q:\", "/deterministic");
}

// Partial mapping
using (var dir = new DisposableDirectory(Temp))
{
dir.CreateDirectory("pdb");
var pdbPath = Path.Combine(dir.Path, @"pdb\a.pdb");
AssertPdbEmit(dir, pdbPath, @"q:\pdb\a.pdb", $@"/pathmap:{dir.Path}=q:\");
}

// Legacy feature flag
using (var dir = new DisposableDirectory(Temp))
{
var pdbPath = Path.Combine(dir.Path, "a.pdb");
AssertPdbEmit(dir, pdbPath, @"a.pdb", $@"/features:pdb-path-determinism");
}
}

[WorkItem(7588, "https://github.com/dotnet/roslyn/issues/7588")]
[Fact]
public void Version()
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

namespace Microsoft.CodeAnalysis.CSharp.UnitTests.Emit
{
[CompilerTrait(CompilerFeature.Determinism)]
public class DeterministicTests : EmitMetadataTestBase
{
private Guid CompiledGuid(string source, string assemblyName, bool debug)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -468,5 +468,6 @@ private AnalyzerFileReference ResolveAnalyzerReference(CommandLineAnalyzerRefere
return null;
}
#endregion

}
}
16 changes: 5 additions & 11 deletions src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -606,16 +606,10 @@ private int RunCore(TextWriter consoleOutput, ErrorLogger errorLogger, Cancellat
// NOTE: Unlike the PDB path, the XML doc path is not embedded in the assembly, so we don't need to pass it to emit.
var emitOptions = Arguments.EmitOptions.
WithOutputNameOverride(outputName).
WithPdbFilePath(finalPdbFilePath);

// The PDB path is emitted in it's entirety into the PE. This makes it impossible to have deterministic
// builds that occur in different source directories. To enable this we shave all path information from
// the PDB when specified by the user.
//
// This is a temporary work around to allow us to make progress with determinism. The following issue
// tracks getting an official solution here.
//
// https://github.com/dotnet/roslyn/issues/9813
WithPdbFilePath(PathUtilities.NormalizePathPrefix(finalPdbFilePath, Arguments.PathMap));

// This feature flag is being maintained until our next major release to avoid unnecessary
// compat breaks with customers.
if (Arguments.ParseOptions.Features.ContainsKey("pdb-path-determinism") && !string.IsNullOrEmpty(emitOptions.PdbFilePath))
{
emitOptions = emitOptions.WithPdbFilePath(Path.GetFileName(emitOptions.PdbFilePath));
Expand Down Expand Up @@ -766,7 +760,7 @@ private int RunCore(TextWriter consoleOutput, ErrorLogger errorLogger, Cancellat
metadataOnly: emitOptions.EmitMetadataOnly,
includePrivateMembers: emitOptions.IncludePrivateMembers,
emitTestCoverageData: emitOptions.EmitTestCoverageData,
pdbFilePath: emitOptions.PdbFilePath,
pePdbFilePath: emitOptions.PdbFilePath,
cancellationToken: cancellationToken);
}
finally
Expand Down
31 changes: 13 additions & 18 deletions src/Compilers/Core/Portable/Compilation/Compilation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2228,7 +2228,7 @@ internal EmitResult Emit(
metadataOnly: options.EmitMetadataOnly,
includePrivateMembers: options.IncludePrivateMembers,
emitTestCoverageData: options.EmitTestCoverageData,
pdbFilePath: options.PdbFilePath,
pePdbFilePath: options.PdbFilePath,
cancellationToken: cancellationToken);
}
}
Expand Down Expand Up @@ -2391,7 +2391,7 @@ internal bool SerializeToPeStream(
bool metadataOnly,
bool includePrivateMembers,
bool emitTestCoverageData,
string pdbFilePath,
string pePdbFilePath,
CancellationToken cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();
Expand All @@ -2409,20 +2409,15 @@ internal bool SerializeToPeStream(
// PDB Stream provider should not be given if PDB is to be embedded into the PE file:
Debug.Assert(moduleBeingBuilt.DebugInformationFormat != DebugInformationFormat.Embedded || pdbStreamProvider == null);

string pdbPath = (pdbStreamProvider != null || moduleBeingBuilt.DebugInformationFormat == DebugInformationFormat.Embedded) ?
(pdbFilePath ?? FileNameUtilities.ChangeExtension(SourceModule.Name, "pdb")) : null;

// The PDB path is emitted in it's entirety into the PE. This makes it impossible to have deterministic
// builds that occur in different source directories. To enable this we shave all path information from
// the PDB when specified by the user.
//
// This is a temporary work around to allow us to make progress with determinism. The following issue
// tracks getting an official solution here.
//
// https://github.com/dotnet/roslyn/issues/9813
string pePdbPath = (moduleBeingBuilt.DebugInformationFormat == DebugInformationFormat.Embedded || Feature("pdb-path-determinism") != null) && !string.IsNullOrEmpty(pdbPath)
? Path.GetFileName(pdbPath)
: pdbPath;
if ((moduleBeingBuilt.DebugInformationFormat == DebugInformationFormat.Embedded || pdbStreamProvider != null))
{
pePdbFilePath = pePdbFilePath ?? FileNameUtilities.ChangeExtension(SourceModule.Name, "pdb");
}

if (moduleBeingBuilt.DebugInformationFormat == DebugInformationFormat.Embedded && !string.IsNullOrEmpty(pePdbFilePath))
{
pePdbFilePath = Path.GetFileName(pePdbFilePath);
}

try
{
Expand All @@ -2433,7 +2428,7 @@ internal bool SerializeToPeStream(
// The calls ISymUnmanagedWriter2.GetDebugInfo require a file name in order to succeed. This is
// frequently used during PDB writing. Ensure a name is provided here in the case we were given
// only a Stream value.
nativePdbWriter = new Cci.PdbWriter(pdbPath, testSymWriterFactory, deterministic);
nativePdbWriter = new Cci.PdbWriter(pePdbFilePath, testSymWriterFactory, deterministic);
}

Func<Stream> getPortablePdbStream;
Expand Down Expand Up @@ -2529,7 +2524,7 @@ internal bool SerializeToPeStream(
getRefPeStream,
getPortablePdbStream,
nativePdbWriter,
pePdbPath,
pePdbFilePath,
metadataOnly,
includePrivateMembers,
deterministic,
Expand Down
36 changes: 36 additions & 0 deletions src/Compilers/Core/Portable/FileSystem/PathUtilities.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.IO;
using System.Linq;
Expand Down Expand Up @@ -615,6 +616,41 @@ private static int PathHashCode(string path)
return hc;
}

public static string NormalizePathPrefix(string filePath, ImmutableArray<KeyValuePair<string, string>> pathMap)
{
if (pathMap.IsDefaultOrEmpty)
{
return filePath;
}

// find the first key in the path map that matches a prefix of the normalized path (followed by a path separator).
// Note that we expect the client to use consistent capitalization; we use ordinal (case-sensitive) comparisons.
foreach (var kv in pathMap)
{
var oldPrefix = kv.Key;
if (!(oldPrefix?.Length > 0)) continue;

if (filePath.StartsWith(oldPrefix, StringComparison.Ordinal) && filePath.Length > oldPrefix.Length && PathUtilities.IsDirectorySeparator(filePath[oldPrefix.Length]))
{
var replacementPrefix = kv.Value;

// Replace that prefix.
var replacement = replacementPrefix + filePath.Substring(oldPrefix.Length);

// Normalize the path separators if used uniformly in the replacement
bool hasSlash = replacementPrefix.IndexOf('/') >= 0;
bool hasBackslash = replacementPrefix.IndexOf('\\') >= 0;
return
(hasSlash && !hasBackslash) ? replacement.Replace('\\', '/') :
(hasBackslash && !hasSlash) ? replacement.Replace('/', '\\') :
replacement;
}
}

return filePath;
}


public static readonly IEqualityComparer<string> Comparer = new PathComparer();

private class PathComparer : IEqualityComparer<string>
Expand Down
38 changes: 2 additions & 36 deletions src/Compilers/Core/Portable/SourceFileResolver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public SourceFileResolver(
throw new ArgumentException(CodeAnalysisResources.NullValueInPathMap, nameof(pathMap));
}

if (IsPathSeparator(key[key.Length - 1]))
if (PathUtilities.IsDirectorySeparator(key[key.Length - 1]))
{
throw new ArgumentException(CodeAnalysisResources.KeyInPathMapEndsWithSeparator, nameof(pathMap));
}
Expand All @@ -83,35 +83,7 @@ public SourceFileResolver(
public override string NormalizePath(string path, string baseFilePath)
{
string normalizedPath = FileUtilities.NormalizeRelativePath(path, baseFilePath, _baseDirectory);
return (normalizedPath == null || _pathMap.IsDefaultOrEmpty) ? normalizedPath : NormalizePathPrefix(normalizedPath, _pathMap);
}

private static string NormalizePathPrefix(string normalizedPath, ImmutableArray<KeyValuePair<string, string>> pathMap)
{
// find the first key in the path map that matches a prefix of the normalized path (followed by a path separator).
// Note that we expect the client to use consistent capitalization; we use ordinal (case-sensitive) comparisons.
foreach (var kv in pathMap)
{
var oldPrefix = kv.Key;
if (!(oldPrefix?.Length > 0)) continue;
if (normalizedPath.StartsWith(oldPrefix, StringComparison.Ordinal) && normalizedPath.Length > oldPrefix.Length && IsPathSeparator(normalizedPath[oldPrefix.Length]))
{
var replacementPrefix = kv.Value;

// Replace that prefix.
var replacement = replacementPrefix + normalizedPath.Substring(oldPrefix.Length);

// Normalize the path separators if used uniformly in the replacement
bool hasSlash = replacementPrefix.IndexOf('/') >= 0;
bool hasBackslash = replacementPrefix.IndexOf('\\') >= 0;
return
(hasSlash && !hasBackslash) ? replacement.Replace('\\', '/') :
(hasBackslash && !hasSlash) ? replacement.Replace('/', '\\') :
replacement;
}
}

return normalizedPath;
return (normalizedPath == null || _pathMap.IsDefaultOrEmpty) ? normalizedPath : PathUtilities.NormalizePathPrefix(normalizedPath, _pathMap);
}

public override string ResolveReference(string path, string baseFilePath)
Expand All @@ -125,12 +97,6 @@ public override string ResolveReference(string path, string baseFilePath)
return FileUtilities.TryNormalizeAbsolutePath(resolvedPath);
}

// For purposes of command-line processing, allow both \ and / to act as path separators.
internal static bool IsPathSeparator(char c)
{
return (c == '\\') || (c == '/');
}

public override Stream OpenRead(string resolvedPath)
{
CompilerPathUtilities.RequireAbsolutePath(resolvedPath, nameof(resolvedPath));
Expand Down
Loading

0 comments on commit b66f68c

Please sign in to comment.