Improve the interpreter internal diagnostic capabilities #116162
Conversation
- Duplicate the concept of MethodSet's from the JIT - Methods can be specified with granularity of by method name, class, instantiation, etc. - This works for DOTNET_Interpreter, DOTNET_InterpDump and DOTNET_InterpHalt - Improve printing of method names to be very nearly equivalent to that of the JIT. This will include signatures/instantiations in all method printing - The only differences are where the JIT has a concept of var_type, and the interpreter just uses CorInfoType. I believe the ouput is very nearly identical, but I'm not 100% confident. - Add a new config switch DOTNET_InterpList, when enabled this will list all methods that go through the Interpreter compiler - Since this is an integer, add support for arbitrary integer config knobs in the interpreter compiler
There was a problem hiding this comment.
Pull Request Overview
Enhance the interpreter’s diagnostics by refining method name printing (mirroring JIT functionality), duplicating MethodSet support for configurable method matching, and adding integer configuration for method listing.
- Refactored method name printing in naming.cpp and compiler.cpp for detailed signature output.
- Introduced MethodSet (in methodset.cpp and interpretershared.h) to support fine‐grained method specification and matching.
- Added new configuration switches and integer config support in interpconfig* files for diagnostic capabilities.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/interpreter/naming.cpp | Updated functions to build method names for diagnostics. |
| src/coreclr/interpreter/methodset.cpp | Implements MethodSet parsing and pattern matching. |
| src/coreclr/interpreter/interpretershared.h | Declares new types and MethodSet for interpreter diagnostics. |
| src/coreclr/interpreter/interpconfigvalues.h | Introduces configuration macros for MethodSet and integer config. |
| src/coreclr/interpreter/interpconfig.h | Updates configuration getters and setters for interpreter configs. |
| src/coreclr/interpreter/interpconfig.cpp | Initializes and destroys new configuration values. |
| src/coreclr/interpreter/eeinterp.cpp | Updates interpretation conditions to use MethodSet matching. |
| src/coreclr/interpreter/datastructs.h | Adds array-grow utilities supporting dynamic array operations. |
| src/coreclr/interpreter/compiler.h | Declares enhanced method name printing functions and member updates. |
| src/coreclr/interpreter/compiler.cpp | Uses improved method printing for verbose debug output. |
| src/coreclr/interpreter/CMakeLists.txt | Includes new source files for the interpreter diagnostic enhancements. |
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| // and this class takes ownership of the string. | ||
| // host - Pointer to host interface | ||
| // | ||
| void MethodSet::initialize(const char* listFromConfig) |
There was a problem hiding this comment.
This is near verbatim copy of the code from the JIT.
(I am just pointing it out. Like on previous instances of code duplication - it does not need to be addressed in this PR.)
There was a problem hiding this comment.
Agreed. Most of this PR is approximately verbatim. Notably, the method name printing is ALSO nearly verbatim, but it has the advantage that it is not dependent on any significant JIT headers. The only thing it depends on is a growable array type. I think it is likely we would be able to share most of the logic of both of these between the JIT/Interpreter, but we haven't set up any real sharing between the two yet.
There was a problem hiding this comment.
Would it make sense to put a marker comment at the top of the file to say where it was copied from? There's a risk of the information becoming stale but if there's an expectation that it will eventually get deduplicated it would likely help with the future effort.
…avidwrighton/runtime into InterpreterLoggingImprovements
There was a problem hiding this comment.
The only differences are where the JIT has a concept of var_type, and the interpreter just uses CorInfoType. I believe the ouput is very nearly identical, but I'm not 100% confident.
Left some feedback that should make this match more, but we might consider changing the JIT's printing to print the CorInfoType names instead. Not sure how breaking we would consider a change like that.
|
@jakobbotsch honestly, I'd prefer to have the JIT match these, but yes, you've identified what I think are all the issues I could find. |
|
I am fine with changing the JIT names to match the names here, if we are ok with that small break. But I do think we should make sure they match each other exactly, regardless of which names we end up with. |
|
@jakobbotsch I've updated the JIT to match the interpreter now. I've also spoken to @jkotas and he isn't concerned about this being a breaking change. (It's not considered a breaking change for customers... it might be slightly surprising to jit devs, but I expect you to all adjust very quickly.) |
|
cc @dotnet/jit-contrib -- types printed in signatures (and matched when specifying method set names) are changing to use the more familiar names instead of the JIT type names. E.g. |
Uh oh!
There was an error while loading. Please reload this page.