-
-
Notifications
You must be signed in to change notification settings - Fork 964
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
Update clrmd to 3.1 #2488
Conversation
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.
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; |
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.
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:
- Run a simple benchmark and ask BDN to disassemble ALL methods for main. It should produce a markdown file with all CLR methods disassembled.
- Same as above, but for this PR.
- Diff two produced .md files and ensure that all instructions are correctly resolved and there are no missing things.
@adamsitnik I ran I then ran the same benchmark with |
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.
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.
I won't have the time to test all possible scenarios in the near future. Since it fixes #2638 (comment) lets merge it now and wait for new potential bug reports. |
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.