Option for removing PDB path information in PE #9814
Conversation
|
CC @dotnet/roslyn-compiler, @tmat |
| @@ -1776,6 +1776,18 @@ private static EmitResult ToEmitResultAndFree(DiagnosticBag diagnostics, bool su | |||
| bool emitPortablePdb = moduleBeingBuilt.EmitOptions.DebugInformationFormat == DebugInformationFormat.PortablePdb; | |||
| string pdbPath = (pdbStreamProvider != null) ? (moduleBeingBuilt.EmitOptions.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 | |||
There was a problem hiding this comment.
I think this should be done in the command line compiler, not here. We should just use whatever was set in EmitOptions.
There was a problem hiding this comment.
It could be done around https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs#L437 as @tmat suggests, but I think of this (i.e. the decision what bits should be placed into the PDB) more as a feature of the core compiler, not the command-line processor. So I like the code here.
There was a problem hiding this comment.
From API point of view it's kind of weird that the caller explicitly sets a path they want to put into the PDB and we change that based on another option.
There was a problem hiding this comment.
I don't think this can be done in the command line compiler. There are two PDB paths we have to consider:
- The path where the PDB is emitted on disk.
- The path embedded in the PE.
If we did the work in the compiler it would effect both options. Doing the work here allows us to properly separate the concerns.
There was a problem hiding this comment.
/// <summary>
/// The name of the PDB file to be embedded in the PE image, or null to use the default.
/// </summary>
/// <remarks>
/// If not specified the file name of the source module with an extension changed to "pdb" is used.
/// </remarks>
public string PdbFilePath { get; private set; }This is not the path that the PDB is saved to. Emit API is not concerned with saving into a file. It just writes to the stream. The command line compiler opens the file stream.
There was a problem hiding this comment.
@tmat I don't see how EmitOptions.PdbFilePath can be a safe value to change. As shown above that path is definitely used in at least one case to control the file path written to disk. It seems that this is a bug or the documentation comment for that property is inaccurate.
There was a problem hiding this comment.
Do you mean the fact that the path is passed to PdbWriter? PdbWriter always writes to a stream. It doesn't write to the specified path. It needs the path just because the native implementation is odd.
There was a problem hiding this comment.
This comment is imprecise:
// 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);It should say "Ensure a name is provided here since we write to a stream."
There was a problem hiding this comment.
PdbWriter always writes to a stream. It doesn't write to the specified path. It needs the path just because the native implementation is odd.
That is indeed odd. If that's the case I'll try and find a better place for the work around.
|
👍 |
Option for removing PDB path information in PE
|
LGTM |
Related to #9813