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

Allow multiple documents in method debug info #66359

Merged
merged 3 commits into from
Jan 14, 2023

Conversation

cston
Copy link
Member

@cston cston commented Jan 11, 2023

The EE uses the document name from the method debug info in the PDB to recognize file-local types, and the EE was assuming the method debug info contained a single document in all cases, otherwise the debug info was ignored. For methods with sequence points from multiple documents (for example, a constructor for a type with a field initializer in a separate partial class), this resulted in no local scopes in the EE within those methods.

For portable PDBs, if the method has sequence points from multiple documents, no document is included in the method debug info. The fix is to skip reading the document in that case.

For Windows PDBs, reading the document info when there are multiple documents is not possible currently (see #66260), so documents are ignored from Windows PDBs, and as a result, file-local types are not available from the EE when debugging with a Windows PDB.

Fixes #66109

VSO bug #1713184

@cston cston marked this pull request as ready for review January 12, 2023 00:37
@cston cston requested a review from a team as a code owner January 12, 2023 00:37
@RikkiGibson RikkiGibson self-assigned this Jan 12, 2023
// we never expect to bind a file type in a context where the BuckStopsHereBinder lacks an AssociatedFileIdentifier
return lastBinder.AssociatedFileIdentifier.Value;
// BuckStopsHereBinder.AssociatedFileIdentifier may be null from the EE.
return lastBinder.AssociatedFileIdentifier.GetValueOrDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess if the file identifier we get for this binder chain is default, we simply return false from IsInScopeOfAssociatedSyntaxTree?

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider asserting lastBinder.AssociatedFileIdentifier.HasValue || (this.Flags & BinderFlags.InEEMethodBinder) != 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess if the file identifier we get for this binder chain is default, we simply return false from IsInScopeOfAssociatedSyntaxTree?

Correct.

Consider asserting lastBinder.AssociatedFileIdentifier.HasValue || (this.Flags & BinderFlags.InEEMethodBinder) != 0.

Sounds good. I'll add that in a subsequent PR unless other changes are needed here.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 2)

@cston
Copy link
Member Author

cston commented Jan 13, 2023

Updated PR to avoid changing the compiler code. @RikkiGibson, @jcouv, @jaredpar, please take another look, thanks.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 3)

@cston cston merged commit da2ab18 into dotnet:release/dev17.4 Jan 14, 2023
@cston cston deleted the 66260-Option3-17.4 branch January 14, 2023 01:18
Cosifne added a commit to Cosifne/roslyn that referenced this pull request Apr 20, 2023
MSRC fix. According to the mail, we should update to 16.5.28.

The feature branch is based on dotnet#66423
The last checked in bug fix is dotnet#66359

Since QB doesn't want to approve dotnet#66398 now. I feel we shouldn't wait for that change to merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants