From b66f68c485fb1c6bbfa69c62490ff47d37d5dae5 Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Wed, 17 May 2017 12:34:17 -0700 Subject: [PATCH] Emitted pdb path should respect PathMap 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: https://github.com/dotnet/roslyn/issues/19592 closes #9813 --- build/scripts/test-determinism.ps1 | 2 +- .../Test/CommandLine/CommandLineTests.cs | 79 ++++++++++++++++++- .../Test/Emit/Emit/DeterministicTests.cs | 1 + .../CommandLine/CommonCommandLineArguments.cs | 1 + .../Portable/CommandLine/CommonCompiler.cs | 16 ++-- .../Core/Portable/Compilation/Compilation.cs | 31 +++----- .../Core/Portable/FileSystem/PathUtilities.cs | 36 +++++++++ .../Core/Portable/SourceFileResolver.cs | 38 +-------- .../Test/CommandLine/CommandLineTests.vb | 68 ++++++++++++++++ .../Portable/Traits/CompilerFeature.cs | 1 + 10 files changed, 206 insertions(+), 67 deletions(-) diff --git a/build/scripts/test-determinism.ps1 b/build/scripts/test-determinism.ps1 index 74f497ebe9e65..c087c9510729a 100644 --- a/build/scripts/test-determinism.ps1 +++ b/build/scripts/test-determinism.ps1 @@ -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 diff --git a/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs b/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs index 093b1192ab2f4..a3d3b87784e2e 100644 --- a/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs +++ b/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs @@ -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().Version; private static readonly string s_compilerShortCommitHash = - CommonCompiler.ExtractShortCommitHash(typeof(CSharpCompiler).GetTypeInfo().Assembly.GetCustomAttribute().Hash); + CommonCompiler.ExtractShortCommitHash(typeof(CSharpCompiler).GetTypeInfo().Assembly.GetCustomAttribute()?.Hash); private readonly string _baseDirectory = TempRoot.Root; @@ -9030,6 +9030,7 @@ public void ParseSeparatedPaths_QuotedComma() paths); } + [CompilerTrait(CompilerFeature.Determinism)] [Fact] public void PathMapParser() { @@ -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() diff --git a/src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs b/src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs index a3a6be5cbb7b4..b393d6bff2fbd 100644 --- a/src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs +++ b/src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs @@ -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) diff --git a/src/Compilers/Core/Portable/CommandLine/CommonCommandLineArguments.cs b/src/Compilers/Core/Portable/CommandLine/CommonCommandLineArguments.cs index 1202e4527b9c4..8fc9437af0378 100644 --- a/src/Compilers/Core/Portable/CommandLine/CommonCommandLineArguments.cs +++ b/src/Compilers/Core/Portable/CommandLine/CommonCommandLineArguments.cs @@ -468,5 +468,6 @@ private AnalyzerFileReference ResolveAnalyzerReference(CommandLineAnalyzerRefere return null; } #endregion + } } diff --git a/src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs b/src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs index 48d6cb87adf90..b40d4d7c77155 100644 --- a/src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs +++ b/src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs @@ -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)); @@ -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 diff --git a/src/Compilers/Core/Portable/Compilation/Compilation.cs b/src/Compilers/Core/Portable/Compilation/Compilation.cs index aa8ebb9a90045..3d755d19ebea6 100644 --- a/src/Compilers/Core/Portable/Compilation/Compilation.cs +++ b/src/Compilers/Core/Portable/Compilation/Compilation.cs @@ -2228,7 +2228,7 @@ internal EmitResult Emit( metadataOnly: options.EmitMetadataOnly, includePrivateMembers: options.IncludePrivateMembers, emitTestCoverageData: options.EmitTestCoverageData, - pdbFilePath: options.PdbFilePath, + pePdbFilePath: options.PdbFilePath, cancellationToken: cancellationToken); } } @@ -2391,7 +2391,7 @@ internal bool SerializeToPeStream( bool metadataOnly, bool includePrivateMembers, bool emitTestCoverageData, - string pdbFilePath, + string pePdbFilePath, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); @@ -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 { @@ -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 getPortablePdbStream; @@ -2529,7 +2524,7 @@ internal bool SerializeToPeStream( getRefPeStream, getPortablePdbStream, nativePdbWriter, - pePdbPath, + pePdbFilePath, metadataOnly, includePrivateMembers, deterministic, diff --git a/src/Compilers/Core/Portable/FileSystem/PathUtilities.cs b/src/Compilers/Core/Portable/FileSystem/PathUtilities.cs index 489d7ba85b1c8..c7fc9ee8f1c5a 100644 --- a/src/Compilers/Core/Portable/FileSystem/PathUtilities.cs +++ b/src/Compilers/Core/Portable/FileSystem/PathUtilities.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.Diagnostics; using System.IO; using System.Linq; @@ -615,6 +616,41 @@ private static int PathHashCode(string path) return hc; } + public static string NormalizePathPrefix(string filePath, ImmutableArray> 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 Comparer = new PathComparer(); private class PathComparer : IEqualityComparer diff --git a/src/Compilers/Core/Portable/SourceFileResolver.cs b/src/Compilers/Core/Portable/SourceFileResolver.cs index d2ca220ab5060..8a5bc05804d3e 100644 --- a/src/Compilers/Core/Portable/SourceFileResolver.cs +++ b/src/Compilers/Core/Portable/SourceFileResolver.cs @@ -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)); } @@ -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> 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) @@ -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)); diff --git a/src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb b/src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb index 9ab1efe92ff5f..a1e9132131e3c 100644 --- a/src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb +++ b/src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb @@ -2887,6 +2887,74 @@ print Goodbye, World" CleanupAllGeneratedFiles(src.Path) End Sub + + + Public Sub PathMapPdbDeterminism() + Dim assertPdbEmit = + Sub(dir As TempDirectory, pePdbPath As String, extraArgs As String()) + + Dim source = + +Imports System +Module Program + Sub Main() + End Sub +End Module + + + Dim src = dir.CreateFile("a.vb").WriteAllText(source.Value) + Dim pdbPath = Path.Combine(dir.Path, "a.pdb") + Dim defaultArgs = {"/nologo", "/debug", "a.vb"} + Dim isDeterministic = extraArgs.Contains("/deterministic") + Dim args = defaultArgs.Concat(extraArgs).ToArray() + Dim outWriter = New StringWriter(CultureInfo.InvariantCulture) + + Dim vbc = New MockVisualBasicCompiler(dir.Path, args) + Dim exitCode = vbc.Run(outWriter) + Assert.Equal(0, exitCode) + + Dim exePath = Path.Combine(dir.Path, "a.exe") + Assert.True(File.Exists(exePath)) + Assert.True(File.Exists(pdbPath)) + + Using peStream = File.OpenRead(exePath) + PdbValidation.ValidateDebugDirectory(peStream, Nothing, pePdbPath, isDeterministic) + End Using + End Sub + + ' No mappings + Using dir As New DisposableDirectory(Temp) + Dim pePdbPath = Path.Combine(dir.Path, "a.pdb") + assertPdbEmit(dir, pePdbPath, {}) + End Using + + ' Simple mapping + Using dir As New DisposableDirectory(Temp) + Dim pePdbPath = "q:\a.pdb" + assertPdbEmit(dir, pePdbPath, {$"/pathmap:{dir.Path}=q:\"}) + End Using + + ' Simple mapping deterministic + Using dir As New DisposableDirectory(Temp) + Dim pePdbPath = "q:\a.pdb" + assertPdbEmit(dir, pePdbPath, {$"/pathmap:{dir.Path}=q:\", "/deterministic"}) + End Using + + ' Partial mapping + Using dir As New DisposableDirectory(Temp) + Dim subDir = dir.CreateDirectory("example") + Dim pePdbPath = "q:\example\a.pdb" + assertPdbEmit(subDir, pePdbPath, {$"/pathmap:{dir.Path}=q:\"}) + End Using + + ' Legacy feature flag + Using dir As New DisposableDirectory(Temp) + Dim pePdbPath = Path.Combine(dir.Path, "a.pdb") + assertPdbEmit(dir, "a.pdb", {"/features:pdb-path-determinism"}) + End Using + End Sub + + Public Sub ParseOut() diff --git a/src/Test/Utilities/Portable/Traits/CompilerFeature.cs b/src/Test/Utilities/Portable/Traits/CompilerFeature.cs index 0530fb6217152..76ff40ae411d9 100644 --- a/src/Test/Utilities/Portable/Traits/CompilerFeature.cs +++ b/src/Test/Utilities/Portable/Traits/CompilerFeature.cs @@ -11,6 +11,7 @@ public enum CompilerFeature Async, Dynamic, ExpressionBody, + Determinism, Iterator, LocalFunctions, Params,