Restored full path in filename property #554
Conversation
|
@jbevain we need some resolution to this, this is affecting our customers' ability to hit breakpoints in some scenarios. /cc @StephaneDelcroix since XamlG is also mentioned by @joj. Thanks |
|
XamlG doesn't use cecil... |
|
XamlC is the problem, not XamlG. My bad, sorry. |
|
@joj @kzu kind reminder that I work on Cecil on my non existent spare time, and that I was out of 3 weeks and I'm still catching up with my work. @marek-safar maintains https://github.com/mono/cecil that is used all throughout Mono and that can be used for scenarios where you're in a rush to push a fix that hasn't made upstream yet. That being said, I'm always happy to help. The PR removes a line that does read as fully intentional, and before removing it we probably need to find out what it was written like this. |
|
@jbevain I sent to mono first but @marek-safar requested I send it here, which makes a lot of sense anyway :) On the fully intentional, completely agreed. That's what troubles me and that's why I need your help. The thing is that that line changes the value that was already in the pdb, and that breaks UWP (not us, because we don't use that value for finding code). Maybe that was needed in Mac or Linux? I'm not sure. |
|
Found the original issue: That stems from: https://bugzilla.xamarin.com/show_bug.cgi?id=54578 @rolfbjarne, I think we'd need your input to find a resolution here. On my side of things, we can make this behavior a changeable parameter when writing Portable PDBs. |
|
Since iOS doesn't have 32 and 64 bit anymore (I think?) this may not be needed. Another option is exploring this, from rolf's comment:
|
|
@rolfbjarne @spouliot is this still needed? |
Yes, this is still needed.
That would be acceptable for us.
We still support both 32-bit and 64-bit iOS. And in any case we might have 32-bit and 64-bit watchOS in the future as well. |
|
@rolfbjarne can we do that at the iOS level instead. As I suspected, this is also breaking Fody: Fody/Fody#616. That field in the ppdb has a very specific meaning to the .NET debugger, and we're changing meaning by replacing it. I'm not sure how the option of using a setting will work in multi-platform (meaning, when we have to support both the mono debugger and the .NET debugger depending on the startup project). I can investigate that. |
|
If the .NET debugger is always going to require a full path, then it doesn't make a lot of sense to make the behavior a parameter in Cecil, except to ease the transition for @rolfbjarne. It all depends on what it would take for iOS to support comparing DLLs with a different full path for the pdb. @rolfbjarne is that something you could support? |
|
Even though I prefer this behavior to be moved to whoever is consuming it in iOS, I must say that even though the .NET Debugger requires that, Mono doesn't... so it wouldn't be completely wrong to do that if we're only using Mono. The problem there is that if the solution is mixed Mono and .NET, we'll have a problem. Isn't moving the behavior just making whoever is getting that property from iOS do the proper Path.GetFileName() as needed? I may be missing something there. |
I'd really like to avoid that, if possible.
Or maybe it would make more sense to make it possible to specify the actual value written by Cecil? Writing a full path only works if that location continue to exist and can be found when the app is debugged; for iOS this means a full path to the pdb on the mac, when the app is executed on the device (where the path is wrong), and using VS for Windows (where the path is also wrong). This seems like a probable reason things work differently for iOS... IMHO allowing build tools to specify the path would make sense, since those build tools might know where the pdb ends up in the end (fwiw with Roslyn it's possible to control the value using the /pathmap option: dotnet/roslyn#19593) |
|
Wouldn't it make more sense then to write the correct path at compile time instead of changing it via Cecil after the fact? If a parameter exists on csc to set that, we should be overriding it in the targets in ios and then let Cecil do nothing with the value. |
That's another option that I think would work for us: Cecil writes back the value it read in, instead of changing it. |
|
That's what my commit does :) We need to find that option, though, and I don't think PathMap is that. PathMap is used for sourcelink, and if I'm reading this test correctly it doesn't affect the Pdb FilePath: https://github.com/dotnet/roslyn/pull/19593/files/3bf9063a3e536cf914d4e6df1b1a250e595afc54#diff-492ea20f4ae7785ece041adfb13b4e7dR9069 |
|
I need to restart for an update. I'll be back in 10 and look for ways to overwrite that in compile time. |
No, with your change Cecil will write out the path of the file it's creating, not the path of the file it read (which is not necessarily the same path). |
|
Oh, Ok. That will absolutely work if we have that datum at that time. |
|
I don't think this would work. We're taking about the shared (netstandard) library pdb, which is
compiled once and used by all heads. So one approach has to be made to work
for all platforms. A solution that works for iOS but breaks .NET won't work
|
|
Just to be clear: if what we're talking about is leaving the value unmodified to whatever it was before cecil was involved, that will totally work. I see what you're saying, though (GetFileName is the file name of the base stream, which may not be the same). I think I should have the original value at that point, I'll modify the PR to write that out. |
UWP uses the filename property to find the pdb file when debugging. Changing the value that's already in the dll to just the filename renders the debugger unable to find the pdb file, and UWP debugging then works with no symbols (meaning, no breakpoints and not pausing and stepping). The lines I removed seem intentional, though, so I'm opening this for discussion. This is at the moment affecting the XamlG task in Xamarin Forms, but it also probably affects other clients of Cecil like Fody. Android and iOS are not affected by this because the logic for finding the pdb in the mono debugger is slightly different than what it is in the .Net debugger. This commit makes Cecil not touch the filepath property at all, letting consumers of the property use it as if cecil was never even run.
|
I changed the commit to preserve the existing value. This will totally work for what I need (meaning, it will not break forms or fody) but I'm not sure it's semantically correct. I'll let @jbevain decide that. IMHO the test itself shows that semantically this is at least confusing. |
|
Yeah I'm not happy with just writing the old pdb path in the header, not everybody is writing in place. The proper fix is to revert to by default writing back the fullpath of the pdb as that will work for both the .NET and Mono debugger, but that breaks the iOS checks. We either need a way to customize the behavior for the iOS team, or for the iOS checks to skip this value when comparing binaries. @rolfbjarne could you point me to the code in question? I'm interested to read how you read a module, which ISymbolReaderProvider / ISymbolWriterProvider you are using and how you're setting things up. |
|
@jbevain this is the code we use to compare assemblies: we have a method to compare two Streams: https://github.com/xamarin/xamarin-macios/blob/5165f1cdde53c8766592c937159b08e8795dd0a6/tools/common/cache.cs#L135-L185 and when comparing assemblies, we use a custom stream that skips the data we don't want to compare: https://github.com/xamarin/xamarin-macios/blob/5165f1cdde53c8766592c937159b08e8795dd0a6/tools/common/cache.cs#L267-L419. The thing is that I'd hoped to eventually delete all this custom assembly comparing code, when deterministic builds became more commonplace. |
|
@rolfbjarne thanks. Looking at this, it seems that you're simply using the This should make it easy for you to customize the PDB path, by providing your own ISymbolWriterProvider that patches the debug header. With that in place, Cecil can go back to writing the full path. On your end, you'd need to use this, and pass it as the @rolfbjarne would that work for you? public sealed class SymbolWriterProvider : ISymbolWriterProvider
{
private readonly DefaultSymbolWriterProvider _defaultSymbolWriterProvider = new DefaultSymbolWriterProvider();
public ISymbolWriter GetSymbolWriter(ModuleDefinition module, string fileName) => new SymbolWriter(_defaultSymbolWriterProvider.GetSymbolWriter(module, fileName));
public ISymbolWriter GetSymbolWriter(ModuleDefinition module, Stream symbolStream) => new SymbolWriter(_defaultSymbolWriterProvider.GetSymbolWriter(module, symbolStream));
}
public sealed class SymbolWriter : ISymbolWriter
{
private readonly ISymbolWriter _symbolWriter;
public SymbolWriter(ISymbolWriter symbolWriter)
{
_symbolWriter = symbolWriter;
}
public ImageDebugHeader GetDebugHeader()
{
var header = _symbolWriter.GetDebugHeader();
if (!header.HasEntries)
return header;
for (int i = 0; i < header.Entries.Length; i++)
{
header.Entries[i] = ProcessEntry(header.Entries[i]);
}
return header;
}
private static ImageDebugHeaderEntry ProcessEntry(ImageDebugHeaderEntry entry)
{
if (entry.Directory.Type != ImageDebugType.CodeView)
return entry;
var reader = new BinaryReader(new MemoryStream(entry.Data));
var newDataStream = new MemoryStream();
var writer = new BinaryWriter(newDataStream);
var sig = reader.ReadUInt32();
if (sig != 0x53445352)
return entry;
writer.Write(sig); // RSDS
writer.Write(reader.ReadBytes(16)); // MVID
writer.Write(reader.ReadUInt32()); // Age
var length = Array.IndexOf(entry.Data, (byte)0, (int)reader.BaseStream.Position) - (int)reader.BaseStream.Position;
var fullPath = Encoding.UTF8.GetString(reader.ReadBytes(length));
writer.Write(Encoding.UTF8.GetBytes(Path.GetFileName(fullPath)));
writer.Write((byte)0);
var newData = newDataStream.ToArray();
var directory = entry.Directory;
directory.SizeOfData = newData.Length;
return new ImageDebugHeaderEntry(directory, newData);
}
public ISymbolReaderProvider GetReaderProvider() => _symbolWriter.GetReaderProvider();
public void Write(MethodDebugInformation info) => _symbolWriter.Write(info);
public void Dispose() => _symbolWriter.Dispose();
} |
|
@jbevain I tried that, but it doesn't quite work. The problem is that And if it's not implemented, this happens: |
… written to assemblies. We need our 32-bit and 64-bit assemblies to be identical so that we can avoid duplicating the .dll in fat apps. One difference used to be that the .dll contained the full path to the corresponding .pdb ([1]), but we changed cecil to only write the filename ([2]). Unfortunately this change breaks something else, so it has to be reverted ([3]). This implements a different solution: we provide a custom symbol writer to Cecil, which only writes the filename of the pdb in the .dll, not the full path. [1]: https://bugzilla.xamarin.com/show_bug.cgi?id=54578 [2]: jbevain/cecil#372 [3]: jbevain/cecil#554
… written to assemblies. We need our 32-bit and 64-bit assemblies to be identical so that we can avoid duplicating the .dll in fat apps. One difference used to be that the .dll contained the full path to the corresponding .pdb ([1]), but we changed cecil to only write the filename ([2]). Unfortunately this change breaks something else, so it has to be reverted ([3]). This implements a different solution: we provide a custom symbol writer to Cecil, which only writes the filename of the pdb in the .dll, not the full path. [1]: https://bugzilla.xamarin.com/show_bug.cgi?id=54578 [2]: jbevain/cecil#372 [3]: jbevain/cecil#554
|
@rolfbjarne that makes sense, not sure why I didn't see it when running tests. Thank you for giving it a try, I'll get back to you. |
|
@rolfbjarne I've opened #564 to remove this interface, making the code above working. Could you validate on your side that it works for you before I merge? Thanks! |
|
@jbevain yes, that works fine! |
|
@rolfbjarne I've merged the PR, pushed the change to revert to writing back the full pdb path, and updated the custom symbol writer above to handle scenarios where the debug header could have trailing \0 for padding after the path. With this, both you and @joj should be good to go. On my end this is a big enough change that I'll push a 0.10.2 nuget with those changes tomorrow. |
|
Thanks @jbevain! I'll update my nugets once that is done and update everything to portable in our templates. |
|
Do I need to resubmit the change of making the writer write the full path? I see that's still in the writer in that PR. |
|
Yep. Excellent. Thanks! |
… written to assemblies. We need our 32-bit and 64-bit assemblies to be identical so that we can avoid duplicating the .dll in fat apps. One difference used to be that the .dll contained the full path to the corresponding .pdb ([1]), but we changed cecil to only write the filename ([2]). Unfortunately this change breaks something else, so it has to be reverted ([3]). This implements a different solution: we provide a custom symbol writer to Cecil, which only writes the filename of the pdb in the .dll, not the full path. [1]: https://bugzilla.xamarin.com/show_bug.cgi?id=54578 [2]: jbevain/cecil#372 [3]: jbevain/cecil#554 (cherry picked from commit 53874c8) # Conflicts: # tools/mtouch/mtouch.csproj
UWP uses the filename property to find the pdb file when debugging. Changing the value that's already in the dll to just the filename renders the debugger unable to find the pdb file, and UWP debugging then works with no symbols (meaning, no breakpoints and not pausing and stepping). The lines I removed seem intentional, though, so I'm opening this for discussion. This is at the moment affecting the XamlG task in Xamarin Forms, but it also probably affects other clients of Cecil like Fody. Android and iOS are not affected by this because the logic for finding the pdb in the mono debugger is slightly different than what it is in the .Net debugger.