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

EnC: Fix document out-of-sync checks when module is not loaded when d… #39836

Merged
merged 5 commits into from
Nov 18, 2019

Conversation

tmat
Copy link
Member

@tmat tmat commented Nov 15, 2019

…ebugging session starts

Previous change #39295 allowed us to rely on PDB source file checksums when distinguishing between design-time-only documents that should be ignored when applying changes and documents whose changes must be applied to the debuggee. The change did not handle the case when the PDB isn't loaded yet when this check is being performed. This bug resulted in changes not being applied correctly and documents getting out-of-sync in scenarios where the modules were not loaded to the debuggee fast enough.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1013242

…ebugging session starts

Previous change dotnet#39295 allowed us to rely on PDB source file checksums when distinguishing between design-time-only documents that should be ignored when applying changes and documents whose changes must be applied to the debuggee. The change did not handle the case when the PDB isn't loaded yet when this check is being performed. This bug resulted in changes not being applied correctly and documents getting out-of-sync in scenarios where the modules were not loaded to the debuggee fast enough.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1013242
@tmat tmat requested a review from a team as a code owner November 15, 2019 03:01
@tmat
Copy link
Member Author

tmat commented Nov 15, 2019

@ivanbasov @jasonmalinowski PTAL

ivanbasov
ivanbasov previously approved these changes Nov 15, 2019
Copy link
Contributor

@ivanbasov ivanbasov left a comment

Choose a reason for hiding this comment

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

:shipit:

@tmat
Copy link
Member Author

tmat commented Nov 15, 2019

Although this change fixes the linked bug, there are some more cases I'm working thru...

@ivanbasov ivanbasov dismissed their stale review November 16, 2019 00:23

revoking review

Copy link
Contributor

@ivanbasov ivanbasov left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

I'll fully admit I don't understand some of the debugger stuff but I can definitely sign off on the well-written comments!


Assert.True(reader.TryGetDocumentChecksum("/a/c.cs", out var actualChecksum, out var actualAlgorithm));
Assert.Equal("21-C8-B2-D7-A3-6B-49-C7-57-DF-67-B8-1F-75-DF-6A-64-FD-59-22", BitConverter.ToString(actualChecksum.ToArray()));
Assert.Equal(new Guid("ff1816ec-aa5e-4d10-87f7-6f4963833460"), actualAlgorithm);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a constant for that?

/// <summary>
/// The current document content matches the content the built module was compiled with.
/// The document content is matched with the build output instead of the loaded module
/// since the module hasn't been loaded yet.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// since the module hasn't been loaded yet.
// since the module hasn't been loaded yet.

@tmat tmat merged commit 82f2e25 into dotnet:release/dev16.4-vs-deps Nov 18, 2019
@tmat tmat deleted the EncNotLoadedModules branch November 18, 2019 23:42
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.

4 participants