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 missing arch-specific RegId implementations #29

Open
6 of 7 tasks
daniel5151 opened this issue Sep 17, 2020 · 4 comments
Open
6 of 7 tasks

Implement missing arch-specific RegId implementations #29

daniel5151 opened this issue Sep 17, 2020 · 4 comments
Labels
API-ergonomics Nothing's "broken", but the API could be improved gdbstub_arch Related to code in `gdbstub_arch` good first issue Good for newcomers help wanted Extra attention is needed

Comments

@daniel5151
Copy link
Owner

daniel5151 commented Sep 17, 2020

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:

  • Armv4t
  • Mips / Mips64
  • Msp430
  • PowerPcAltivec32
  • Riscv32 / Riscv64
  • x86 (i386)
  • x86_64

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.

@daniel5151 daniel5151 added help wanted Extra attention is needed good first issue Good for newcomers API-ergonomics Nothing's "broken", but the API could be improved labels Sep 17, 2020
daniel5151 added a commit that referenced this issue Oct 7, 2020
Issue #29 has been updated to reflect this new approach.
keiichiw added a commit to keiichiw/gdbstub that referenced this issue Oct 26, 2020
This is one of action items in issue daniel5151#29.
keiichiw added a commit to keiichiw/gdbstub that referenced this issue Oct 26, 2020
This is one of action items in issue daniel5151#29.
keiichiw added a commit to keiichiw/gdbstub that referenced this issue Oct 28, 2020
This is one of action items in issue daniel5151#29.
keiichiw added a commit to keiichiw/gdbstub that referenced this issue Oct 29, 2020
This is one of action items in issue daniel5151#29.
daniel5151 pushed a commit that referenced this issue Oct 29, 2020
* implement `RegId` for x86

This is one of action items in issue #29.

* implement `RegId` for x86_64
daniel5151 added a commit that referenced this issue Jan 9, 2021
If #29 is ever resolved, a similar method could be added to `RegId`,
enabling more efficient `pc` queries. Note that the less efficient
approach would still need to be supported, as the ability to query
individual registers is not a required feature implemented by all
targets.
daniel5151 added a commit that referenced this issue Feb 13, 2021
If #29 is ever resolved, a similar method could be added to `RegId`,
enabling more efficient `pc` queries. Note that the less efficient
approach would still need to be supported, as the ability to query
individual registers is not a required feature implemented by all
targets.
@DrChat
Copy link
Contributor

DrChat commented May 27, 2021

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
Copy link
Owner Author

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
Copy link
Contributor

DrChat commented May 27, 2021

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
Copy link
Owner Author

Ah, well that's great then!

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

@daniel5151 daniel5151 added the gdbstub_arch Related to code in `gdbstub_arch` label Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API-ergonomics Nothing's "broken", but the API could be improved gdbstub_arch Related to code in `gdbstub_arch` good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants