Skip to content

[mono] catch the case of updated methods in mono_debug_lookup_source_location #94540

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 12, 2023
Merged

[mono] catch the case of updated methods in mono_debug_lookup_source_location #94540

merged 5 commits into from
Nov 12, 2023

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Nov 8, 2023 •
edited
Loading

If a newly added method is in a stack trace, look it up in the EnC debug information, if available.

Related to #93860

@ghost ghost assigned lambdageek Nov 8, 2023
@ghost
Copy link

ghost commented Nov 8, 2023

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

If a newly added method is in a stack trace, look it up in the EnC debug information, if available.

Related to #93860

TODO:

  • Add a variant of the test where we apply the update without a PDB delta (needs test infrastructure fixup)
  • Add debugger tests
Author: lambdageek
Assignees: -
Labels:

area-Diagnostics-coreclr
Milestone: -

@lambdageek lambdageek added area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc and removed area-Diagnostics-coreclr labels Nov 8, 2023
@lambdageek
Copy link
Member Author

I think I also need to update mono_debug_lookup_source_location_by_il, and figure out how to make a test get in there

@build-analysis build-analysis bot mentioned this pull request Nov 8, 2023
Closed
3 tasks
@jeromelaban jeromelaban mentioned this pull request Nov 9, 2023
Closed
@lambdageek
Copy link
Member Author

Actually I think mono_debug_lookup_source_location_by_il doesn't need an update (or at least it's not obvious to me that it is useful).

It's called from 3 places:

  1. From mono_get_frame_info if il_offset != -1 - but as far as I can tell il_offset is never set to anything else. That branch is dead code.
  2. From mono_emit_jit_dump but that's support for profiling, which isn't something we support with hot reload.

@lambdageek
Copy link
Member Author

/backport to release/8.0-staging

@github-actions GitHub Actions
Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6828799602

@github-actions github-actions bot mentioned this pull request Nov 10, 2023
Merged
@build-analysis build-analysis bot mentioned this pull request Nov 10, 2023
Closed
@lambdageek lambdageek mentioned this pull request Nov 10, 2023
Merged
@lambdageek @thaystg
add debugger test
6967b79 
Co-Authored-By: Thays Grazia <thaystg@gmail.com>
@lambdageek lambdageek requested a review from radical as a code owner November 11, 2023 17:56
This was referenced Nov 11, 2023
Open
Closed
Closed
@lambdageek
Copy link
Member Author

/backport to release/8.0-staging

@github-actions GitHub Actions
Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6843025042

@lambdageek lambdageek merged commit 38463e2 into dotnet:main Nov 12, 2023
lambdageek added a commit that referenced this pull request Nov 13, 2023
@github-actions @lambdageek @thaystg
[release/8.0-staging] [mono] catch the case of updated methods in mon…
0087f0b 
…o_debug_lookup_source_location (#94612)

Backport of #94540 to release/8.0-staging

If a newly added method is in a stack trace, look it up in the EnC debug information, if available.

Related to #93860

* Add new crashing test

* [mono] catch the case of updated methods in mono_debug_lookup_source_location

Co-authored-by: Aleksey Kliger <alklig@microsoft.com>
Co-authored-by: Thays Grazia <thaystg@gmail.com>
jeromelaban added a commit to unoplatform/Uno.DotnetRuntime.WebAssembly that referenced this pull request Nov 14, 2023
@jeromelaban
Merge pull request #140 from unoplatform/dev/jela/pr-94540
5708d38 
fix: Apply patch for dotnet/runtime#94540
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

@radical radical Awaiting requested review from radical

1 more reviewer

@vargaz vargaz vargaz approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@lambdageek lambdageek

Labels
area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lambdageek @vargaz