search

daniel5151 / gdbstub

An ergonomic, featureful, and easy-to-integrate implementation of the GDB Remote Serial Protocol in Rust (with no-compromises #![no_std] support)
Other
291 stars 45 forks source link

Implement missing arch-specific `RegId` implementations #29

Open daniel5151 opened 4 years ago

daniel5151 commented 4 years ago

Overview

22 added support for register-level read/writes, and introduced a new RegId associated type to the existing Registers trait. This associated type is used to translate raw GDB register ids (i.e: a arch-dependent usize) into a structured human-readable enum identifying the register.

e.g:

/// 32-bit ARM core register identifier.
#[derive(Debug, Clone, Copy)]
pub enum ArmCoreRegId {
    /// General purpose registers (R0-R12)
    Gpr(u8),
    /// Stack Pointer (R13)
    Sp,
    /// Link Register (R14)
    Lr,
    /// Program Counter (R15)
    Pc,
    /// Floating point registers (F0-F7)
    Fpr(u8),
    /// Floating point status
    Fps,
    /// Current Program Status Register (cpsr)
    Cpsr,
}

impl RegId for ArmCoreRegId {
    fn from_raw_id(id: usize) -> Option<(Self, usize)> {
        match id {
            0..=12 => Some((Self::Gpr(id as u8), 4)),
            13 => Some((Self::Sp, 4)),
            14 => Some((Self::Lr, 4)),
            15 => Some((Self::Pc, 4)),
            16..=23 => Some((Self::Fpr(id as u8), 4)),
            25 => Some((Self::Cpsr, 4)),
            _ => None,
        }
    }
}

Unfortunately, this API was only added after several contributors had already upstreamed their Arch implementations. As a result, there are several arch implementations which are missing proper RegId enums.

As a stop-gap measure, affected Arch implementations have been modified to accept a RegIdImpl type parameter, which requires users to manually specify a RegId implementation. If none is available, users can also specify RegId = (), which uses a stubbed implementation that always returns None.

e.g:

pub enum PowerPcAltivec32<RegIdImpl: RegId> {
    #[doc(hidden)]
    _Marker(core::marker::PhantomData<RegIdImpl>),
}

impl<RegIdImpl: RegId> Arch for PowerPcAltivec32<RegIdImpl> {
    type Usize = u32;
    type Registers = reg::PowerPcCommonRegs;
    type RegId = RegIdImpl;
    // ...
}

Action Items

At the time of writing, the following Arch implementations are still missing proper RegId implementations:

Whenever a RegId enum is upstreamed, the associated Arch's RegIdImpl parameter will be defaulted to the newly added enum. This will simplify the API without requiring an explicit breaking API change. Once all RegIdImpl have a default implementation, only a single breaking API change will be required to remove RegIdImpl entirely.

Please only contribute RegId implementations that have been reasonably tested in your own project!

As with all architecture-specific bits of functionality in gdbstub, it's up to the contributor to test whether or not the feature works as expected - all I can do is review the core for style and efficiency.

This issue is not a blocker, and I do not mind having Arch implementations with a RegIdImpl parameter lingering in the codebase.

DrChat commented 3 years ago

Here is the relevant target description file from the GDB sources for PPC32: https://github.com/bminor/binutils-gdb/blob/master/gdb/features/rs6000/powerpc-32.c

daniel5151 commented 3 years ago

I would also leave the following links as well:

Just as a reminder though, this issue is not a bug or a blocker - it is simply tracking something that should get resolved eventually. Moreover, implementing a new RegId isn't just adding a new impl RegId enum to the codebase -- you'd also have to put in some legwork to actually test the implementation as part of a larger project.

@jamcleod, you wouldn't happen to have some spare time on your hands to circle back and upstream a RegId implementation for PowerPcAltivec32? I know it's been a while, but I thought I'd ask since you were the one who originally upstreamed the rest of the PowerPC code.

If not, all good! I totally don't mind leaving this issue open until someone happens to use gdbstub with a Power PC target, and is willing to upstream a tested RegId implementation :)

DrChat commented 3 years ago

Appreciate the other references :)

I have a PPC target, but I'll have to figure out the dynamic dispatch first before I can test a RegId implementation for it.

daniel5151 commented 3 years ago

Ah, well that's great then!

In that case, I'm looking forwards to seeing a PR 😄