[binutils-gdb] gdb/riscv: improve a comment about fcsr, fflags, and frm registers

[Andrew Burgess <aburgess@sourceware.org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=0e6a6e40bb86a348468dd5ac9169df94c21da2f4

commit 0e6a6e40bb86a348468dd5ac9169df94c21da2f4
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Thu Aug 11 15:23:24 2022 +0100

    gdb/riscv: improve a comment about fcsr, fflags, and frm registers
    
    There's a comment in riscv-tdep.c that explains some of the background
    about how we check for the fcsr, fflags, and frm registers within a
    riscv target description.
    
    This comment (and the functionality it describes) relates to how QEMU
    advertises these registers within its target description.
    
    Unfortunately, QEMU includes these three registers in both the fpu and
    crs target description features.  To work around this GDB uses one of
    the register declarations, and ignores the other, this means the GDB
    user sees a single copy of each register, and things just work.
    
    When I originally wrote the comment I thought it didn't matter which
    copy of the register GDB selected, the fpu copy or the csr copy, so
    long as we just used one of them.  The comment reflected this belief.
    
    Upon further investigation, it turns out I was wrong.  GDB has to use
    the csr copy of the register.  If GDB tries to use the register from
    the fpu feature then QEMU will return an error when GDB tries to read
    or write the register.
    
    Luckily, the code within GDB (currently) will always select the csr
    copy of the register, so nothing is broken, but the comment is wrong.
    This commit updates the comment to better describe what is actually
    going on.
    
    Of course, I should probably also send a patch to QEMU to fix up the
    target description that is sent to GDB.

Diff:
---
 gdb/riscv-tdep.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index b9a51f7ae6a..9ec430d8a10 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -3591,22 +3591,25 @@ riscv_tdesc_unknown_reg (struct gdbarch *gdbarch, tdesc_feature *feature,
      and CSR register sets.
 
      Some targets (QEMU) copied these target descriptions into their source
-     tree, and so we're currently stuck working with some targets that
+     tree, and so we're now stuck working with some versions of QEMU that
      declare the same registers twice.
 
-     There's not much we can do about this any more.  Assuming the target
-     will direct a request for either register number to the correct
-     underlying hardware register then it doesn't matter which one GDB
-     uses, so long as we (GDB) are consistent (so that we don't end up with
-     invalid cache misses).
-
-     As we always scan the FPU registers first, then the CSRs, if the
-     target has included the offending registers in both sets then we will
-     always see the FPU copies here, as the CSR versions will replace them
-     in the register list.
-
-     To prevent these duplicates showing up in any of the register list,
-     record their register numbers here.  */
+     To make matters worse, if GDB tries to read or write to these
+     registers using the register number assigned in the FPU feature set,
+     then QEMU will fail to read the register, so we must use the register
+     number declared in the CSR feature set.
+
+     Luckily, GDB scans the FPU feature first, and then the CSR feature,
+     which means that the CSR feature will be the one we end up using, the
+     versions of these registers in the FPU feature will appear as unknown
+     registers and will be passed through to this code.
+
+     To prevent these duplicate registers showing up in any of the register
+     lists, and to prevent GDB every trying to access the FPU feature copies,
+     we spot the three problematic registers here, and record the register
+     number that GDB has assigned them.  Then in riscv_register_name we will
+     return no name for the three duplicates, this hides the duplicates from
+     the user.  */
   if (strcmp (tdesc_feature_name (feature), riscv_freg_feature.name ()) == 0)
     {
       riscv_gdbarch_tdep *tdep = gdbarch_tdep<riscv_gdbarch_tdep> (gdbarch);