Fix EH profiler notifications #123564
Fix EH profiler notifications #123564
Conversation
The new EH was incorrectly notifying exception search and unwind function enter / leave in case of funclets. For each funclet, it was reporting enter and leave of the parent and then it reported it again for the parent itself. This was different from the old EH behavior where only one enter / leave was reported for each method. This change rectifies it. Close dotnet#123351
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the new exception handling implementation where profiler notifications for exception search and unwind function enter/leave were being reported multiple times for methods with funclets (e.g., finally blocks). The fix ensures that each method is only reported once per exception handling pass, matching the behavior of the old exception handling system.
Changes:
- Added a check to skip duplicate profiler notifications when transitioning between funclets and their parent methods
| if (pThis->m_crawl.IsFunclet() && (pMD == pExInfo->m_pMDToReportFunctionLeave)) | ||
| { | ||
| // In case of a funclet, the pMD represents the parent of the funclet. We only want to report entering and leaving | ||
| // the method once. So we ignore transitions from the funclet to the parent method or the funclet into another funclet | ||
| // of the same parent method. | ||
| return; | ||
| } |
There was a problem hiding this comment.
Consider adding a test case that validates this fix by checking that ExceptionSearchFunctionEnter/ExceptionSearchFunctionLeave and ExceptionUnwindFunctionEnter/ExceptionUnwindFunctionLeave are called exactly once per method when an exception is thrown from within a try-finally block. This would prevent regression of issue #123351.
A test could be structured similar to the scenario in the issue: a method with a try-finally block that throws from the finally, with a profiler that tracks and validates the count of Enter/Leave pairs for each FunctionID.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
This request won't work well since this PR originates in a personal fork. If you want copilot to help with a PR like this, it works best if the PR is started by copilot. It is a known github copilot workflow limitation.
I am starting all my PRs as copilot PRs for this reason. Once the PR is opened you can push changes into it just like you would into a regular PR, but you can also ask copilot to help seamlessly that is really nice.
There was a problem hiding this comment.
I am half done with a test that I've started to create myself after your initial comment. I'll push it here once done.
There was a problem hiding this comment.
This request won't work well
rats - thanks for the info about it though!
I'll push it here once done.
I'll just wait for that, thanks!
|
Add a test for these EH notifications under src\tests\profile ? cc @dotnet/dotnet-diag-contrib |
|
Agreed a test case would be useful. Lets see how well copilot does at creating one! |
3 participants
The new EH was incorrectly notifying exception search and unwind function enter / leave in case of funclets. For each funclet, it was reporting enter and leave of the parent and then it reported it again for the parent itself.
This was different from the old EH behavior where only one enter / leave was reported for each method.
This change rectifies it.
Close #123351