From f06da6d8913fccb7de836ebe73a0bc6a7e6f7985 Mon Sep 17 00:00:00 2001 From: Noah Falk Date: Sat, 25 Oct 2025 14:48:14 -0700 Subject: [PATCH 1/4] Add MethodDesc comments Added clarifying comments on some MethodDesc APIs related to TransientIL and diagnostics. --- src/coreclr/vm/method.inl | 13 +++++++++++++ src/coreclr/vm/prestub.cpp | 3 +++ 2 files changed, 16 insertions(+) diff --git a/src/coreclr/vm/method.inl b/src/coreclr/vm/method.inl index b74fcc29a68512..b6f0d81431fa65 100644 --- a/src/coreclr/vm/method.inl +++ b/src/coreclr/vm/method.inl @@ -109,6 +109,9 @@ inline PTR_DynamicMethodDesc MethodDesc::AsDynamicMethodDesc() return dac_cast(this); } +// NOTE: Not all methods that create IL at runtime are considered dynamic methods. This only returns TRUE +// for methods represented by DynamicMethodDesc. Transient IL (see TryGenerateTransientILImplementation) also +// generates IL at runtime but returns FALSE here. inline bool MethodDesc::IsDynamicMethod() { LIMITED_METHOD_CONTRACT; @@ -121,12 +124,22 @@ inline bool MethodDesc::IsLCGMethod() return ((mcDynamic == GetClassification()) && dac_cast(this)->IsLCGMethod()); } +// NOTE: This method only detects the subset of ILStubs that are internally represented using DynamicMethodDesc. +// There are other methods compiled from IL generated at runtime via ILStubLinker that still return FALSE here. +// See TryGenerateTransientILImplementation. inline bool MethodDesc::IsILStub() { WRAPPER_NO_CONTRACT; return ((mcDynamic == GetClassification()) && dac_cast(this)->IsILStub()); } +// This method is intended to identify runtime defined methods that don't have a corresponding Metadata or +// LCG definition so they can be filtered from diagnostic introspection (stacktraces, code viewing, stepping, etc). +// Partly this is this is a user experience consideration to preserve the abstraction users would expect based on +// source code and assembly contents. Partly it is also a technical limitation that many parts of +// diagnostics don't know how to work with methods that aren't backed by metadata and IL in a module. +// Currently this method only triages methods whose code was generated from IL. We rely on filtering that occurs implictly +// elsewhere to avoid including other kinds of stubs like prestubs or unboxing stubs. inline bool MethodDesc::IsDiagnosticsHidden() { WRAPPER_NO_CONTRACT; diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp index 4eed9884cffb69..a3544a20b119a0 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -1057,6 +1057,9 @@ bool MethodDesc::TryGenerateTransientILImplementation(DynamicResolver** resolver { STANDARD_VM_CONTRACT; + // When adding new methods implemented by Transient IL, consider if MethodDesc::IsDiagnosticsHidden() needs to be + // updated as well. + if (TryGenerateAsyncThunk(resolver, methodILDecoder)) { return true; From 5c8692bfdd4b4e831a6b53fa8ee6ea7cac9ec39b Mon Sep 17 00:00:00 2001 From: Noah Falk Date: Sat, 25 Oct 2025 15:21:52 -0700 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/coreclr/vm/method.inl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/method.inl b/src/coreclr/vm/method.inl index b6f0d81431fa65..697516efaa7e91 100644 --- a/src/coreclr/vm/method.inl +++ b/src/coreclr/vm/method.inl @@ -135,10 +135,10 @@ inline bool MethodDesc::IsILStub() // This method is intended to identify runtime defined methods that don't have a corresponding Metadata or // LCG definition so they can be filtered from diagnostic introspection (stacktraces, code viewing, stepping, etc). -// Partly this is this is a user experience consideration to preserve the abstraction users would expect based on +// Partly this is a user experience consideration to preserve the abstraction users would expect based on // source code and assembly contents. Partly it is also a technical limitation that many parts of // diagnostics don't know how to work with methods that aren't backed by metadata and IL in a module. -// Currently this method only triages methods whose code was generated from IL. We rely on filtering that occurs implictly +// Currently this method only triages methods whose code was generated from IL. We rely on filtering that occurs implicitly // elsewhere to avoid including other kinds of stubs like prestubs or unboxing stubs. inline bool MethodDesc::IsDiagnosticsHidden() { From 6dbdcc6eb63d1e39834304997506a0080615cea5 Mon Sep 17 00:00:00 2001 From: Noah Falk Date: Sat, 25 Oct 2025 23:26:32 -0700 Subject: [PATCH 3/4] Hopefully improving the diagnostics comment --- src/coreclr/vm/method.inl | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/coreclr/vm/method.inl b/src/coreclr/vm/method.inl index 697516efaa7e91..22f9a1349883b9 100644 --- a/src/coreclr/vm/method.inl +++ b/src/coreclr/vm/method.inl @@ -133,15 +133,26 @@ inline bool MethodDesc::IsILStub() return ((mcDynamic == GetClassification()) && dac_cast(this)->IsILStub()); } -// This method is intended to identify runtime defined methods that don't have a corresponding Metadata or -// LCG definition so they can be filtered from diagnostic introspection (stacktraces, code viewing, stepping, etc). -// Partly this is a user experience consideration to preserve the abstraction users would expect based on -// source code and assembly contents. Partly it is also a technical limitation that many parts of -// diagnostics don't know how to work with methods that aren't backed by metadata and IL in a module. -// Currently this method only triages methods whose code was generated from IL. We rely on filtering that occurs implicitly -// elsewhere to avoid including other kinds of stubs like prestubs or unboxing stubs. +// This method is intended to identify methods that aren't shown in diagnostic introspection (stacktraces, +// code viewing, stepping, etc). Partly this is a user experience consideration to preserve the +// abstraction users would expect based on source code and assembly contents. Partly it is also a technical +// limitation that many parts of diagnostics don't know how to work with methods that aren't backed by +// metadata and IL in a module. Currently this method only triages methods whose code was generated from IL. inline bool MethodDesc::IsDiagnosticsHidden() { + // Although good user experience can be subjective these are guidelines: + // - We don't want stacktraces to show extra frames when the IL only shows a single call was made. If our runtime stackwalker + // is going to include multiple frames pointing at different MethodDescs then all but one of them should be hidden. + // - In most cases when multiple MethodDescs occur for the same IL call, one of them is a MethodDesc that + // compiled original IL defined in the module (for its default code version at least). That is the one we want to show + // and the rest should be hidden. + // - In other cases the user defines methods in their source code but provides no implementation (for + // example interop calls, Array methods, Delegate.Invoke, or UnsafeAccessor). Ideally a filtered stacktrace would include exactly + // one frame for these calls as well but we haven't always done this consistently. For calls that redirect to another managed method users + // tolerate if the runtime-implemented frame is missing because they can still see the managed target method. + + // NOTE: Currently this method doesn't filter out unboxing stubs although it seems like it should. I haven't identified a concrete + // scenario that produces a bad user experience but more exploration might find one. WRAPPER_NO_CONTRACT; return IsILStub() || IsAsyncThunkMethod(); } From e25ce8d97424f9fb6c7c949dd17a5ea5ba51bc5e Mon Sep 17 00:00:00 2001 From: Noah Falk Date: Sun, 26 Oct 2025 16:11:47 -0700 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Jan Kotas --- src/coreclr/vm/method.inl | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/coreclr/vm/method.inl b/src/coreclr/vm/method.inl index 22f9a1349883b9..4c22811b5f41eb 100644 --- a/src/coreclr/vm/method.inl +++ b/src/coreclr/vm/method.inl @@ -137,7 +137,7 @@ inline bool MethodDesc::IsILStub() // code viewing, stepping, etc). Partly this is a user experience consideration to preserve the // abstraction users would expect based on source code and assembly contents. Partly it is also a technical // limitation that many parts of diagnostics don't know how to work with methods that aren't backed by -// metadata and IL in a module. Currently this method only triages methods whose code was generated from IL. +// metadata and IL in a module. inline bool MethodDesc::IsDiagnosticsHidden() { // Although good user experience can be subjective these are guidelines: @@ -151,10 +151,8 @@ inline bool MethodDesc::IsDiagnosticsHidden() // one frame for these calls as well but we haven't always done this consistently. For calls that redirect to another managed method users // tolerate if the runtime-implemented frame is missing because they can still see the managed target method. - // NOTE: Currently this method doesn't filter out unboxing stubs although it seems like it should. I haven't identified a concrete - // scenario that produces a bad user experience but more exploration might find one. WRAPPER_NO_CONTRACT; - return IsILStub() || IsAsyncThunkMethod(); + return IsILStub() || IsAsyncThunkMethod() || IsWrapperStub(); } inline BOOL MethodDesc::IsQCall()