Patchwork [v2,1/4] Add helper functions to trad_frame to support register cache maps.

login
register
mail settings
Submitter John Baldwin
Date Sept. 28, 2018, 9:44 p.m.
Message ID <93121ec2-866f-2d6f-4682-336ed043b4d0@FreeBSD.org>
Download mbox | patch
Permalink /patch/29578/
State New
Headers show

Comments

John Baldwin - Sept. 28, 2018, 9:44 p.m.
On 9/27/18 12:31 PM, Simon Marchi wrote:
> On 2018-09-24 04:51 PM, John Baldwin wrote:
>> Currently, signal frame handlers require explicitly coded calls to
>> trad_frame_set_reg_addr() to describe the location of saved registers
>> within a signal frame.  This change permits the regcache_map_entry
>> arrays used with regcache::supply_regset and regcache::collect_regset
>> to be used to describe a block of saved registers given an initial
>> address for the register block.
>>
>> Some systems use the same layout for registers in core dump notes,
>> native register sets with ptrace(), and the register contexts saved in
>> signal frames.  On these systems, a single register map can now be
>> used to describe the layout of registers in all three places.
>>
>> If a register map entry's size does not match the native size of a
>> register, try to match the semantics used by
>> regcache::transfer_regset.  If a register slot is too large, assume
>> that the register's value is stored in the first N bytes and ignore
>> the remaning bytes.  If the register slot is smaller than the
>> register, assume the slot holds the low N bytes of the register's
>> value.  Read these low N bytes from the target and zero-extend them to
>> generate a register value.
> 
> LGTM.  It sounds very good to de-duplicate the knowledge of the register
> layout in those structures.

After some discussions on IRC, I've modified this patch a bit.  If we want
to rename 'regcache_map_entry' to be less regcache-specific then I think
that might be something to do as a followup change.  The main changes
relative to the v2 patch are only providing a single new function (the
one taking a frame_cache) until a caller shows up needing to use the other
function (taking the saved register array), using a typed pointer for the
regache_map_entry argument to the new function, and adding some comments to
regcache.h to note that this structure is now also used by trad_frame as
well as document the semantics when a register's "slot" in the map doesn't
match the size of a register.  I've pasted it below rather than rerolling
the whole series:

Author: John Baldwin <jhb@FreeBSD.org>
Date:   Fri Sep 28 14:37:22 2018 -0700

    Add a helper function to trad_frame to support register cache maps.
    
    Currently, signal frame handlers require explicitly coded calls to
    trad_frame_set_reg_addr() to describe the location of saved registers
    within a signal frame.  This change permits the regcache_map_entry
    arrays used with regcache::supply_regset and regcache::collect_regset
    to be used to describe a block of saved registers given an initial
    address for the register block.
    
    Some systems use the same layout for registers in core dump notes,
    native register sets with ptrace(), and the register contexts saved in
    signal frames.  On these systems, a single register map can now be
    used to describe the layout of registers in all three places.
    
    If a register map entry's size does not match the native size of a
    register, try to match the semantics used by
    regcache::transfer_regset.  If a register slot is too large, assume
    that the register's value is stored in the first N bytes and ignore
    the remaning bytes.  If the register slot is smaller than the
    register, assume the slot holds the low N bytes of the register's
    value.  Read these low N bytes from the target and zero-extend them to
    generate a register value.
    
    While here, document the semantics for both regcache::transfer_regset
    and trad_frame with respect to register slot's whose size does not
    match the register's size.
    
    gdb/ChangeLog:
    
            * regcache.h (struct regcache_map_entry): Note that this type can
            be used with traditional frame caches.
            * trad-frame.c (trad_frame_set_reg_regmap): New.
            * trad-frame.h (trad_frame_set_reg_regmap): New.
Simon Marchi - Oct. 5, 2018, 8:18 p.m.
On 2018-09-28 05:44 PM, John Baldwin wrote:
> On 9/27/18 12:31 PM, Simon Marchi wrote:

>> On 2018-09-24 04:51 PM, John Baldwin wrote:

>>> Currently, signal frame handlers require explicitly coded calls to

>>> trad_frame_set_reg_addr() to describe the location of saved registers

>>> within a signal frame.  This change permits the regcache_map_entry

>>> arrays used with regcache::supply_regset and regcache::collect_regset

>>> to be used to describe a block of saved registers given an initial

>>> address for the register block.

>>>

>>> Some systems use the same layout for registers in core dump notes,

>>> native register sets with ptrace(), and the register contexts saved in

>>> signal frames.  On these systems, a single register map can now be

>>> used to describe the layout of registers in all three places.

>>>

>>> If a register map entry's size does not match the native size of a

>>> register, try to match the semantics used by

>>> regcache::transfer_regset.  If a register slot is too large, assume

>>> that the register's value is stored in the first N bytes and ignore

>>> the remaning bytes.  If the register slot is smaller than the

>>> register, assume the slot holds the low N bytes of the register's

>>> value.  Read these low N bytes from the target and zero-extend them to

>>> generate a register value.

>>

>> LGTM.  It sounds very good to de-duplicate the knowledge of the register

>> layout in those structures.

> 

> After some discussions on IRC, I've modified this patch a bit.  If we want

> to rename 'regcache_map_entry' to be less regcache-specific then I think

> that might be something to do as a followup change.  The main changes

> relative to the v2 patch are only providing a single new function (the

> one taking a frame_cache) until a caller shows up needing to use the other

> function (taking the saved register array), using a typed pointer for the

> regache_map_entry argument to the new function, and adding some comments to

> regcache.h to note that this structure is now also used by trad_frame as

> well as document the semantics when a register's "slot" in the map doesn't

> match the size of a register.  I've pasted it below rather than rerolling

> the whole series:

> 

> Author: John Baldwin <jhb@FreeBSD.org>

> Date:   Fri Sep 28 14:37:22 2018 -0700

> 

>     Add a helper function to trad_frame to support register cache maps.

>     

>     Currently, signal frame handlers require explicitly coded calls to

>     trad_frame_set_reg_addr() to describe the location of saved registers

>     within a signal frame.  This change permits the regcache_map_entry

>     arrays used with regcache::supply_regset and regcache::collect_regset

>     to be used to describe a block of saved registers given an initial

>     address for the register block.

>     

>     Some systems use the same layout for registers in core dump notes,

>     native register sets with ptrace(), and the register contexts saved in

>     signal frames.  On these systems, a single register map can now be

>     used to describe the layout of registers in all three places.

>     

>     If a register map entry's size does not match the native size of a

>     register, try to match the semantics used by

>     regcache::transfer_regset.  If a register slot is too large, assume

>     that the register's value is stored in the first N bytes and ignore

>     the remaning bytes.  If the register slot is smaller than the

>     register, assume the slot holds the low N bytes of the register's

>     value.  Read these low N bytes from the target and zero-extend them to

>     generate a register value.

>     

>     While here, document the semantics for both regcache::transfer_regset

>     and trad_frame with respect to register slot's whose size does not

>     match the register's size.

>     

>     gdb/ChangeLog:

>     

>             * regcache.h (struct regcache_map_entry): Note that this type can

>             be used with traditional frame caches.

>             * trad-frame.c (trad_frame_set_reg_regmap): New.

>             * trad-frame.h (trad_frame_set_reg_regmap): New.


Thanks, this LGTM with a nit:

>  /* Mapping between register numbers and offsets in a buffer, for use

> -   in the '*regset' functions below.  In an array of

> -   'regcache_map_entry' each element is interpreted like follows:

> +   in the '*regset' functions below and with traditional frame caches.

> +   In an array of 'regcache_map_entry' each element is interpreted

> +   like follows:

>  

>     - If 'regno' is a register number: Map register 'regno' to the

>       current offset (starting with 0) and increase the current offset

>       by 'size' (or the register's size, if 'size' is zero).  Repeat

>       this with consecutive register numbers up to 'regno+count-1'.

>  

> +     For each described register, if 'size' is larger than the

> +     register's size, the register's value is assumed to be stored in

> +     the first N (where N is the register size) bytes at the current

> +     offset.  The remaining 'size' - N bytes are filled with zeroes by

> +     'regcache_collect_regset' and ignored by other consumers.

> +

> +     If 'size' is smaller than the register's size, only the first

> +     'size' bytes of a register's value are assumed to be stored at

> +     the current offset.  'regcache_collect_regset' copies the first

> +     'size' bytes of a register's value to the output buffer.

> +     'regcache_supply_regset' copies the bytes from the input buffer

> +     into the low 'size' bytes of the register's value leaving the

> +     remaining bytes of the register's value unchanged.  Frame caches

> +     read the 'size' bytes from the stack frame and zero extend them

> +     to generate the register's value.


In that case, regcache_supply_part will set the first 'size' bytes of the
register.  In the case of big-endian, does that mean the high 'size' bytes
will be set?  Perhaps you could say first 'size' bytes instead.

Simon

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 56645e960f..f869713738 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@ 
+2018-09-28  John Baldwin  <jhb@FreeBSD.org>
+
+       * regcache.h (struct regcache_map_entry): Note that this type can
+       be used with traditional frame caches.
+       * trad-frame.c (trad_frame_set_reg_regmap): New.
+       * trad-frame.h (trad_frame_set_reg_regmap): New.
+
 2018-09-28  John Baldwin  <jhb@FreeBSD.org>
 
        * disasm-selftests.c (print_one_insn_test): Add bfd_arch_riscv to
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 4a45f33871..57a4ab3488 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -72,14 +72,31 @@  extern void regcache_cooked_write_unsigned (struct regcache *regcache,
 extern void regcache_write_pc (struct regcache *regcache, CORE_ADDR pc);
 
 /* Mapping between register numbers and offsets in a buffer, for use
-   in the '*regset' functions below.  In an array of
-   'regcache_map_entry' each element is interpreted like follows:
+   in the '*regset' functions below and with traditional frame caches.
+   In an array of 'regcache_map_entry' each element is interpreted
+   like follows:
 
    - If 'regno' is a register number: Map register 'regno' to the
      current offset (starting with 0) and increase the current offset
      by 'size' (or the register's size, if 'size' is zero).  Repeat
      this with consecutive register numbers up to 'regno+count-1'.
 
+     For each described register, if 'size' is larger than the
+     register's size, the register's value is assumed to be stored in
+     the first N (where N is the register size) bytes at the current
+     offset.  The remaining 'size' - N bytes are filled with zeroes by
+     'regcache_collect_regset' and ignored by other consumers.
+
+     If 'size' is smaller than the register's size, only the first
+     'size' bytes of a register's value are assumed to be stored at
+     the current offset.  'regcache_collect_regset' copies the first
+     'size' bytes of a register's value to the output buffer.
+     'regcache_supply_regset' copies the bytes from the input buffer
+     into the low 'size' bytes of the register's value leaving the
+     remaining bytes of the register's value unchanged.  Frame caches
+     read the 'size' bytes from the stack frame and zero extend them
+     to generate the register's value.
+
    - If 'regno' is REGCACHE_MAP_SKIP: Add 'count*size' to the current
      offset.
 
diff --git a/gdb/trad-frame.c b/gdb/trad-frame.c
index fd9f24084d..a3484d2b39 100644
--- a/gdb/trad-frame.c
+++ b/gdb/trad-frame.c
@@ -22,6 +22,7 @@ 
 #include "trad-frame.h"
 #include "regcache.h"
 #include "frame-unwind.h"
+#include "target.h"
 #include "value.h"
 
 struct trad_frame_cache
@@ -148,6 +149,62 @@  trad_frame_set_reg_addr (struct trad_frame_cache *this_trad_cache,
   trad_frame_set_addr (this_trad_cache->prev_regs, regnum, addr);
 }
 
+void
+trad_frame_set_reg_regmap (struct trad_frame_cache *this_trad_cache,
+                          const struct regcache_map_entry *regmap,
+                          CORE_ADDR addr, size_t size)
+{
+  struct gdbarch *gdbarch = get_frame_arch (this_trad_cache->this_frame);
+  int offs = 0, count;
+
+  for (; (count = regmap->count) != 0; regmap++)
+    {
+      int regno = regmap->regno;
+      int slot_size = regmap->size;
+
+      if (slot_size == 0 && regno != REGCACHE_MAP_SKIP)
+       slot_size = register_size (gdbarch, regno);
+
+      if (offs + slot_size > size)
+       break;
+
+      if (regno == REGCACHE_MAP_SKIP)
+       offs += count * slot_size;
+      else
+       for (; count--; regno++, offs += slot_size)
+         {
+           /* Mimic the semantics of regcache::transfer_regset if a
+              register slot's size does not match the size of a
+              register.
+
+              If a register slot is larger than a register, assume
+              the register's value is stored in the first N bytes of
+              the slot and ignore the remaining bytes.
+
+              If the register slot is smaller than the register,
+              assume that the slot contains the low N bytes of the
+              register's value.  Since trad_frame assumes that
+              registers stored by address are sized according to the
+              register, read the low N bytes and zero-extend them to
+              generate a register value.  */
+           if (slot_size >= register_size (gdbarch, regno))
+             trad_frame_set_reg_addr (this_trad_cache, regno, addr + offs);
+           else
+             {
+               enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+               gdb_byte buf[slot_size];
+
+               if (target_read_memory (addr + offs, buf, sizeof buf) == 0)
+                 {
+                   LONGEST val
+                     = extract_unsigned_integer (buf, sizeof buf, byte_order);
+                   trad_frame_set_reg_value (this_trad_cache, regno, val);
+                 }
+             }
+         }
+    }
+}
+
 void
 trad_frame_set_unknown (struct trad_frame_saved_reg this_saved_regs[],
                        int regnum)
diff --git a/gdb/trad-frame.h b/gdb/trad-frame.h
index a11a2c29c9..0d04bf4127 100644
--- a/gdb/trad-frame.h
+++ b/gdb/trad-frame.h
@@ -23,6 +23,7 @@ 
 #include "frame.h"             /* For "struct frame_id".  */
 
 struct frame_info;
+struct regcache_map_entry;
 struct trad_frame_cache;
 
 /* A simple, or traditional frame cache.
@@ -45,6 +46,9 @@  void trad_frame_set_reg_realreg (struct trad_frame_cache *this_trad_cache,
                                 int regnum, int realreg);
 void trad_frame_set_reg_addr (struct trad_frame_cache *this_trad_cache,
                              int regnum, CORE_ADDR addr);
+void trad_frame_set_reg_regmap (struct trad_frame_cache *this_trad_cache,
+                               const struct regcache_map_entry *regmap,
+                               CORE_ADDR addr, size_t size);
 void trad_frame_set_reg_value (struct trad_frame_cache *this_cache,
                               int regnum, LONGEST val);