This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2 1/4] Add helper functions to trad_frame to support register cache maps.


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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]