From: Lancelot SIX <lsix@lancelotsix.com>
To: Zoran Zaric <zoran.zaric@amd.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH v3 23/28] Add support for any location description in CFI
Date: Sun, 31 Oct 2021 22:58:08 +0000 [thread overview]
Message-ID: <20211031225757.3kvczlsig7lfyzvu@ubuntu.lan> (raw)
In-Reply-To: <20211014093235.69756-24-zoran.zaric@amd.com>
Hi,
I have included minor nits below
On Thu, Oct 14, 2021 at 10:32:30AM +0100, Zoran Zaric via Gdb-patches wrote:
> From: Zoran Zaric <Zoran.Zaric@amd.com>
>
> One of the main benefits of allowing location description to be on the
> DWARF stack is that now CFI expression based register rules can be
> defined using a location description operations. This allows a register
> of one frame to be saved in any location, including any composite
> location.
>
> To fully support this feature, the execute_stack_op function in
> dwarf2/frame.c needs to return a single struct value object instead of
> just an address.
>
> Function put_frame_register_bytes also needs to change to support any
> location description.
>
> This support is a one of the key features to truly support optimized
> code.
>
> gdb/ChangeLog:
>
> * dwarf2/frame.c (execute_stack_op): Change to return a struct
> value object.
> (dwarf2_frame_cache): Change to call new execute_stack_op
> definition.
> (dwarf2_frame_prev_register): Change to call new execute_stack_op
> definition.
> * frame.c (put_frame_register_bytes): Add support for writing to
> composite location description.
> ---
> gdb/dwarf2/frame.c | 54 ++++++++++++++++++++++++++--------------------
> gdb/frame.c | 36 +++++++++++++++++++++++++------
> 2 files changed, 61 insertions(+), 29 deletions(-)
>
> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
> index e17b36e243b..e70dcd5a86e 100644
> --- a/gdb/dwarf2/frame.c
> +++ b/gdb/dwarf2/frame.c
> @@ -236,16 +236,17 @@ register %s (#%d) at %s"),
> }
> }
>
> -static CORE_ADDR
> +static value *
> execute_stack_op (const gdb_byte *exp, ULONGEST len, int addr_size,
> struct frame_info *this_frame, CORE_ADDR initial,
> - int initial_in_stack_memory, dwarf2_per_objfile *per_objfile)
> + int initial_in_stack_memory, dwarf2_per_objfile *per_objfile,
> + struct type* type = nullptr, bool as_lval = true)
> {
> scoped_value_mark free_values;
> - struct type *type = address_type (per_objfile->objfile->arch (),
> - addr_size);
> + struct type *init_type = address_type (per_objfile->objfile->arch (),
> + addr_size);
>
> - value *init_value = value_at_lazy (type, initial);
> + value *init_value = value_at_lazy (init_type, initial);
> std::vector<value *> init_values;
>
> set_value_stack (init_value, initial_in_stack_memory);
> @@ -255,10 +256,15 @@ execute_stack_op (const gdb_byte *exp, ULONGEST len, int addr_size,
> = dwarf2_evaluate (exp, len, true, per_objfile, nullptr,
> this_frame, addr_size, &init_values, nullptr);
>
> - if (VALUE_LVAL (result_val) == lval_memory)
> - return value_address (result_val);
> - else
> - return value_as_address (result_val);
> + /* We need to clean up all the values that are not needed any more.
> + The problem with a value_ref_ptr class is that it disconnects the
> + RETVAL from the value garbage collection, so we need to make
> + a copy of that value on the stack to keep everything consistent.
> + The value_ref_ptr will clean up after itself at the end of this block. */
> + value_ref_ptr value_holder = value_ref_ptr::new_reference (result_val);
> + free_values.free_to_mark ();
> +
> + return value_copy (result_val);
> }
> \f
>
> @@ -989,10 +995,14 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
> break;
>
> case CFA_EXP:
> - cache->cfa =
> - execute_stack_op (fs.regs.cfa_exp, fs.regs.cfa_exp_len,
> - cache->addr_size, this_frame, 0, 0,
> - cache->per_objfile);
> + {
> + struct value *value
> + = execute_stack_op (fs.regs.cfa_exp, fs.regs.cfa_exp_len,
> + cache->addr_size, this_frame, 0, 0,
> + cache->per_objfile);
> + cache->cfa = value_address (value);
> + }
> +
> break;
>
> default:
> @@ -1190,24 +1200,22 @@ dwarf2_frame_prev_register (struct frame_info *this_frame, void **this_cache,
> return frame_unwind_got_register (this_frame, regnum, realnum);
>
> case DWARF2_FRAME_REG_SAVED_EXP:
> - addr = execute_stack_op (cache->reg[regnum].loc.exp.start,
> + return execute_stack_op (cache->reg[regnum].loc.exp.start,
> cache->reg[regnum].loc.exp.len,
> - cache->addr_size,
> - this_frame, cache->cfa, 1,
> - cache->per_objfile);
> - return frame_unwind_got_memory (this_frame, regnum, addr);
> + cache->addr_size, this_frame,
> + cache->cfa, 1, cache->per_objfile,
> + register_type (gdbarch, regnum));
>
> case DWARF2_FRAME_REG_SAVED_VAL_OFFSET:
> addr = cache->cfa + cache->reg[regnum].loc.offset;
> return frame_unwind_got_constant (this_frame, regnum, addr);
>
> case DWARF2_FRAME_REG_SAVED_VAL_EXP:
> - addr = execute_stack_op (cache->reg[regnum].loc.exp.start,
> + return execute_stack_op (cache->reg[regnum].loc.exp.start,
> cache->reg[regnum].loc.exp.len,
> - cache->addr_size,
> - this_frame, cache->cfa, 1,
> - cache->per_objfile);
> - return frame_unwind_got_constant (this_frame, regnum, addr);
> + cache->addr_size, this_frame,
> + cache->cfa, 1, cache->per_objfile,
> + register_type (gdbarch, regnum), false);
>
> case DWARF2_FRAME_REG_UNSPECIFIED:
> /* GCC, in its infinite wisdom decided to not provide unwind
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 16673258373..3d85d2c7b59 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1532,26 +1532,50 @@ put_frame_register_bytes (struct frame_info *frame, int regnum,
> {
> int curr_len = register_size (gdbarch, regnum) - offset;
>
> + struct value *value = frame_unwind_register_value (frame->next,
> + regnum);
> +
> if (curr_len > len)
> curr_len = len;
>
> const gdb_byte *myaddr = buffer.data ();
> - if (curr_len == register_size (gdbarch, regnum))
> +
> + /* Compute value is a special new case. The problem is that
^^
There is one extra space at the start of the comment (and in the
subsequent lines I guess).
Also the comment related to a 'new case'. From the perspective of
someone reading the comment in frame.c (not in a patch), this just like
a special case. Maybe rephrase with something like:
Computed value is a special case. The computed callback
mechanism requires a strut value argument, so we need to make
one.
> + the computed callback mechanism only supports a struct
> + value arguments, so we need to make one. */
> + if (value != NULL && VALUE_LVAL (value) == lval_computed)
Prefer nullptr over NULL.
> + {
> + const lval_funcs *funcs = value_computed_funcs (value);
> + type * reg_type = register_type (gdbarch, regnum);
I guess funcs->write could be checked to be non nullptr before
retrieving regtype. If 'error' is called, reg_type has no use.
> +
> + if (funcs->write == NULL)
NULL -> nullptr
> + error (_("Attempt to assign to an unmodifiable value."));
> +
> + struct value *from_value = allocate_value (reg_type);
> + memcpy (value_contents_raw (from_value), myaddr,
> + TYPE_LENGTH (reg_type));
> +
> + set_value_offset (value, offset);
> +
> + funcs->write (value, from_value);
> + release_value (from_value);
> + }
> + else if (curr_len == register_size (gdbarch, regnum))
> {
> put_frame_register (frame, regnum, myaddr);
> }
> else
> {
> - struct value *value = frame_unwind_register_value (frame->next,
> - regnum);
> gdb_assert (value != NULL);
>
> - memcpy ((char *) value_contents_writeable (value) + offset, myaddr,
> - curr_len);
> + memcpy ((char *) value_contents_writeable (value) + offset,
> + myaddr, curr_len);
> put_frame_register (frame, regnum, value_contents_raw (value));
> - release_value (value);
> }
>
> + if (value != NULL)
NULL -> nullptr
Best,
Lancelot.
> + release_value (value);
> +
> myaddr += curr_len;
> len -= curr_len;
> offset = 0;
> --
> 2.17.1
>
next prev parent reply other threads:[~2021-10-31 22:58 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-14 9:32 [PATCH v3 00/28] Allow location description on the DWARF stack Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 01/28] Add new register access interface to expr.c Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 02/28] Add new memory " Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 03/28] Add new classes that model DWARF stack element Zoran Zaric
2021-10-21 23:43 ` Lancelot SIX
2021-10-22 16:38 ` Zoran Zaric
2021-10-22 21:34 ` Lancelot SIX
2021-10-14 9:32 ` [PATCH v3 04/28] Add to_location method to DWARF entry classes Zoran Zaric
2021-10-22 21:21 ` Lancelot SIX
2021-10-25 21:23 ` Simon Marchi
2021-11-01 16:01 ` Zoran Zaric
2021-11-01 20:36 ` Simon Marchi
2021-11-01 16:00 ` Zoran Zaric
2021-11-01 17:48 ` Lancelot SIX
2021-10-14 9:32 ` [PATCH v3 05/28] Add to_value " Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 06/28] Add read method to location description classes Zoran Zaric
2021-10-25 18:33 ` Lancelot SIX
2021-10-25 21:37 ` Simon Marchi
2021-11-02 14:26 ` Zoran Zaric
2021-11-03 19:03 ` Simon Marchi
2021-11-05 11:58 ` Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 07/28] Add write " Zoran Zaric
2021-10-25 20:21 ` Lancelot SIX
2021-11-03 10:27 ` Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 08/28] Add deref " Zoran Zaric
2021-10-25 22:31 ` Lancelot SIX
2021-11-03 10:51 ` Zoran Zaric
2021-11-03 17:37 ` Simon Marchi
2021-11-05 11:55 ` Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 09/28] Add read_from_gdb_value method to dwarf_location Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 10/28] Add write_to_gdb_value " Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 11/28] Add is_implicit_ptr_at " Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 12/28] Add indirect_implicit_ptr to dwarf_location class Zoran Zaric
2021-10-26 20:52 ` Lancelot SIX
2021-11-03 15:11 ` Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 13/28] Add is_optimized_out " Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 14/28] Add new computed struct value callback interface Zoran Zaric
2021-10-26 22:50 ` Lancelot SIX
2021-11-04 11:32 ` Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 15/28] Add to_gdb_value method to DWARF entry class Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 16/28] Change DWARF stack to use new dwarf_entry classes Zoran Zaric
2021-10-31 17:58 ` Lancelot SIX
2021-11-04 12:43 ` Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 17/28] Remove old computed struct value callbacks Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 18/28] Comments cleanup between expr.h and expr.c Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 19/28] Remove dwarf_expr_context from expr.h interface Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 20/28] Move read_addr_from_reg function to frame.c Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 21/28] Add frame info check to DW_OP_reg operations Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 22/28] Remove DWARF expression composition check Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 23/28] Add support for any location description in CFI Zoran Zaric
2021-10-31 22:58 ` Lancelot SIX [this message]
2021-11-04 15:09 ` Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 24/28] Add DWARF operations for byte and bit offset Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 25/28] Add support for DW_OP_LLVM_undefined operation Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 26/28] Add support for nested composite locations Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 27/28] Add DW_OP_LLVM_extend DWARF operation Zoran Zaric
2021-11-01 21:48 ` Lancelot SIX
2021-11-04 16:26 ` Zoran Zaric
2021-10-14 9:32 ` [PATCH v3 28/28] Add DW_OP_LLVM_select_bit_piece " Zoran Zaric
2021-11-01 22:25 ` Lancelot SIX
2021-11-04 16:39 ` Zoran Zaric
2021-11-05 11:54 ` [PATCH v3 00/28] Allow location description on the DWARF stack Zaric, Zoran (Zare)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211031225757.3kvczlsig7lfyzvu@ubuntu.lan \
--to=lsix@lancelotsix.com \
--cc=gdb-patches@sourceware.org \
--cc=zoran.zaric@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).