-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preserve custom debug information on types #185
Conversation
@@ -854,6 +854,7 @@ public interface ISymbolReader : IDisposable { | |||
ISymbolWriterProvider GetWriterProvider (); | |||
bool ProcessDebugHeader (ImageDebugHeader header); | |||
MethodDebugInformation Read (MethodDefinition method); | |||
Collection<CustomDebugInformation> Read (ICustomDebugInformationProvider provider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a rather big breaking change - everything which implements the interface would have to now implement the new method. I think this should use default implementation (assuming we can pull that off - netstandard and all), or we would need to introduce a new interface instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can use default implementations since Mono.Cecil builds for netstandard2.0. I'm hoping it's OK to change the interface since we've touched the symbol writing interface before: jbevain#810 (comment) but I'll see if Jb has any feedback. If we have to, I think it would be possible (even if a bit messy) to implement this by doing all of the work in ProcessDebugHeader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another alternative is to introduce a new interface and have the classes which implement ISymbolWriter
implement it as well. And then cast if it it's not available, just do the old thing. Not super clean either though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a guess as to how often people actually implement the Cecil interfaces? Versus just using them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a GitHub search I couldn't find a single implementer of ISymbolReader
other than Cecil, and only one external implementer of ISymbolWriter
, in xamarin-macios.
- Use static lambdas - Check HasCustomDebugInformation - Test writing modified CustomDebugInformation - Formatting fixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from Vitek's concern about changing the interface
Co-authored-by: Jackson Schuster <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the fact that this interface has been broken before and has very few implementors, this looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a reasonable change that fixes a customer scenario. Doesn’t look Like people are really meant to implement these interfaces themselves, so not too worried about a break.
Portable PDBs may have CustomDebugInformation on many metadata entities (see HasCustomDebugInformation in https://github.com/dotnet/runtime/blob/main/docs/design/specs/PortablePdb-Metadata.md#customdebuginformation-table-0x37).
For a select few of the CustomDebugInformation kinds (for example state machine hoisted local scopes), cecil has dedicated types to represent these in the object model. For the rest, cecil just reads the custom debug info out of the blob heap as a
byte[]
.When writing back a module, cecil walks the metadata as represented in its object model, building the debug information as it goes.
So to support custom debug information for a new metadata token type:
ICustomDebugInformationProvider
,This change implements the above for
TypeDefinition
. It extends theISymbolReader
/ISymbolWriter
interfaces with a new method for reading/writing custom debug info for anyICustomDebugInformationProvider
, and provides helpers for calling the symbol reader that can be used when adding custom debug information to other types in the future.It doesn't include support for decoding the
TypeDefinitionDocument
debug info - it continues representing these asBinaryCustomDebugInformation
.Fixes dotnet/runtime#100051.