From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 6FF47385802E for ; Tue, 26 Oct 2021 22:50:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6FF47385802E Received: from ubuntu.lan (cust120-dsl54.idnet.net [212.69.54.120]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 30893819D4; Tue, 26 Oct 2021 22:50:16 +0000 (UTC) Date: Tue, 26 Oct 2021 22:50:01 +0000 From: Lancelot SIX To: Zoran Zaric Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v3 14/28] Add new computed struct value callback interface Message-ID: <20211026225001.3h5rebymgr67rayo@ubuntu.lan> References: <20211014093235.69756-1-zoran.zaric@amd.com> <20211014093235.69756-15-zoran.zaric@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211014093235.69756-15-zoran.zaric@amd.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Tue, 26 Oct 2021 22:50:16 +0000 (UTC) X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Oct 2021 22:50:20 -0000 Hi, As for previous patches, I have included (minor) comments below. On Thu, Oct 14, 2021 at 10:32:21AM +0100, Zoran Zaric via Gdb-patches wrote: > From: Zoran Zaric > > At this point all support is there to add a new callback interface > for the computed struct value infrastructure. > > Original callback interface (piece closure) is going to be removed as > soon as the switch to the new DWARF entry classes is done in the next > few patches. > > gdb/ChangeLog: > > * dwarf2/expr.c (class computed_closure): New class. > (closure_value_funcs): New closure callback structure. > (copy_value_closure): New function. > (free_value_closure): New function. > (rw_closure_value): New function. > (check_synthetic_pointer): New function. > (write_closure_value): New function. > (read_closure_value): New function. > (is_optimized_out_closure_value): New function. > (indirect_closure_value): New function. > (coerce_closure_ref): New function. > --- > gdb/dwarf2/expr.c | 307 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 307 insertions(+) > > diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c > index 1b6622d5f59..e33be7e7c1c 100644 > --- a/gdb/dwarf2/expr.c > +++ b/gdb/dwarf2/expr.c > @@ -323,6 +323,92 @@ class dwarf_location; > class dwarf_memory; > class dwarf_value; > > +/* Closure callback functions. */ > + > +static void * > +copy_value_closure (const value *v); > + > +static void > +free_value_closure (value *v); > + > +static void > +rw_closure_value (value *v, value *from); > + > +static int > +check_synthetic_pointer (const value *value, LONGEST bit_offset, > + int bit_length); > + > +static void > +write_closure_value (value *to, value *from); > + > +static void > +read_closure_value (value *v); > + > +static bool > +is_optimized_out_closure_value (value *v); > + > +static value * > +indirect_closure_value (value *value); > + > +static value * > +coerce_closure_ref (const value *value); > + > +/* Functions for accessing a variable described by DW_OP_piece, > + DW_OP_bit_piece or DW_OP_implicit_pointer. */ > + > +static const lval_funcs closure_value_funcs = { > + read_closure_value, > + write_closure_value, > + is_optimized_out_closure_value, > + indirect_closure_value, > + coerce_closure_ref, > + check_synthetic_pointer, > + copy_value_closure, > + free_value_closure > +}; > + > +/* Closure class that encapsulates a DWARF location description and a > + frame information used when that location description was created. > + Used for lval_computed value abstraction. */ > + > +class computed_closure : public refcounted_object > +{ > +public: > + computed_closure (std::unique_ptr location, > + struct frame_id frame_id) > + : m_location (std::move (location)), m_frame_id (frame_id) > + {} > + > + computed_closure (std::unique_ptr location, > + struct frame_info *frame) > + : m_location (std::move (location)), m_frame (frame) > + {} > + > + const dwarf_location &get_location () const; Is there a reason not to inline this function definition as well? It seems as trivial as get_frame_id and get_frame. > + > + frame_id get_frame_id () const > + { If I understand correctly, m_frame_id and m_frame are mutually exclusive. To be on the safe side, I would be tempted to add a check here: gdb_assert (m_frame == nullptr); > + return m_frame_id; > + } > + > + frame_info *get_frame () const > + { > + return m_frame; > + } > + > +private: > + /* Entry that this class encloses. */ > + const std::unique_ptr m_location; > + > + /* Frame ID context of the closure. */ > + frame_id m_frame_id; > + > + /* In the case of frame expression evaluator the frame_id > + is not safe to use because the frame itself is being built. > + Only in these cases we set and use frame info directly. */ > + frame_info *m_frame = NULL; I guess nullptr is prefered over NULL (across the patch). > +}; > + > /* Base class that describes entries found on a DWARF expression > evaluation stack. */ > > @@ -524,6 +610,11 @@ class dwarf_location : public dwarf_entry > > using dwarf_location_up = std::unique_ptr; > > +const dwarf_location &computed_closure::get_location () const > +{ > + return *m_location; > +} > + > void > dwarf_location::read_from_gdb_value (frame_info *frame, struct value *value, > int value_bit_offset, > @@ -1594,6 +1685,222 @@ dwarf_composite::is_optimized_out (frame_info *frame, bool big_endian, > return false; > } > > +static void * > +copy_value_closure (const value *v) > +{ > + computed_closure *closure = ((computed_closure*) value_computed_closure (v)); > + > + if (closure == nullptr) > + internal_error (__FILE__, __LINE__, _("invalid closure type")); > + > + closure->incref (); > + return closure; > +} > + > +static void > +free_value_closure (value *v) > +{ > + computed_closure *closure = ((computed_closure*) value_computed_closure (v)); > + > + if (closure == nullptr) > + internal_error (__FILE__, __LINE__, _("invalid closure type")); > + > + closure->decref (); > + > + if (closure->refcount () == 0) > + delete closure; > +} > + > +/* Read or write a closure value V. If FROM != NULL, operate in "write > + mode": copy FROM into the closure comprising V. If FROM == NULL, > + operate in "read mode": fetch the contents of the (lazy) value V by > + composing it from its closure. */ > + > +static void > +rw_closure_value (value *v, value *from) > +{ > + LONGEST bit_offset = 0, max_bit_size; > + computed_closure *closure = (computed_closure*) value_computed_closure (v); > + bool big_endian = type_byte_order (value_type (v)) == BFD_ENDIAN_BIG; > + const dwarf_location &location = closure->get_location (); > + > + if (from == NULL) > + { > + if (value_type (v) != value_enclosing_type (v)) > + internal_error (__FILE__, __LINE__, > + _("Should not be able to create a lazy value with " > + "an enclosing type")); > + } > + > + ULONGEST bits_to_skip = HOST_CHAR_BIT * value_offset (v); > + > + /* If there are bits that don't complete a byte, count them in. */ > + if (value_bitsize (v)) > + { > + bits_to_skip += HOST_CHAR_BIT * value_offset (value_parent (v)) > + + value_bitpos (v); > + if (from != NULL && big_endian) > + { > + /* Use the least significant bits of FROM. */ > + max_bit_size = HOST_CHAR_BIT * TYPE_LENGTH (value_type (from)); > + bit_offset = max_bit_size - value_bitsize (v); > + } > + else > + max_bit_size = value_bitsize (v); > + } > + else > + max_bit_size = HOST_CHAR_BIT * TYPE_LENGTH (value_type (v)); > + > + frame_info *frame = closure->get_frame (); > + > + if (frame == NULL) > + frame = frame_find_by_id (closure->get_frame_id ()); > + > + if (from == NULL) > + { > + location.write_to_gdb_value (frame, v, bit_offset, bits_to_skip, > + max_bit_size - bit_offset, 0); > + } > + else > + { > + location.read_from_gdb_value (frame, from, bit_offset, bits_to_skip, > + max_bit_size - bit_offset, 0); > + } > +} > + > +static void > +read_closure_value (value *v) > +{ > + rw_closure_value (v, NULL); > +} > + > +static void > +write_closure_value (value *to, value *from) > +{ > + rw_closure_value (to, from); > +} > + > +/* Check if a closure value V contains describes any piece One of 'contains' and 'describes' might be too much here I guess. Best, Lancelot. > + of the underlying location description as optimized out. */ > + > +static bool > +is_optimized_out_closure_value (value *v) > +{ > + LONGEST max_bit_size; > + computed_closure *closure = (computed_closure*) value_computed_closure (v); > + bool big_endian = type_byte_order (value_type (v)) == BFD_ENDIAN_BIG; > + const dwarf_location &location = closure->get_location (); > + > + if (value_type (v) != value_enclosing_type (v)) > + internal_error (__FILE__, __LINE__, > + _("Should not be able to create a lazy value with " > + "an enclosing type")); > + > + ULONGEST bits_to_skip = HOST_CHAR_BIT * value_offset (v); > + > + /* If there are bits that don't complete a byte, count them in. */ > + if (value_bitsize (v)) > + { > + bits_to_skip += HOST_CHAR_BIT * value_offset (value_parent (v)) > + + value_bitpos (v); > + max_bit_size = value_bitsize (v); > + } > + else > + max_bit_size = HOST_CHAR_BIT * TYPE_LENGTH (value_type (v)); > + > + frame_info *frame = closure->get_frame (); > + > + if (frame == NULL) > + frame = frame_find_by_id (closure->get_frame_id ()); > + > + return location.is_optimized_out (frame, big_endian, bits_to_skip, > + max_bit_size, 0); > +} > + > +/* An implementation of an lval_funcs method to see whether a value is > + a synthetic pointer. */ > + > +static int > +check_synthetic_pointer (const value *value, LONGEST bit_offset, > + int bit_length) > +{ > + LONGEST total_bit_offset = bit_offset + HOST_CHAR_BIT * value_offset (value); > + > + if (value_bitsize (value)) > + total_bit_offset += value_bitpos (value); > + > + computed_closure *closure > + = (computed_closure *) value_computed_closure (value); > + > + return closure->get_location ().is_implicit_ptr_at (total_bit_offset, > + bit_length); > +} > + > +/* An implementation of an lval_funcs method to indirect through a > + pointer. This handles the synthetic pointer case when needed. */ > + > +static value * > +indirect_closure_value (value *value) > +{ > + computed_closure *closure > + = (computed_closure *) value_computed_closure (value); > + > + struct type *type = check_typedef (value_type (value)); > + if (type->code () != TYPE_CODE_PTR) > + return NULL; > + > + LONGEST bit_length = HOST_CHAR_BIT * TYPE_LENGTH (type); > + LONGEST bit_offset = HOST_CHAR_BIT * value_offset (value); > + > + if (value_bitsize (value)) > + bit_offset += value_bitpos (value); > + > + frame_info *frame = get_selected_frame (_("No frame selected.")); > + > + /* This is an offset requested by GDB, such as value subscripts. > + However, due to how synthetic pointers are implemented, this is > + always presented to us as a pointer type. This means we have to > + sign-extend it manually as appropriate. Use raw > + extract_signed_integer directly rather than value_as_address and > + sign extend afterwards on architectures that would need it > + (mostly everywhere except MIPS, which has signed addresses) as > + the later would go through gdbarch_pointer_to_address and thus > + return a CORE_ADDR with high bits set on architectures that > + encode address spaces and other things in CORE_ADDR. */ > + bfd_endian byte_order = gdbarch_byte_order (get_frame_arch (frame)); > + LONGEST pointer_offset > + = extract_signed_integer (value_contents (value), > + TYPE_LENGTH (type), byte_order); > + > + return closure->get_location ().indirect_implicit_ptr (frame, type, > + pointer_offset, > + bit_offset, bit_length); > +} > + > +/* Implementation of the coerce_ref method of lval_funcs for synthetic C++ > + references. */ > + > +static value * > +coerce_closure_ref (const value *value) > +{ > + struct type *type = check_typedef (value_type (value)); > + > + if (value_bits_synthetic_pointer (value, value_embedded_offset (value), > + TARGET_CHAR_BIT * TYPE_LENGTH (type))) > + { > + computed_closure *closure > + = (computed_closure *) value_computed_closure (value); > + frame_info *frame = get_selected_frame (_("No frame selected.")); > + > + return closure->get_location ().indirect_implicit_ptr (frame, type); > + } > + else > + { > + /* Else: not a synthetic reference; do nothing. */ > + return NULL; > + } > +} > + > struct piece_closure > { > /* Reference count. */ > -- > 2.17.1 >