Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NativeAOT] Natvis visualization for threadstatic variables. #90473

Merged
merged 11 commits into from
Aug 14, 2023

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Aug 13, 2023

Fixes: #89227

The change implements visualization for threadstatic variables in common cases.

The essence of the change:

=== Uninlined case:

  • For reliability reasons the threadstatic storage is all managed in 8.0 (vs. mix of native/managed in 7.0).
    Made corresponding adjustments for indexing of native vs. managed arrays in the natvis.
  • Adjusted natvis for renames and removal of redundant m_numThreadLocalModuleStatics

=== Inlined case

  • Since TypeThreadStaticIndexNode is needed for natvis, start emitting TypeThreadStaticIndexNode for inlined types as well. (we stopped emitting that for inlined threadstatics since the index is not needed at run time)
  • Added support in natvis to detect inlined case and interpret TypeThreadStaticIndexNode accordingly
    (that is mostly to treat the offset not as an index of the storage block, but an offset within the storage block)

=== overall

  • added more comments in a few places
  • refactored natvis a bit to make it a bit less painful to read

@ghost
Copy link

ghost commented Aug 13, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes: #89227

Author: VSadov
Assignees: VSadov
Labels:

area-NativeAOT-coreclr

Milestone: -

@VSadov
Copy link
Member Author

VSadov commented Aug 13, 2023

This is how threadstatics visualization looks in optimized builds.
( S_P_CoreLib_System_Threading_Thread::__THREADSTATICINDEX.Get() is the incantation to get to threadstatics of
System.Threading.Thread)

image

@VSadov
Copy link
Member Author

VSadov commented Aug 13, 2023

This is the same view using Debug (unoptimized build)

There are no differences, but a curious user can do ConsoleApp32_Program::__THREADSTATICINDEX.IsInlined() and see if the type uses inlined storage.

image

@VSadov
Copy link
Member Author

VSadov commented Aug 13, 2023

Limitations:

  • generic types are not supported
    This never worked, not even for Something<int>, for which we do emit the TypeThreadStaticIndexNode and have all information necessary.

Natvis is a terrible language for doing anything complex and appears to be unable to parse nested generic types which arise when we need to visualize for a generic instantiation.

A typical failure looks like the following (both 7.0 and 8.0):

image

This is visualizing System.Buffers.TlsOverPerCoreLockedStacksArrayPool<Char>

If you notice S_P_CoreLib_System_Buffers_TlsOverPerCoreLockedStacksArrayPool_1<Char>::__THREADSTATICINDEX is present, but it's Get() fails to light up.

The natvis diagnostics contains

    Error while evaluating '*(__type__THREADSTATICSS_P_CoreLib_System_Buffers_TlsOverPerCoreLockedStacksArrayPool_1<Char>>**)(&(*GetStorage()).values)[ClassIndex]' in the context of type 'ConsoleApp32.exe!__ThreadStaticHelper<__type__THREADSTATICSS_P_CoreLib_System_Buffers_TlsOverPerCoreLockedStacksArrayPool_1<Char>>'.

The unbalanced extra > in TlsOver<Char>>** appears to be a result of incorrect unwrapping of a nested generic helper. Possibly a bug in the natvis parser.
We pattern match __ThreadStaticHelper<*> and use the match in ($T1*), and in a generic case, T1 ends up having an extra >.

public void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb)
{
sb.Append(_type != null ? nameMangler.NodeMangler.ThreadStaticsIndex(_type) : "_inlinedThreadStaticsIndex");
sb.Append(nameMangler.NodeMangler.ThreadStaticsIndex(_type));
Copy link
Member Author

@VSadov VSadov Aug 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key change. We used to create just one _inlinedThreadStaticsIndex node for the entire storage block in inlined case and were reusing the node for every type involved.
Now we are back to having individual index nodes for all types that have threadstatics. This allows natvis to know the offsets of each type's storage within the block.

@VSadov
Copy link
Member Author

VSadov commented Aug 13, 2023

Another limitation:

  • Sometimes, if the case is simple enough, the visualization does not work.

For example a threadstatic like

class c1
{
    [ThreadStatic]
    public static int tt;
}

Such threadstatic would be completely functional - i.e. c1.tt can be assigned/read/incremented, but cannot be visualized.

  Name Value Type
  ConsoleApp32_c1::__THREADSTATICINDEX.Get() identifier "ConsoleApp32_c1" is undefined  

I think in simple cases we may be dropping/trimming away parts which are important for the visualization.
This can happen regardless of Release/Debug or 8.0/7.0

@@ -94,6 +94,11 @@ protected override void EmitCode(NodeFactory factory, ref ARM64Emitter encoder,
encoder.EmitJNE(factory.HelperEntrypoint(HelperEntrypoint.EnsureClassConstructorRunAndReturnThreadStaticBase));
EmitInlineTLSAccess(factory, ref encoder);
}

// REVIEW: how to keep a node around?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Report it in ComputeNonRelocationBasedDependencies

protected override DependencyList ComputeNonRelocationBasedDependencies(NodeFactory factory)

Copy link
Member Author

@VSadov VSadov Aug 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will look into using that.

The tricky part is to not root threadstatic index for every type that has threadstatics, but only for those that are reachable in the code, even though the index itself is not used.
(technically the index node is “used” - at compile time, and maybe debug time, but the code itself, once emitted, does not need the index)

Copy link
Member Author

@VSadov VSadov Aug 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although, I think, we can assume that for every type with inlined storage the index will be "used", since we build the storage block based on the analysis that we did in the scanner.

Copy link
Member Author

@VSadov VSadov Aug 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just made the node that describes the storage block report as static dependencies the index nodes for the types whose storage is inlined in the block.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@VSadov
Copy link
Member Author

VSadov commented Aug 14, 2023

Thanks!!!

@VSadov VSadov merged commit c5aba1b into dotnet:main Aug 14, 2023
96 of 108 checks passed
@VSadov VSadov deleted the natvis branch August 14, 2023 23:49
@ghost ghost locked as resolved and limited conversation to collaborators Sep 14, 2023
@kunalspathak
Copy link
Member

@VSadov - what changes are needed once we have #89472?

@VSadov
Copy link
Member Author

VSadov commented Jan 11, 2024

I do not think any changes are necessary. The shape of TLS is not changing, so this should keep working. (I will confirm, just in case)

@kunalspathak
Copy link
Member

Here is what I see:

image

@VSadov
Copy link
Member Author

VSadov commented Jan 11, 2024

Yes, that looks functional. The natvis script digs into the TLS structure on the runtime side and knows about possible shapes, so the visualization should work regardless of how TLS is accessed from the code.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thread statics are no longer visible in debugger
3 participants