This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 1/4] Add helper functions to trad_frame to support register cache maps.
- From: Simon Marchi <simon dot marchi at ericsson dot com>
- To: John Baldwin <jhb at FreeBSD dot org>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Fri, 5 Oct 2018 20:18:11 +0000
- Subject: Re: [PATCH v2 1/4] Add helper functions to trad_frame to support register cache maps.
- References: <20180924205151.22217-1-jhb@FreeBSD.org> <20180924205151.22217-2-jhb@FreeBSD.org> <378fd054-ca62-e3fd-9da9-a053a77cd53e@ericsson.com> <93121ec2-866f-2d6f-4682-336ed043b4d0@FreeBSD.org>
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