VM: Include generic instantiations in stack frames #96440

hez2010 · 2024-01-03T09:32:17Z

We currently don't have generic instantiations in the stack frames, which sometimes makes it hard to locate and identify an exception by only seeing the stack trace without actually debugging the program.

Adding generic instantiations in the stack frames to resolve this issue.

Test code:

using System.Diagnostics;

new Foo<int, string>().Bar<float, string>();

class Foo<T, U>
{
    public void Bar<V, W>()
    {
        Console.WriteLine(new StackTrace());
    }
}

Before:

   at Foo`2[T,U].Bar[V,W]()
   at Program.<Main>$(String[] args)

After:

   at Foo`2[System.Int32,System.String].Bar[System.Single,System.String]()
   at Program.<Main>$(String[] args)

This can also help const generics (#89730) if we add the support for it in the future, as displaying the const value of const type arguments in the stack trace could be really helpful while diagnosing an exception.

ghost · 2024-01-03T09:32:28Z

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

Issue Details

We currently don't have generic instantiations in the stack frames, which sometimes makes it hard to locate an exception by only seeing the stack trace without actually debugging the program.

Adding generic instantiations in the stack frames to resolve this issue.

Test code:

using System.Diagnostics;

new Foo<int, string>().Bar<float, string>();

class Foo<T, U>
{
    public void Bar<V, W>()
    {
        Console.WriteLine(new StackTrace());
    }
}

Before:

   at Foo`2[T,U].Bar[V,W]()
   at Program.<Main>$(String[] args)

After:

   at Foo`2[[System.Int32, System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Bar[Single,__Canon]()
   at Program.<Main>$(String[] args)

For a shared generic argument, it returns the canonical type as is because we didn't encode the exact type info in the crawl stack frame. But still better then nothing.

Author:	hez2010
Assignees:	-
Labels:	`area-System.Diagnostics`, `community-contribution`
Milestone:	-

src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs

src/coreclr/vm/debugdebugger.cpp

src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs

src/coreclr/vm/jitinterface.cpp

hez2010 · 2024-02-25T08:21:24Z

Now it's ready for review.

hez2010 · 2024-03-14T16:34:44Z

I start to think that reporting System.__Canon is fine because we are already doing this for fatal internal clr errors.
Example:

   Fatal error. Internal CLR error. (0x80131506)
 -   at System.Collections.Generic.Dictionary`2[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Resize(Int32, Boolean)
 -   at System.Collections.Generic.Dictionary`2[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].TryInsert(System.__Canon, System.__Canon, System.Collections.Generic.InsertionBehavior)
 at MyCustomDictionaryExtensions.MergeDictionary(System.Collections.Generic.Dictionary`2<System.String,System.String>, System.Collections.Generic.Dictionary`2<System.String,System.String>, Boolean)

MichalStrehovsky · 2024-03-15T07:03:03Z

is fine because we are already doing this for fatal internal clr errors.

Fatal CLR errors capture details that are important for the developers of the runtime because they're runtime bugs. They are not expected to be hit/consumed/processed by users. Mono has entirely different outputs for fatal runtime errors with even more confusing details. Stacks in fatal errors serve different purposes for different audiences.

noahfalk

The user facing behavior seemed fine to me though I still want to nail down what happens for optimized shared generic code frames.

In terms of the correctness and performance of the type system generics and reflection code I'm guessing @davidwrighton or @jkotas is probably a better person than I to look at that part.

noahfalk · 2024-03-15T05:49:53Z

...raries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/Environment.StackTrace.cs

@@ -40,27 +46,27 @@ public void StackTraceTest()
            }
        }

-        [MethodImpl(MethodImplOptions.NoInlining)]
+        [MethodImpl(MethodImplOptions.NoOptimization | MethodImplOptions.NoInlining)]


I don't want to weaken the existing stackwalking test by only validating it against unoptimized code. You could create some alternate frames that are unoptimized and call those instead if the switch is set.

(The same thing applies in the other tests)

I also think we should explicitly have a test that demonstrates the behavior of a stack trace that occurs in optimized shared generic code where the shared generics context is no longer in scope. Even if it doesn't have exact type naming we still want to ensure it degrades gracefully rather than doing anything bad (like crashing).

Was there ever an agreed upon behavior for what text gets shown for the generic parameters in an optimized shared generic frame? I believe the text "System.__Canon" has already leaked out in a few other places such as the error handling text @hez2010 mentioned or it has shown up in VS debugger callstacks for many years when debugging optimized generic code:

I feel like the cat is already out of the bag in terms of the revealing that type name so using it here is aligned with precedent elsewhere. I'd also be fine reverting to use the type parameter name if you think its worth trying to get the cat back into the bag @jkotas?

We do not leak __Canon through public APIs. We have always treated leaking __Canon through public APIs as bug.

I agree with #96440 (comment) .

noahfalk · 2024-03-15T06:06:35Z

src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs

@@ -116,8 +116,16 @@ public static IEnumerable<object[]> ToString_TestData()
            yield return new object[] { new StackFrame(), "MoveNext at offset {offset} in file:line:column {fileName}:{lineNumber}:{column}" + Environment.NewLine };
            yield return new object[] { new StackFrame("FileName", 1, 2), "MoveNext at offset {offset} in file:line:column FileName:1:2" + Environment.NewLine };
            yield return new object[] { new StackFrame(int.MaxValue), "<null>" + Environment.NewLine };
-            yield return new object[] { GenericMethod<string>(), "GenericMethod<T> at offset {offset} in file:line:column {fileName}:{lineNumber}:{column}" + Environment.NewLine };
-            yield return new object[] { GenericMethod<string, int>(), "GenericMethod<T,U> at offset {offset} in file:line:column {fileName}:{lineNumber}:{column}" + Environment.NewLine };
+            if (AppContext.TryGetSwitch("Switch.System.Diagnostics.StackTrace.ShowGenericInstantiations", out var showGenericInstantiations) && showGenericInstantiations)


I see this code that checks for the switch, but is there something which is setting the switch? I want to ensure the test is explicitly validating the behavior both with the switch enabled and disabled.

noahfalk · 2024-03-15T06:14:56Z

src/coreclr/vm/jitinterface.cpp

-                fProfilerRequiresGenericsContextForEnterLeave = TRUE;
-            }
-            END_PROFILER_CALLBACK();
+            methInfo->options = CorInfoOptions(methInfo->options|CORINFO_GENERICS_CTXT_KEEP_ALIVE);


@AndyAyersMS - Do you know if the JIT is currently preserving the generic context when optimizations are disabled somehow? I expected this would be something we already do but looking around at how CORINFO_GENERICS_CTXT_KEEP_ALIVE is used now that didn't seem to be the case.

It is not.

Generally speaking the generic context is only kept alive if we see the CORINFO_GENERICS_CTXT_KEEP_ALIVE flag from the jithost, or if there is a runtime lookup that uses the context.

src/coreclr/vm/jitinterface.cpp

jkotas · 2024-03-15T10:32:25Z

the correctness and performance of the type system generics and reflection code I'm guessing @davidwrighton or @jkotas is probably a better person than I to look at that part.

Reliability of this opt-in switch is my top concern. This change takes a dependency on generic context being tracked correctly in new places. If we want do productize this switch, we need to have a test strategy for how we ensure that it is reliable.

hez2010 · 2024-04-06T09:54:53Z

Seems that something around EH has changed after my last main branch merging resulting in GC hole issue happening again.
We need to fetch the inst under cooperative gc mode after fetching the generic context immediately, otherwise the GC may move the generic context and results in AV.

hez2010 · 2024-04-06T14:45:48Z

While quite interesting that the AV only happens on Windows x86. Investigating.

hez2010 · 2024-04-08T15:18:35Z

I was trying to load the exact MethodDesc while initializing the crawl frame, but loading it can possibly trigger a throw or a GC as it may need to create a new MethodDesc, so it's not the right place to load the exact MethodDesc.

@jkotas Do you have any suggestion on this?

hez2010 · 2024-04-08T15:24:08Z

src/coreclr/vm/stackwalk.cpp

+                    InstantiatedMethodDesc *pInstMD = InstantiatedMethodDesc::FindLoadedInstantiatedMethodDesc(th.GetMethodTable(), pConstructedFunc->GetMemberDef(), Instantiation(), false);
+
+                    if (pInstMD != NULL)
+                    {
+                        pConstructedFunc = pInstMD;
+                    }


It should really be

pConstructedFunc = InstantiatedMethodDesc::FindOrCreateExactClassMethod(th.GetMethodTable(), pConstructedFunc);

but this would possibly throw or trigger a GC.

jkotas · 2024-04-08T16:43:51Z

@jkotas Do you have any suggestion on this?

Avoid changing behavior of regular stackwalk - it would introduce both reliability bugs and performance regressions. Any extra work should be done only during the stackwalk that produces the diagnostic stacktrace and only when the special mode is enabled.

This goes back to my comment #96440 (comment) . I will leave it to @dotnet/area-system-diagnostics area owners on they would like to approach it.

In the current shape, the feature added by this PR addresses very niche scenario (it only helps when running debug build outside debugger, and one has to opt-in). The cost to productize it just may not be worth it.

noahfalk · 2024-04-15T22:01:09Z

Hi @hez2010, I feel bad saying this after the PR has been open for so long but I don't think this one is going to work out. I was investigating and talking a bit with Jan and Andy - the problem looks deeper than just testing. In order for this approach to work the generic context needs to be reliable always and that isn't a guarantee the JIT currently makes in its tracking info. Even if great testing found every case where it doesn't currently work, its a cost both now and in the future for the entire JIT codebase to enforce stronger rules around that tracking. This feature appears too niche to make that tradeoff worthwhile. Since we didn't see any clear path to overcoming the reliability issue I think our best option is to close this PR. Sorry to be the bearer of bad news.

cc @jkotas @AndyAyersMS

hez2010 · 2024-04-16T00:59:28Z

Okay. I'm closing the PR.
Thanks for all the advice and review!

Including generic instantiations in stack frames

1e21c17

ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 3, 2024

dotnet-issue-labeler bot added the area-System.Diagnostics label Jan 3, 2024

build-analysis bot mentioned this pull request Jan 3, 2024

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

2 tasks

Fix another test

44f2275

build-analysis bot mentioned this pull request Jan 3, 2024

Tracking issue for "WORKLOAD TIMED OUT" #90309

Closed

jkotas reviewed Jan 3, 2024

View reviewed changes

src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs Outdated Show resolved Hide resolved

jkotas reviewed Jan 3, 2024

View reviewed changes

src/coreclr/vm/debugdebugger.cpp Show resolved Hide resolved

jkotas reviewed Jan 3, 2024

View reviewed changes

src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs Outdated Show resolved Hide resolved

Always keep generic context alive

250472a

jkotas reviewed Jan 3, 2024

View reviewed changes

src/coreclr/vm/jitinterface.cpp Outdated Show resolved Hide resolved

tommcdon requested a review from mikem8361 January 3, 2024 16:44

hez2010 added 3 commits January 4, 2024 02:26

Only keep generic context when optimization is disabled

dbc1445

Add support for exception stacktrace as well

a804a43

Update tests

dc44a0d

Try to fix the GC hole

869f439

hez2010 force-pushed the feature/generic-stacktrace branch from b411c6f to 869f439 Compare January 4, 2024 03:32

hez2010 added 2 commits January 4, 2024 12:59

GC hole fixes take 2

fbe823e

Protect oRef

2048e41

hez2010 force-pushed the feature/generic-stacktrace branch from aa027dd to 2048e41 Compare January 4, 2024 04:20

hez2010 marked this pull request as ready for review February 25, 2024 08:21

Code cleanup

8e9c556

This was referenced Feb 26, 2024

[mt][browser] HttpClient_CancelInDifferentThread failing with operation cancelled #98101

Open

profiler\\multiple\\multiple\\multiple.cmd failing on windows arm64 #98817

Closed

noahfalk reviewed Mar 15, 2024

View reviewed changes

hez2010 added 2 commits March 29, 2024 17:57

Merge branch 'main' into feature/generic-stacktrace

6cc5a6a

Fix unmatched braces

bd83c8a

This was referenced Mar 29, 2024

[browser][MT] TIMED_OUT but 0 was expected #99888

Closed

System.Runtime.InteropServices.JavaScript.Tests.JSImportTest.MissingImport test fails on CI leg: browser-wasm linux Release LibraryTests_Threading #100421

Closed

hez2010 added 3 commits April 7, 2024 00:03

Try fix GC hole

5465ff4

Fix contract

1a6c019

Fix debug assert

58ec764

hez2010 force-pushed the feature/generic-stacktrace branch from 4f3b326 to 58ec764 Compare April 6, 2024 16:31

build-analysis bot mentioned this pull request Apr 6, 2024

ComInterfaceGenerator.Unit.Tests failed on linux-x64 running interpreter #100683

Closed

Don't create new MT

657e2b3

hez2010 commented Apr 8, 2024

View reviewed changes

This was referenced Apr 8, 2024

GC\Regressions\v2.0-beta2\452950\452950\452950.cmd failing on Mono minijit Windows x64 #99729

Open

at GC_Regressions._v2_0_beta2_452950_452950_452950_._v2_0_beta2_452950_452950_452950_cmd() #100174

Closed

hez2010 closed this Apr 16, 2024

github-actions bot locked and limited conversation to collaborators May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VM: Include generic instantiations in stack frames #96440

VM: Include generic instantiations in stack frames #96440

hez2010 commented Jan 3, 2024 •

edited

Loading

ghost commented Jan 3, 2024

hez2010 commented Feb 25, 2024

hez2010 commented Mar 14, 2024 •

edited

Loading

MichalStrehovsky commented Mar 15, 2024

noahfalk left a comment

noahfalk Mar 15, 2024

noahfalk Mar 15, 2024

jkotas Mar 15, 2024

noahfalk Mar 15, 2024

noahfalk Mar 15, 2024

AndyAyersMS Mar 15, 2024

jkotas commented Mar 15, 2024

hez2010 commented Apr 6, 2024

hez2010 commented Apr 6, 2024 •

edited

Loading

hez2010 commented Apr 8, 2024 •

edited

Loading

hez2010 Apr 8, 2024

jkotas commented Apr 8, 2024

noahfalk commented Apr 15, 2024

hez2010 commented Apr 16, 2024

VM: Include generic instantiations in stack frames #96440

VM: Include generic instantiations in stack frames #96440

Conversation

hez2010 commented Jan 3, 2024 • edited Loading

ghost commented Jan 3, 2024

hez2010 commented Feb 25, 2024

hez2010 commented Mar 14, 2024 • edited Loading

MichalStrehovsky commented Mar 15, 2024

noahfalk left a comment

Choose a reason for hiding this comment

noahfalk Mar 15, 2024

Choose a reason for hiding this comment

noahfalk Mar 15, 2024

Choose a reason for hiding this comment

jkotas Mar 15, 2024

Choose a reason for hiding this comment

noahfalk Mar 15, 2024

Choose a reason for hiding this comment

noahfalk Mar 15, 2024

Choose a reason for hiding this comment

AndyAyersMS Mar 15, 2024

Choose a reason for hiding this comment

jkotas commented Mar 15, 2024

hez2010 commented Apr 6, 2024

hez2010 commented Apr 6, 2024 • edited Loading

hez2010 commented Apr 8, 2024 • edited Loading

hez2010 Apr 8, 2024

Choose a reason for hiding this comment

jkotas commented Apr 8, 2024

noahfalk commented Apr 15, 2024

hez2010 commented Apr 16, 2024

hez2010 commented Jan 3, 2024 •

edited

Loading

hez2010 commented Mar 14, 2024 •

edited

Loading

hez2010 commented Apr 6, 2024 •

edited

Loading

hez2010 commented Apr 8, 2024 •

edited

Loading