-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add basic natvis visualizations for some VM types #52853
Conversation
Tagging subscribers to this area: @dotnet/runtime-infrastructure Issue DetailsAdd some natvis visualizations for SString and Contributes to #52772
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for doing this.
@@ -465,6 +456,10 @@ function(add_jit jitName) | |||
${JIT_ARCH_LINK_LIBRARIES} | |||
) | |||
|
|||
if (CLR_CMAKE_HOST_WIN32) | |||
link_natvis_sources_for_target(${jitName} PRIVATE clrjit.natvis) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the purpose of PRIVATE
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PRIVATE
makes sure to keep the natvis linked only to the jit dlls and not to dlls that link to the jit dlls in the CMake build. See here in the CMake docs: https://cmake.org/cmake/help/latest/command/target_link_libraries.html#id3
<Type Name="HolderBase<*>"> | ||
<DisplayString>{*m_value}</DisplayString> | ||
<Expand> | ||
<ExpandedItem>m_value</ExpandedItem> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need ExpandedItem
for this? May be a screenshot will help. You can also consider adding StringView
for better copy paste experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since HolderBase
doesn't usually hold a string, I think StringView
would be the wrong element to add here. I think DisplayString
handles that case. I'll change it to m_value
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok..And then what is the purpose of ExpandedItem
? Isn't DisplayString
sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExpandedItem
basically removes a level of indirection in the expanded tree when looking in the locals/auto/watch view, so when the holder object is expanded, we get directly to the held value.
Hello @jkoritzinsky! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Wasm failures are unrelated. |
Add some natvis visualizations for SString and
BaseHolder<T>
to get the natvis files started and the infrastructure hooked up so others can add more visualizations for CoreCLR types if desired.Contributes to #52772