Conversation
There was a problem hiding this comment.
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.
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
|
AndreyAkinshin
left a comment
AndreyAkinshin
left a comment
There was a problem hiding this comment.
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. |
3 participants
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.