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

implement RegId for x86/x86_64 #34

Merged
merged 2 commits into from
Oct 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/arch/x86/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ pub mod reg;
/// Check out the [module level docs](../index.html#whats-with-regidimpl) for
/// more info about the `RegIdImpl` type parameter.
#[allow(non_camel_case_types)]
pub enum X86_64_SSE<RegIdImpl: RegId> {
pub enum X86_64_SSE<RegIdImpl: RegId = reg::id::X86_64CoreRegId> {
#[doc(hidden)]
_Marker(core::marker::PhantomData<RegIdImpl>),
}
Expand All @@ -32,7 +32,7 @@ impl<RegIdImpl: RegId> Arch for X86_64_SSE<RegIdImpl> {
/// Check out the [module level docs](../index.html#whats-with-regidimpl) for
/// more info about the `RegIdImpl` type parameter.
#[allow(non_camel_case_types)]
pub enum X86_SSE<RegIdImpl: RegId> {
pub enum X86_SSE<RegIdImpl: RegId = reg::id::X86CoreRegId> {
#[doc(hidden)]
_Marker(core::marker::PhantomData<RegIdImpl>),
}
Expand Down
201 changes: 197 additions & 4 deletions src/arch/x86/reg/id.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,198 @@
// TODO: Add proper `RegId` implementation. See [issue #29](https://github.com/daniel5151/gdbstub/issues/29)
// pub enum X86RegId {}
use crate::arch::RegId;

// TODO: Add proper `RegId` implementation. See [issue #29](https://github.com/daniel5151/gdbstub/issues/29)
// pub enum X86_64RegId {}
/// FPU register identifier.
#[derive(Debug, Clone, Copy)]
pub enum X87FpuInternalRegId {
/// Floating-point control register
Fctrl,
/// Floating-point status register
Fstat,
/// Tag word
Ftag,
/// FPU instruction pointer segment
Fiseg,
/// FPU intstruction pointer offset
Fioff,
/// FPU operand segment
Foseg,
/// FPU operand offset
Fooff,
/// Floating-point opcode
Fop,
}

impl X87FpuInternalRegId {
fn from_u8(val: u8) -> Option<Self> {
use self::X87FpuInternalRegId::*;

let r = match val {
0 => Fctrl,
1 => Fstat,
2 => Ftag,
3 => Fiseg,
4 => Fioff,
5 => Foseg,
6 => Fooff,
7 => Fop,
_ => return None,
};
Some(r)
}
}

/// 32-bit x86 core + SSE register identifier.
///
/// Source: https://github.com/bminor/binutils-gdb/blob/master/gdb/features/i386/32bit-core.xml
/// Additionally: https://github.com/bminor/binutils-gdb/blob/master/gdb/features/i386/32bit-sse.xml
#[derive(Debug, Clone, Copy)]
#[non_exhaustive]
pub enum X86CoreRegId {
/// Accumulator
Eax,
/// Count register
Ecx,
/// Data register
Edx,
/// Base register
Ebx,
/// Stack pointer
Esp,
/// Base pointer
Ebp,
/// Source index
Esi,
/// Destination index
Edi,
/// Instruction pointer
Eip,
/// Status register
Eflags,
/// Segment registers: CS, SS, DS, ES, FS, GS
Segment(u8),
/// FPU registers: ST0 through ST7
St(u8),
/// FPU internal registers
Fpu(X87FpuInternalRegId),
/// SIMD Registers: XMM0 through XMM7
Xmm(u8),
/// SSE Status/Control Register
Mxcsr,
}

impl RegId for X86CoreRegId {
fn from_raw_id(id: usize) -> Option<(Self, usize)> {
use self::X86CoreRegId::*;

let r = match id {
0 => (Eax, 4),
1 => (Ecx, 4),
2 => (Edx, 4),
3 => (Ebx, 4),
4 => (Esp, 4),
5 => (Ebp, 4),
6 => (Esi, 4),
7 => (Edi, 4),
8 => (Eip, 4),
9 => (Eflags, 4),
10..=15 => (Segment(id as u8 - 10), 4),
16..=23 => (St(id as u8 - 16), 10),
24..=31 => match X87FpuInternalRegId::from_u8(id as u8 - 24) {
Some(r) => (Fpu(r), 4),
None => unreachable!(),
},
32..=39 => (Xmm(id as u8 - 32), 16),
40 => (Mxcsr, 4),
_ => return None,
};
Some(r)
}
}

/// 64-bit x86 core + SSE register identifier.
///
/// Source: https://github.com/bminor/binutils-gdb/blob/master/gdb/features/i386/64bit-core.xml
/// Additionally: https://github.com/bminor/binutils-gdb/blob/master/gdb/features/i386/64bit-sse.xml
#[derive(Debug, Clone, Copy)]
#[non_exhaustive]
pub enum X86_64CoreRegId {
/// General purpose registers:
/// RAX, RBX, RCX, RDX, RSI, RDI, RBP, RSP, r8-r15
Gpr(u8),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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 😄

/// Instruction pointer
Rip,
/// Status register
Eflags,
/// Segment registers: CS, SS, DS, ES, FS, GS
Segment(u8),
/// FPU registers: ST0 through ST7
St(u8),
/// FPU internal registers
Fpu(X87FpuInternalRegId),
/// SIMD Registers: XMM0 through XMM15
Xmm(u8),
/// SSE Status/Control Register
Mxcsr,
}

impl RegId for X86_64CoreRegId {
fn from_raw_id(id: usize) -> Option<(Self, usize)> {
use self::X86_64CoreRegId::*;

let r = match id {
0..=15 => (Gpr(id as u8), 8),
16 => (Rip, 4),
17 => (Eflags, 8),
18..=23 => (Segment(id as u8 - 18), 4),
24..=31 => (St(id as u8 - 24), 10),
32..=39 => match X87FpuInternalRegId::from_u8(id as u8 - 32) {
Some(r) => (Fpu(r), 4),
None => unreachable!(),
},
40..=55 => (Xmm(id as u8 - 40), 16),
56 => (Mxcsr, 4),
_ => return None,
};
Some(r)
}
}

#[cfg(test)]
mod tests {
daniel5151 marked this conversation as resolved.
Show resolved Hide resolved
use crate::arch::traits::RegId;
use crate::arch::traits::Registers;

/// Compare the following two values which are expected to be the same:
/// * length of data written by `Registers::gdb_serialize()` in byte
/// * sum of sizes of all registers obtained by `RegId::from_raw_id()`
fn test<Rs: Registers, RId: RegId>() {
// Obtain the data length written by `gdb_serialize` by passing a custom
// closure.
let mut serialized_data_len = 0;
let counter = |b: Option<u8>| {
if b.is_some() {
serialized_data_len += 1;
}
};
Rs::default().gdb_serialize(counter);

// Accumulate register sizes returned by `from_raw_id`.
let mut i = 0;
let mut sum_reg_sizes = 0;
while let Some((_, size)) = RId::from_raw_id(i) {
sum_reg_sizes += size;
i += 1;
}

assert_eq!(serialized_data_len, sum_reg_sizes);
}

#[test]
fn test_x86() {
test::<crate::arch::x86::reg::X86CoreRegs, crate::arch::x86::reg::id::X86CoreRegId>()
}

#[test]
fn test_x86_64() {
test::<crate::arch::x86::reg::X86_64CoreRegs, crate::arch::x86::reg::id::X86_64CoreRegId>()
}
}