[clr-interp] Tracing fixes. #122467
[clr-interp] Tracing fixes. #122467
Conversation
…ta, and update event_pipe to be a bit more resilient to interpreter codegen patterns - Fix rundown to be able to walk the interpreter code heaps - Fix the CodeHeapIterator implementation to have a correct move constructor/move operator - Tweak the eventpipe code to be able to ignore stack frames without a control_pc - Adjust rundownvalidation test to print out what condition failed. This fixes BinderTracingTest.ResolutionFlow and rundownvalidation
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
There was a problem hiding this comment.
Pull request overview
This PR fixes interpreter-related tracing issues by enabling rundown to walk interpreter code heaps and making event pipe more resilient to interpreter codegen patterns.
- Adds support for iterating over interpreter code heaps during rundown in addition to JIT heaps
- Fixes CodeHeapIterator to support move semantics correctly with proper resource management
- Updates event pipe stack walking to handle frames without valid control_pc values
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/tracing/eventpipe/rundownvalidation/rundownvalidation.cs | Adds diagnostic logging to print out which rundown event conditions failed |
| src/coreclr/vm/eventtrace.cpp | Extends rundown to iterate both JIT and interpreter code heaps, and fixes code version checking to work with interpreter code |
| src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.cpp | Updates stack walk callback to skip frames without valid control_pc and adds debug assertions for expected scenarios |
| src/coreclr/vm/codeman.h | Refactors CodeHeapIterator to use RAII holder for proper resource management and adds m_codeType field to track code type |
| src/coreclr/vm/codeman.cpp | Implements the RAII holder for CodeHeapIterator and adds logic to distinguish between JIT and interpreter code headers |
| CodeHeapIterator heapIterator = | ||
| #ifdef FEATURE_INTERPRETER | ||
| (i == 0) ? | ||
| #endif | ||
| ExecutionManager::GetEEJitManager()->GetCodeHeapIterator(pLoaderAllocatorFilter) | ||
| #ifdef FEATURE_INTERPRETER | ||
| : | ||
| #endif | ||
| ExecutionManager::GetInterpreterJitManager()->GetCodeHeapIterator(pLoaderAllocatorFilter); |
There was a problem hiding this comment.
When FEATURE_INTERPRETER is not defined, the ternary operator starting on line 4999 becomes syntactically incorrect. The code would expand to just the first branch without the conditional, resulting in a compilation error. The entire ternary expression (lines 4997-5005) needs to be wrapped in FEATURE_INTERPRETER guards, or restructured to use if/else statements that properly handle both FEATURE_INTERPRETER being defined and undefined.
| } | ||
| #endif | ||
| // With FUNCTIONSONLY flag, we may hit frames without a meaningful control_pc, but with a valid MethodDesc. | ||
| // there is no point in reporting those frames as ep_stack_contents_append doesn't actually record the function |
There was a problem hiding this comment.
The comment uses lowercase "there" at the start of the sentence. It should be capitalized: "There is no point in reporting those frames..."
| // there is no point in reporting those frames as ep_stack_contents_append doesn't actually record the function | |
| // There is no point in reporting those frames as ep_stack_contents_append doesn't actually record the function |
| EECodeGenManagerReleaseIteratorHolder& operator=(EECodeGenManagerReleaseIteratorHolder&& other) | ||
| { | ||
| LIMITED_METHOD_CONTRACT; | ||
|
|
||
| if (this != &other) | ||
| { | ||
| m_manager = other.m_manager; | ||
| other.m_manager = NULL; | ||
| } | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
The move assignment operator should release the currently held manager before taking ownership of the new one. Without this, if the holder already owns a manager, it will be leaked when reassigned. Consider calling the destructor logic (ReleaseIterator) on the current m_manager before assigning from other.
1 participant
Update rundown to dump the interpreter generated code data, and update event_pipe to be a bit more resilient to interpreter codegen patterns
This fixes BinderTracingTest.ResolutionFlow and rundownvalidation