fix RiscvRegId's from_raw_id for RISCV64

[fzyz999]

Hello. For RISC-V 64, the size of Csr and Gpr is 64bit. However, the RiscvRegId's from_raw_id returns 4 bytes for both RV32 and RV64. Thus I create a new type for RV64 and re-implement from_raw_id. Now, the RegId::from_raw_id() works correctly on RV64 architecture.

I am new Rust. I think my implementation is a little ugly but I have no idea how to implement this elegantly. The RiscvRegId64 and RiscvRegId have same definition. I define this new type only for override from_raw_id. I have tried to define something like RiscvRegId<U>. But the compiler complains that U is an unused parameter.

[daniel5151]

Ah, good catch!

Your implementation is actually quite reasonable for someone who's new to Rust. Admittedly, it is possible to use some Rust type-system shenanigans to get a slightly more elegant solution with <U> working correctly:

/// RISC-V Register identifier.
#[derive(Debug, Clone, Copy)]
#[non_exhaustive]
pub enum RiscvRegId<U> {
    /// General Purpose Register (x0-x31).
    Gpr(u8),
    /// Floating Point Register (f0-f31).
    Fpr(u8),
    /// Program Counter.
    Pc,
    /// Control and Status Register.
    Csr(u16),
    /// Privilege level.
    Priv,

    #[doc(hidden)]
    _Marker(core::marker::PhantomData<U>)
}

// use a macro to keep the two implementations in-sync
macro_rules! impl_riscv_reg_id {
    ($usize:ty) => {
        impl RegId for RiscvRegId<$usize> {
            fn from_raw_id(id: usize) -> Option<(Self, usize)> {
                const USIZE: usize = core::mem::size_of::<$usize>();
                
                let reg_size = match id {
                    0..=31 => (Self::Gpr(id as u8), USIZE),
                    32 => (Self::Pc, USIZE),
                    33..=64 => (Self::Fpr((id - 33) as u8), USIZE),
                    65..=4160 => (Self::Csr((id - 65) as u16), USIZE),
                    4161 => (Self::Priv, 1),
                    _ => return None,
                };
                Some(reg_size)
            }
        }
    };
}

impl_riscv_reg_id!(u32);
impl_riscv_reg_id!(u64);

Essentially, we use a PhantomData variant to squelch the unused parameter warning. To avoid cluttering up the docs with this internal implementation detail, we mark the variant as #[doc(hidden)].

Can you update your PR to use this generic implementation instead? It should compile, though I haven't actually checked.

---

Unfortunately the tricky thing with this PR + bugfix is that it is technically a semver breaking change (you're changing the associated type of the Riscv64 Arch), and as such, it can't be released as part of a non-breaking gdbstub 0.4.x release...

I have an idea for how to avoid this in the future, but for now, here's what we are going to have to do:

1. (you) retarget this PR to the dev/0.5 branch, so that it lands as part of the next breaking gdbstub release
2. (me) pop-open a tracking issue for this bug, which will be closed once the bugfix is released

Unfortunately, I'm a bit busy with work / life at the moment, so I don't have a firm timetable on when gdbstub 0.5.0 will be released. That said, it shouldn't be more than a couple months.

In the meantime, you'll just have to maintain a custom in-tree Arch implementation with this bugfix. Thankfully, due to the way the Arch trait it designed, you do not need to use a fork of gdbstub to define custom architectures - you can just create your own types, implement Arch, Registers, and RegId on them, and it'll Just Work.

[fzyz999]

Thank you for your guidance. I learned a lot from it. I've reimplemented the code as you suggested. Then, I retarget this PR to the dev/0.5 branch(I follow the instructions at Github's official doc).

GitHub suggests that there are some conflicting files. Do I need to do something? This is my first time retargeting a PR. I'm unfamiliar with it.

[daniel5151]

Glad I could help!
As for retargeting the PR... turns out this might be a bit harder than I anticipated 😅

You've currently got your changes on a fork of the master branch, but instead you're going to want to migrate your commits over to a fork of the dev/0.5 branch (to avoid pulling in unrelated commits from the master branch that haven't made it into dev/0.5 yet). Using git cherry-pick is probably the easiest way.

The next part is a bit sketchy, and I'm not sure if it'll work...

Basically, you'll want to force-push your local dev/0.5 branch to your remote master branch (which this PR is currently tracked off-of). Ideally, you'd have made this PR off a branch (not just master) so that you wouldn't have to trash your remotes, but it's fine, if you ever want to do another PR, just re-clone the repo lol.

Untested, but something like this should work:

# in the master branch
git checkout dev/0.5
git cherry-pick 12bd701
git cherry-pick 1c7797e
git push
# it should give you a command with `--set-upstream`. swap out `dev/0.5` with `master`
<run that command to push the changes>

Let me know if that works! I'll be out today, but I can help more tomorrow :)

[fzyz999]

Thanks for your help. The detailed explanation you gave was very helpful. I think that works! I executed the following instructions.

git checkout dev/0.5
git cherry-pick 12bd701
# after fix the conflict files
git commit -a
git push origin dev/0.5:master -f

Then, Github said This branch has no conflicts with the base branch. I think everything is all right now.

[daniel5151]

Fantastic work! Thanks again for the contribution 😄

---

As I mentioned before, while I can't commit to a solid release date for gdbstub 0.5, in the meantime, you can continue using the library without requiring a fork, simply my maintaining an in-tree definition of Riscv64 + RiscvRegId.

Also, if you get the chance, do consider providing any feedback you might have about gdbstub by leaving a comment on #31! It's often quite difficult to gauge the efficacy of a library when you're developing it, so and/all feedback is much appreciated :)