Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Modify amd64walker to use table based decode #25958

Merged
merged 6 commits into from
Sep 3, 2019

Conversation

sdmaclea
Copy link

@sdmaclea sdmaclea commented Aug 1, 2019

Use tables for NativeWalker::DecodeInstructionForPatchSkip()
Also files used to generate the decode tables in doc folder.

Fixes #25132

@sdmaclea sdmaclea added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) arch-x64 area-Diagnostics labels Aug 1, 2019
@sdmaclea sdmaclea self-assigned this Aug 1, 2019
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

The odds of me finding a bug here are virtually zero given my very limited knowledge of AMD64 assembly. I think we should find a reviewer with some AMD64 assembly expertise (@CarolEidt @briansull @BruceForstall ?)

I also think it would be handy to look at what you've already done to test this and if there is anything else we should be doing. This feels like the most sizable code change we've taken to a pre-existing debugger feature in a long time and its not clear to me how likely we'd be to spot regressions with review alone.

src/debug/ee/amd64/amd64InstrDecode.h Outdated Show resolved Hide resolved
src/debug/ee/amd64/amd64InstrDecode.h Outdated Show resolved Hide resolved
src/debug/ee/amd64/doc/createOpcodes.cpp Outdated Show resolved Hide resolved
src/debug/ee/amd64/doc/README.md Outdated Show resolved Hide resolved
@sdmaclea
Copy link
Author

sdmaclea commented Aug 1, 2019

So far this has fairly simplistic testing.

I tested this fixed #25132. I also tested I could set a breakpoint on any line in the file without changing results.

I will test this against our internal tests when I get instructions from @hoyosjs

@sdmaclea sdmaclea removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 15, 2019
@sdmaclea sdmaclea merged commit f82281f into dotnet:master Sep 3, 2019
@sdmaclea sdmaclea deleted the amd64InstrDecode branch September 3, 2019 16:32
@tannergooding
Copy link
Member

@sdmaclea, is this going to be backported to 3.0 considered it results in corrupted data when debugging?

@sdmaclea
Copy link
Author

sdmaclea commented Sep 3, 2019

We thought this was too big a change to meet the 3.0 bar.

@tannergooding
Copy link
Member

What about 3.1 for the LTS?

sdmaclea added a commit to sdmaclea/coreclr that referenced this pull request Mar 26, 2020
* Modify amd64walker to use table based decode

Use tables for NativeWalker::DecodeInstructionForPatchSkip()
Also files used to generate the decode tables in doc folder.
sdmaclea added a commit to sdmaclea/coreclr that referenced this pull request May 1, 2020
* Modify amd64walker to use table based decode

Use tables for NativeWalker::DecodeInstructionForPatchSkip()
Also files used to generate the decode tables in doc folder.
Anipik pushed a commit that referenced this pull request Jun 9, 2020
…28033)

* Whitespace (#25957)

* Modify amd64walker to use table based decode (#25958)

* Modify amd64walker to use table based decode

Use tables for NativeWalker::DecodeInstructionForPatchSkip()
Also files used to generate the decode tables in doc folder.

* Review feedback and typos

+ Typos (#26968)
+ Feedback from 3.1 amd64InstrDecode review (dotnet/runtime#35670)
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Modify amd64walker to use table based decode

Use tables for NativeWalker::DecodeInstructionForPatchSkip()
Also files used to generate the decode tables in doc folder.


Commit migrated from dotnet/coreclr@f82281f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breakpoint changes the output of the program on x64
3 participants