Emitted pdb path should respect PathMap #19593

jaredpar · 2017-05-17T20:08:19Z

Customer scenario

The scenario is enabling fully deterministic builds across different source
enlistments. The PDB path is embedded into the PE. This causes the path
which is embedded, not the path written to disk, to respect the pathmap
compiler option.

Details ...

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

Bugs this fixes:

closes #9813

Workarounds, if any

No official work around.

Risk

Very low

How was the bug found?

Dogfood and customer reports

jaredpar · 2017-05-17T20:08:28Z

CC @dotnet/roslyn-compiler

jaredpar · 2017-05-17T20:08:36Z

CC @MeiChin-Tsai for ask mode approval

jaredpar · 2017-05-17T20:20:37Z

src/Compilers/Core/Portable/CommandLine/CommonCommandLineArguments.cs

@@ -279,6 +279,40 @@ internal CommandLineArguments()
        {
        }

+        internal static string NormalizePathPrefix(string filePath, ImmutableArray<KeyValuePair<string, string>> pathMap)


Moved from SourceFileResolver.

I struggled a bit on finding an appropriate place to put this method. Considered a couple of other places like FileUtilities.cs. But there didn't seem to be a perfect fit for this method. Open to suggestions here.

I think the dependency of SourceFileResolver on CommandLineArguments is weird. Why isn't PathUtilities a good place?

In reply to: 117100409 [](ancestors = 117100409)

PathUtilities felt like a place which was general path manipulation free of any compiler feature logic.

As said before though, I don't have a strong opinion here. Just couldn't find a perfect fit.

I don't see it as coupled with compiler feature. Path Map could be treated as a general self-contained concept.

MeiChin-Tsai · 2017-05-17T20:36:52Z

approved. Seems that all checks are not completed yet.

jaredpar · 2017-05-17T21:26:39Z

Hmm, tests were passing locally. Wonder if one of my last second minor clean ups changed behavior. 😦

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: dotnet#19592 closes dotnet#9813

jaredpar · 2017-05-17T22:53:25Z

@dotnet/roslyn-compiler, @tmat fixed the test failure issue. Apparently I reformatted a VB file when I was cleaning up my code for submission and that broke a bunch of tests 😦

tmat · 2017-05-17T22:56:44Z

🕐

tmat · 2017-05-17T23:00:18Z

src/Compilers/Core/Portable/SourceFileResolver.cs

@@ -66,7 +66,7 @@ public SourceFileResolver(ImmutableArray<string> searchPaths, string baseDirecto
                        throw new ArgumentException(CodeAnalysisResources.NullValueInPathMap, nameof(pathMap));
                    }

-                    if (IsPathSeparator(key[key.Length - 1]))
+                    if (PathUtilities.IsDirectorySeparator(key[key.Length - 1]))


PathUtilities.IsDirectorySeparator [](start = 24, length = 34)

Just realized -- this changes semantics on Unix. The helper in PathUtilities uses the current platform separator(s), but here we always want both / and \

tmat · 2017-05-17T23:00:58Z

src/Compilers/Core/Portable/FileSystem/PathUtilities.cs

+                var oldPrefix = kv.Key;
+                if (!(oldPrefix?.Length > 0)) continue;
+
+                if (filePath.StartsWith(oldPrefix, StringComparison.Ordinal) && filePath.Length > oldPrefix.Length && PathUtilities.IsDirectorySeparator(filePath[oldPrefix.Length]))


PathUtilities.IsDirectorySeparator [](start = 118, length = 34)

Just realized -- this changes semantics on Unix. The helper in PathUtilities uses the current platform separator(s), but here we always want both / and \

gafter · 2017-05-17T23:22:32Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

-                : pdbPath;
+            if ((moduleBeingBuilt.DebugInformationFormat == DebugInformationFormat.Embedded || pdbStreamProvider != null))
+            {
+                pePdbFilePath = pePdbFilePath ?? FileNameUtilities.ChangeExtension(SourceModule.Name, "pdb");


if pePdbFilePath is null when we get here, so that we compute the file name after the ?? do we still perform the path mapping?

Oh, I see that there is no directory in the path in that case.

gafter · 2017-05-17T23:22:47Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

-            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))


Double parens just to be safe ;)

Ha. I actually had a && here for a bit hence the extra parens. Will remove.

jaredpar · 2017-05-17T23:37:08Z

Fixed issue around path separators.

jaredpar · 2017-05-18T17:29:25Z

Looked at the integration test failures. Both appear to be the known timeout bug.

jaredpar · 2017-05-18T17:30:00Z

retest windows_debug_vs-integration_prtest please

jaredpar · 2017-05-18T17:30:11Z

retest windows_release_vs-integration_prtest please

gafter

LGTM

AlekseyTs · 2017-05-18T17:47:23Z

src/Compilers/Core/Portable/CommandLine/CommonCommandLineArguments.cs

@@ -468,5 +468,6 @@ private AnalyzerFileReference ResolveAnalyzerReference(CommandLineAnalyzerRefere
            return null;
        }
        #endregion
+
    }


} [](start = 4, length = 1)

It looks like whitespace is the only change in this file, consider reverting.

AlekseyTs

LGTM

jaredpar added the Area-Compilers label May 17, 2017

dnfclas added the cla-already-signed label May 17, 2017

jaredpar commented May 17, 2017

View reviewed changes

jaredpar force-pushed the fix-pathmap branch from a6b3c23 to aaab10b Compare May 17, 2017 22:52

tmat reviewed May 17, 2017

View reviewed changes

gafter added the Concept-Determinism The issue involves our ability to support determinism in binaries and PDBs created at build time. label May 17, 2017

gafter reviewed May 17, 2017

View reviewed changes

Fix subtle issue around path separators

3bf9063

tmat approved these changes May 17, 2017

View reviewed changes

gafter approved these changes May 18, 2017

View reviewed changes

This was referenced May 18, 2017

Master Unit Test Failures #19556

Closed

C# command line tests fail in development build #19245

Closed

AlekseyTs reviewed May 18, 2017

View reviewed changes

AlekseyTs approved these changes May 18, 2017

View reviewed changes

jaredpar merged commit 305d62b into dotnet:master May 18, 2017

jaredpar deleted the fix-pathmap branch May 18, 2017 19:09

jkotas mentioned this pull request Mar 3, 2018

Compiler does not honor Deterministic flag dotnet/corert#5486

Open

rolfbjarne mentioned this pull request Jan 18, 2019

Restored full path in filename property jbevain/cecil#554

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emitted pdb path should respect PathMap #19593

Emitted pdb path should respect PathMap #19593

jaredpar commented May 17, 2017

jaredpar commented May 17, 2017

jaredpar commented May 17, 2017

jaredpar May 17, 2017

tmat May 17, 2017

jaredpar May 17, 2017

tmat May 17, 2017

MeiChin-Tsai commented May 17, 2017

jaredpar commented May 17, 2017

jaredpar commented May 17, 2017

tmat commented May 17, 2017 •

edited

Loading

tmat May 17, 2017

tmat May 17, 2017

jaredpar May 17, 2017

gafter May 17, 2017

gafter May 17, 2017

gafter May 17, 2017

jaredpar May 17, 2017

jaredpar commented May 17, 2017

jaredpar commented May 18, 2017

jaredpar commented May 18, 2017

jaredpar commented May 18, 2017

gafter left a comment

AlekseyTs May 18, 2017

AlekseyTs left a comment

Emitted pdb path should respect PathMap #19593

Emitted pdb path should respect PathMap #19593

Conversation

jaredpar commented May 17, 2017

jaredpar commented May 17, 2017

jaredpar commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MeiChin-Tsai commented May 17, 2017

jaredpar commented May 17, 2017

jaredpar commented May 17, 2017

tmat commented May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented May 17, 2017

jaredpar commented May 18, 2017

jaredpar commented May 18, 2017

jaredpar commented May 18, 2017

gafter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

tmat commented May 17, 2017 •

edited

Loading