Fix ModuleLoadUnloadTraceData.ModuleID cast to be unchecked since its really a ulong member. #4354
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The ModuleUnload events ModuleID is typed as a uint64 in the EventPipe manifest and emitted as a uint64 in the event payload. However, the parsing logic in ModuleLoadUnload event in trace event https://github.com/microsoft/perfview/blob/a6c87911fe1aef8f59c9ce54aa4e16a1be6db91e/src/TraceEvent/Parsers/ClrEtwAll.cs.base#L4963 handles it as a long. Android devices running arm64 can use pointer tagging meaning that high bits can be set in 64-bit addresses and since module id is a memory address, it will be returned as a negative long from MofuleLoadUnload event and since all diagnostic tooling is build with overflow checking on by default, casting it to a ulong will trigger and overflow exception when negative. Fix is to do an unchecked cast in this case since the value should be threated as a ulong in first place.
jonathanpeppers
approved these changes
Oct 23, 2023
There was a problem hiding this comment.
This seems to work locally:
D:\src\dotnet\diagnostics\src\Tools\dotnet-gcdump [lateralusX/fix-moduleid-cast]> dotnet run -c Release -- collect -p 6432
Writing gcdump to 'D:\src\dotnet\diagnostics\src\Tools\dotnet-gcdump\20231023_105821_6432.gcdump'...
Finished writing 492293 bytes.
tommcdon
approved these changes
Oct 23, 2023
|
Is this ready to be merged? |
hoyosjs
approved these changes
Oct 23, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The ModuleLoadUnload events ModuleID is typed as a uint64 in the EventPipe manifest and emitted as a uint64 in the event payload. However, the parsing logic in ModuleLoadUnloadTraceDataevent in trace event:
https://github.com/microsoft/perfview/blob/a6c87911fe1aef8f59c9ce54aa4e16a1be6db91e/src/TraceEvent/Parsers/ClrEtwAll.cs.base#L4963
handles it as a long. Android devices running arm64 can use pointer tagging meaning that high bits can be set in 64-bit addresses and since module id is a memory address, it will be returned as a negative long from ModuleLoadUnload event and since all diagnostic tooling is build with overflow checking enabled by default, casting it to a ulong will trigger and overflow exception when high bit is set.
Fix is to do an unchecked cast in this case since the value should be threated as a ulong in first place.
Fixes #4348.