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

Fix source_file bug in mono_ppdb_lookup_location_internal #96497

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

bholmes
Copy link
Contributor

@bholmes bholmes commented Jan 4, 2024

document-records affect sequence points that follow in the byte stream, not points previously read.

When reading the document-record we may already be at the target offset. Do not update docname local if we have reached the end target already otherwise the wrong source_file will be reported for the requested offset

document-records affect sequence points that follow in the byte stream,
not points previously read.

When reading the document-record we may already be at the target offset.
Do not update docname local if we have reached the end target already
otherwise the wrong source_file will be reported for the requested offset
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 4, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 4, 2024
@bholmes
Copy link
Contributor Author

bholmes commented Jan 4, 2024

@thaystg Can you think of a way to add a test for this? (I have verified the fix in Unity)

Our bug was from a class that had the ctor method body defined in one file and variables with initializers in another.
The last variable initialization line file name would be reported incorrectly.

@thaystg
Copy link
Member

thaystg commented Jan 4, 2024

"The last variable initialization line file name would be reported incorrectly." does it mean that you could not step in the correct line, right?

@thaystg
Copy link
Member

thaystg commented Jan 4, 2024

If this is the case you can create a test case on wasm debugger tests.

public async Task CreateGoodBreakpointAndHit()
{

Like this one, trying to add a breakpoint at the last variable initialization line.

Please let me know if you need any help to do it.

@bholmes
Copy link
Contributor Author

bholmes commented Jan 4, 2024

@bholmes
Copy link
Contributor Author

bholmes commented Jan 5, 2024

@thaystg I have not been able to add a test to the BreakpointTests. As soon as I add a field with initializer to a partial class in the debugger-test2.cs file, I can no longer hit any breakpoints inside of (or any methods called by) the .ctor of the same partial class in debugger-test.cs. This is both with and without my changes. I am going to give up on this.

Also any reason to be concerned about the 'Build browser-wasm linux Release Mono_DebuggerTests_chrome' failure?

@thaystg thaystg merged commit a58f7ca into dotnet:main Jan 5, 2024
111 checks passed
@thaystg
Copy link
Member

thaystg commented Jan 5, 2024

@ilonatommy if you have any available time, can you try to create a test case for this one?

@vargaz
Copy link
Contributor

vargaz commented Jan 23, 2024

This seems to have caused a regression:
#97342

@thaystg
Copy link
Member

thaystg commented Jan 23, 2024

Let's revert this and backport the PR to net9 preview1?

@unity-nikos
Copy link

@thaystg @vargaz @bholmes could this be the case in trunk/LTS too? As the same PR landed there here.

@bholmes
Copy link
Contributor Author

bholmes commented Feb 20, 2024

@thaystg @vargaz @bholmes could this be the case in trunk/LTS too? As the same PR landed there here.

Unity 1897 was patched with Unity-Technologies/mono#1941

@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants