implement RegId for x86/x86_64
#34
Conversation
| #[non_exhaustive] | ||
| pub enum X86_64CoreRegId { | ||
| /// General purpose registers: RAX, RBX, RCX, RDX, RSI, RDI, RBP, RSP, r8-r15 | ||
| Gpr(u8), |
There was a problem hiding this comment.
I also considered to have Rax, Rbx, etc as different enum variants but chose this design to align with the design of X86_64CoreRegs for now.
Which design do you prefer?
There was a problem hiding this comment.
The order of general purpose registers seems not to be well defined.
It looks inconsistent even in the Intel SDM. For example, RSI comes before RDI in some paragraphs, but it sometimes comes after RDI in other parts:(
That's why I had considered to have different variants for these registers. Actually, X86CoreRegs does so. (crosvm also does so.)
But, it's probably a different topic. The current design is also reasonable, as it simply follows the order defined in the GDB's XML file and the order is documented well. So, let me leave this design as is.
There was a problem hiding this comment.
Ahh, classic x86...
Personally, I'm now leaning towards breaking out RAX, RBX, ..., RSP into their own variants (and now that I think about it, the Segment registers as well). That said, I think it's fine if we leave the design as-is, as it matches the existing x86[_64]CoreRegs structure.
I'll probably open a tracking issue to break-up the GPRs + SRs into their own struct fields / variants, though that should be something I can take care of myself at some point closer to the release of 0.5
There was a problem hiding this comment.
Sounds good!
The register names are really confusing. Actually, when I was implementing the GDB feature for crosvm, I mistakenly swapped the order of RBP and RSP, and wasted a whole day to debug it😓
So, I'm really supportive of breaking GPRs + SRs into their own variants.
There was a problem hiding this comment.
I've gone ahead and opened the tracking issue at #35
If you're bored, feel free to open a PR for it yourself, otherwise, I'll try to get around to it at some arbitrary point in the future 😄
keiichiw
force-pushed
the
x86-64-RegId
branch
from
07856eb to
741367f
Compare
|
@daniel5151 Please take a look. |
There was a problem hiding this comment.
Thank you for the PR!
I have a couple of comments, but all in all, this LGTM 👍
I won't lie, I'm pretty excited to see that my library might be integrated into crosvm!
I had a blast working on crosvm as an intern, and I'm glad to be able to contribute back to it again (albeit indirectly 😉)
keiichiw
force-pushed
the
x86-64-RegId
branch
from
741367f to
4970659
Compare
|
Please take a look again. I addressed your comments. Also, I added tests. |
| #[non_exhaustive] | ||
| pub enum X86_64CoreRegId { | ||
| /// General purpose registers: RAX, RBX, RCX, RDX, RSI, RDI, RBP, RSP, r8-r15 | ||
| Gpr(u8), |
There was a problem hiding this comment.
Ahh, classic x86...
Personally, I'm now leaning towards breaking out RAX, RBX, ..., RSP into their own variants (and now that I think about it, the Segment registers as well). That said, I think it's fine if we leave the design as-is, as it matches the existing x86[_64]CoreRegs structure.
I'll probably open a tracking issue to break-up the GPRs + SRs into their own struct fields / variants, though that should be something I can take care of myself at some point closer to the release of 0.5
This is one of action items in issue daniel5151#29.
keiichiw
force-pushed
the
x86-64-RegId
branch
from
4970659 to
abaf039
Compare
|
Thanks for the comments and suggestions! I addressed them 😄 |
This is one of action items in issue #29.