Skip to content

Fix param-nullchecking crash related to EmbeddedTypesManager#58825

Merged
RikkiGibson merged 1 commit intodotnet:mainfrom
RikkiGibson:pnc-embeddable
Jan 18, 2022
Merged

Fix param-nullchecking crash related to EmbeddedTypesManager#58825
RikkiGibson merged 1 commit intodotnet:mainfrom
RikkiGibson:pnc-embeddable

Conversation

@RikkiGibson
Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson commented Jan 13, 2022 •
edited
Loading

Closes #58824
Related to #36024

Need to figure out what a reproducer test case would look like. Have added a reproducer.

Manually verified that the project in #58824 builds with this change (when the previous change #58822 is also included).

@ghost ghost added the Area-Compilers label Jan 13, 2022
@RikkiGibson RikkiGibson marked this pull request as ready for review January 13, 2022 22:04
@RikkiGibson RikkiGibson requested a review from a team as a code owner January 13, 2022 22:04
RikkiGibson
RikkiGibson commented Jan 13, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Emitter/NoPia/EmbeddedTypesManager.cs
Debug.Assert(member.AdaptedSymbol.IsDefinition);
Debug.Assert(ModuleBeingBuilt.SourceModule.AnyReferencedAssembliesAreLinked);

if (member.AdaptedSymbol.OriginalDefinition is SynthesizedGlobalMethodSymbol)
Copy link
Copy Markdown
Member Author

@RikkiGibson RikkiGibson Jan 13, 2022

Choose a reason for hiding this comment

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

SynthesizedGlobalMethodSymbol has a null ContainingType, so we avoid checking its ContainingType here.

AlekseyTs
AlekseyTs approved these changes Jan 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 1)

@RikkiGibson
Copy link
Copy Markdown
Member Author

RikkiGibson commented Jan 18, 2022

@dotnet/roslyn-compiler for the second review

@cston
Copy link
Copy Markdown
Contributor

cston commented Jan 18, 2022 

    protected override EmbeddedType GetEmbeddedTypeForMember(SymbolAdapter member, SyntaxNode syntaxNodeOpt, DiagnosticBag diagnostics)

How is this method affected by the parameter null checking change?

Refers to: src/Compilers/CSharp/Portable/Emitter/NoPia/EmbeddedTypesManager.cs:587 in 2028508. [](commit_id = 2028508, deletion_comment = False)

@RikkiGibson
Copy link
Copy Markdown
Member Author

RikkiGibson commented Jan 18, 2022 •
edited
Loading 

    protected override EmbeddedType GetEmbeddedTypeForMember(SymbolAdapter member, SyntaxNode syntaxNodeOpt, DiagnosticBag diagnostics)

How is this method affected by the parameter null checking change?

Refers to: src/Compilers/CSharp/Portable/Emitter/NoPia/EmbeddedTypesManager.cs:587 in 2028508. [](commit_id = 2028508, deletion_comment = False)

When a metadata reference with embedInteropTypes: true is present, we make an additional pass over all the symbols in use, and whenever we find a member from the embedInteropTypes: true reference, we embed the member (and any containing types) into the output assembly. This is the NoPIA scenario.

Since we now have the <PrivateImplementationDetails>.ThrowIfNull method in the bound tree, we end up visiting its symbol and expecting it to have a non-null containing type. The synthesized global methods are unusual for method symbols used in the emit layer since they have no ContainingType in the symbol model. That causes a crash in combination with the NoPIA behavior described above.

@RikkiGibson RikkiGibson merged commit 497011d into dotnet:main Jan 18, 2022
@ghost ghost added this to the Next milestone Jan 18, 2022
@RikkiGibson RikkiGibson deleted the pnc-embeddable branch January 18, 2022 17:50
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Compilers

Projects

None yet

Milestone

17.2.P1

Development

Successfully merging this pull request may close these issues.

Param-nullchecking crash in EmbeddedTypesManager

3 participants

@RikkiGibson @cston @AlekseyTs