Skip to content
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

Add PDB support for PersistedAssemblyBuilder #100706

Merged
merged 12 commits into from
Apr 16, 2024
Merged

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Apr 5, 2024

Added new APIs needed for adding symbol info and implemented PDB metadata generation

  • Most PDB tables emitting is inspired by or copied from Roslyn implementation, especially sequence points section
  • For unit testing used MetadataReader
    Approved API shape:
namespace System.Reflection.Emit;

public abstract partial class ModuleBuilder : System.Reflection.Module
{
    [EditorBrowsable(Never)]
    public ISymbolDocumentWriter DefineDocument (string url, Guid language, Guid languageVendor, Guid documentType) => DefineDocument(url, language);

    public ISymbolDocumentWriter DefineDocument(string url, Guid language = default);
    protected virtual ISymbolDocumentWriter DefineDocumentCore(string url, Guid language = default);
}

 public abstract partial class ILGenerator
 {
    public void MarkSequencePoint(ISymbolDocumentWriter document, int startLine, int startColumn, int endLine, int endColumn) { }
    protected virtual void MarkSequencePointCore(ISymbolDocumentWriter document, int startLine, int startColumn, int endLine, int endColumn) { }
 }
 
public abstract partial class LocalBuilder : LocalVariableInfo
{
    public void SetLocalSymInfo(string name);
    protected virtual void SetLocalSymInfoCore(string name) { }
}

public sealed class PersistedAssemblyBuilder : AssemblyBuilder
{
    public MetadataBuilder GenerateMetadata(out BlobBuilder ilStream, out BlobBuilder mappedFieldData, out MetadataBuilder pdbBuilder) { }
}

Contributes to #92975
Fixes #99935

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

jkoritzinsky added a commit to AaronRobinsonMSFT/DNMD that referenced this pull request Apr 10, 2024
/// <param name="endColumn">The column in the line where the sequence point ends.</param>
/// <exception cref="ArgumentException"><paramref name="document"/> is not valid.</exception>
/// <remarks>The parameters validated in the caller: <see cref="MarkSequencePoint"/>.</remarks>
protected virtual void MarkSequencePointCore(ISymbolDocumentWriter document, int startLine, int startColumn, int endLine, int endColumn) { }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it no-op in order to avoid breaking somebody that is not using/emitting PDB, but the more think about it is seems better to throw here so that people know that they are using an AssemblyBuilder implementation that doesn't support symbols.

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

/// <exception cref="ArgumentNullException"><paramref name="document"/> is <see langword="null"/>.</exception>
/// <exception cref="ArgumentException"><paramref name="document"/> is not valid.</exception>
/// <exception cref="ArgumentOutOfRangeException">
/// <paramref name="startLine"/> is not within range [0, 0x20000000) or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did these max values come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the runtime doc

The values of non-hidden sequence point must satisfy the following constraints
* IL Offset is within range [0, 0x20000000)
* IL Offset of a sequence point is lesser than IL Offset of the subsequent sequence point.
* Start Line is within range [0, 0x20000000) and not equal to 0xfeefee.
* End Line is within range [0, 0x20000000) and not equal to 0xfeefee.
* Start Column is within range [0, 0x10000)
* End Column is within range [0, 0x10000)
* End Line is greater or equal to Start Line.
* If Start Line is equal to End Line then End Column is greater than Start Column.

parentImport = GetImportScopeHandle(scope._importNamespaces, parentImport);
firstLocalVariableHandle = GetLocalVariableHandle(scope._locals, firstLocalVariableHandle);
_pdbBuilder.AddLocalScope(methodHandle, parentImport, firstLocalVariableHandle,
constantList: MetadataTokens.LocalConstantHandle(1), scope._startOffset, scope._endOffset - scope._startOffset);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is LocalConstantHandle(1) coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// If no scope defines any constants, <see cref="MetadataTokens.LocalConstantHandle(int)"/>(1).

@buyaa-n buyaa-n merged commit 6f3b1a6 into dotnet:main Apr 16, 2024
150 checks passed
@buyaa-n buyaa-n deleted the pdb_suppport branch April 16, 2024 15:56
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
* Initial PDB API shape and sequence point implementation

* Finish PDB implementation and testing

* Fix bug in multi doc sequence point

* Validate null name for SetLocalSymInfo, do not add locals that has no name to the locals table

* Throw on parent virtual methods, remove unnecessary throw

* Apply suggestions from code review

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

* Apply feedback, fix issue found in sequence point PreviousNonHidden.StatrtLine/StartColumn

---------

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators May 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal] Add APIs that needed for emitting symbols with reflection emit PersistedAssemblyBuilder
5 participants