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

Update clrmd to 3.1 #2488

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update clrmd to 3.1 #2488

wants to merge 1 commit into from

Conversation

timcassell
Copy link
Collaborator

@timcassell timcassell commented Dec 28, 2023

Fixes #2383

I left the x86/x64 disassemblers on v1, those can be updated separately.

Also, as discussed in #2383, this breaks netcoreapp3.0 and older, so we should probably release this in 0.14.0.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

I am supportive of this change, but we need to perform a LOT of testing.

Thank you for your hard work on this @timcassell !

return new[] { new ILToNativeMap() { StartAddress = startAddress, EndAddress = endAddress } };
}

return sortedMaps;
Copy link
Member

Choose a reason for hiding this comment

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

It took me a lot of time to workaround all ClrMd bugs. If we really want to remove these workarounds we need to do a very extensive testing:

  1. Run a simple benchmark and ask BDN to disassemble ALL methods for main. It should produce a markdown file with all CLR methods disassembled.
  2. Same as above, but for this PR.
  3. Diff two produced .md files and ensure that all instructions are correctly resolved and there are no missing things.

@timcassell
Copy link
Collaborator Author

@adamsitnik I ran dotnet run -c Release -f net8.0 --filter BenchmarkDotNet.Samples.IntroDisassembly.SumLocal --disasmFilter '*'
And it looks like we get a lot more information in 3.1 (as expected since clrmd was specifically updated to account for changes in the runtime).

image

I then ran the same benchmark with net6.0 to compare the older runtime results. The output was in a different order, but after comparing individual functions, they look identical.

image

Copy link
Member

@AndreyAkinshin AndreyAkinshin left a comment

Choose a reason for hiding this comment

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

I have tested this branch with some benchmarks, and it works like a charm! No concerns from my side: I haven't found any issues. However, I'm not aware of all the corner cases and I don't know how to perform proper extensive testing. I approve the PR and I don't mind including it in v0.14.0.

@adamsitnik what do you think? If you feel confident, we can merge it. If you have any questions, please provide a set of scenarios that should be tested.

P.S. I feel like it's just impossible to verify such changes properly. There is always a chance of getting a corner case with a regression. We can always just give it a try: release the new version, collect feedback, and fix any further issues once they are discovered.

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

Successfully merging this pull request may close these issues.

DisassemblyDiagnoser does not work in InProcessEmitToolchain
3 participants