From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (vps-42846194.vps.ovh.net [IPv6:2001:41d0:801:2000::2400]) by sourceware.org (Postfix) with ESMTPS id 56424385AC3B for ; Mon, 25 Oct 2021 22:31:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 56424385AC3B Received: from ubuntu.lan (cust120-dsl54.idnet.net [212.69.54.120]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 8E1DB818C0; Mon, 25 Oct 2021 22:31:16 +0000 (UTC) Date: Mon, 25 Oct 2021 22:31:07 +0000 From: Lancelot SIX To: Zoran Zaric Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v3 08/28] Add deref method to location description classes Message-ID: <20211025223015.vx6aoonz6kut2o3a@ubuntu.lan> References: <20211014093235.69756-1-zoran.zaric@amd.com> <20211014093235.69756-9-zoran.zaric@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211014093235.69756-9-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]); Mon, 25 Oct 2021 22:31:16 +0000 (UTC) X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_SBL_CSS, 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: Mon, 25 Oct 2021 22:31:19 -0000 Hi, I include few comments in the patch bellow. On Thu, Oct 14, 2021 at 10:32:15AM +0100, Zoran Zaric via Gdb-patches wrote: > From: Zoran Zaric > > Concept of reading from a location seems to be too low level for the > DWARF standard. What the standard actually describes is a concept of > dereferencing, where the type of the operation result can be > specified in advance. > > This can be seen in the definition of the DW_OP_derefX family of > expression operations, but it is also happening implicitly in the case > of DW_OP_fbreg, DW_OP_regval_type and DW_OP_bregX family of operations. > > Currently, the DW_OP_derefX operations will take the value from the > DWARF expression stack and implicitly convert it to a memory location > description (in reality treat it as a memory address for a given > target) and apply the dereference operation to it. When we allow any > location description on a DWARF expression stack, these operations need > to work in the same way. > > The conclusion here is that we need an universal method that model the *a* universal method that *models* > dereference operation for any class derived from a location description > class. > > It is worth mentioning that because of how the passed in buffers are > currently being implemented, we needed a specialisation for the deref > method of the dwarf_memory class to support them. > > gdb/ChangeLog: > > * dwarf2/expr.c (dwarf_location::deref): New method. > (dwarf_memory::deref): New method. > --- > gdb/dwarf2/expr.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 121 insertions(+) > > diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c > index 83a2db028ca..87caba5fd62 100644 > --- a/gdb/dwarf2/expr.c > +++ b/gdb/dwarf2/expr.c > @@ -422,6 +422,18 @@ class dwarf_location : public dwarf_entry > bool big_endian, int *optimized, > int *unavailable) const = 0; > > + /* Apply dereference operation on the DWARF location description. > + Operation returns a DWARF value of a given TYPE type while FRAME > + contains a frame context information of the location. ADDR_INFO > + (if present) describes a passed in memory buffer if a regular > + memory read is not desired for certain address range. If the SIZE > + is specified, it must be equal or smaller then the TYPE type size. then -> than ? > + If SIZE is smaller then the type size, the value will be zero then -> than ? > + extended to the difference. */ > + virtual std::unique_ptr deref > + (frame_info *frame, const property_addr_info *addr_info, > + struct type *type, size_t size = 0) const; > + > protected: > /* Architecture of the location. */ > gdbarch *m_arch; > @@ -489,6 +501,43 @@ class dwarf_value : public dwarf_entry > > using dwarf_value_up = std::unique_ptr; > > +std::unique_ptr > +dwarf_location::deref (frame_info *frame, const property_addr_info *addr_info, > + struct type *type, size_t size) const > +{ > + bool big_endian = type_byte_order (type) == BFD_ENDIAN_BIG; > + size_t actual_size = size != 0 ? size : TYPE_LENGTH (type); > + > + if (actual_size > TYPE_LENGTH (type)) > + ill_formed_expression (); > + > + /* If the size of the object read from memory is different > + from the type length, we need to zero-extend it. */ > + gdb::byte_vector read_buf (TYPE_LENGTH (type), 0); > + gdb_byte *buf_ptr = read_buf.data (); > + int optimized, unavailable; > + > + if (big_endian) > + buf_ptr += TYPE_LENGTH (type) - actual_size; > + > + this->read (frame, buf_ptr, 0, actual_size * HOST_CHAR_BIT, > + 0, 0, big_endian, &optimized, &unavailable); > + > + if (optimized) > + throw_error (OPTIMIZED_OUT_ERROR, > + _("Can't do read-modify-write to " > + "update bitfield; containing word " > + "has been optimized out")); The error message should be about dereferencing I guess. > + if (unavailable) > + throw_error (NOT_AVAILABLE_ERROR, > + _("Can't dereference " > + "update bitfield; containing word " > + "is unavailable")); > + > + return make_unique > + (gdb::array_view (read_buf), type); > +} > + > /* Undefined location description entry. This is a special location > description type that describes the location description that is > not known. */ > @@ -541,6 +590,11 @@ class dwarf_memory final : public dwarf_location > size_t location_bit_limit, bool big_endian, > int *optimized, int *unavailable) const override; > > + std::unique_ptr deref (frame_info *frame, > + const property_addr_info *addr_info, > + struct type *type, > + size_t size = 0) const override; > + > private: > /* True if the location belongs to a stack memory region. */ > bool m_stack; > @@ -663,6 +717,73 @@ dwarf_memory::write (frame_info *frame, const gdb_byte *buf, > } > } > > +std::unique_ptr > +dwarf_memory::deref (frame_info *frame, const property_addr_info *addr_info, > + struct type *type, size_t size) const > +{ > + bool big_endian = type_byte_order (type) == BFD_ENDIAN_BIG; > + size_t actual_size = size != 0 ? size : TYPE_LENGTH (type); > + > + if (actual_size > TYPE_LENGTH (type)) > + ill_formed_expression (); > + > + gdb::byte_vector read_buf (TYPE_LENGTH (type), 0); > + size_t size_in_bits = actual_size * HOST_CHAR_BIT; > + gdb_byte *buf_ptr = read_buf.data (); > + bool passed_in_buf = false; > + > + if (big_endian) > + buf_ptr += TYPE_LENGTH (type) - actual_size; > + > + /* Covers the case where we have a passed in memory that is not > + part of the target and requires for the location description > + to address it instead of addressing the actual target > + memory. */ > + LONGEST this_size = bits_to_bytes (m_bit_suboffset, size_in_bits); > + > + /* We shouldn't have a case where we read from a passed in > + memory and the same memory being marked as stack. */ > + if (!m_stack && this_size && addr_info != nullptr > + && addr_info->valaddr.data () != nullptr) > + { > + CORE_ADDR offset = (CORE_ADDR) m_offset - addr_info->addr; > + /* Using second buffer here because the copy_bitwise > + doesn't support in place copy. */ > + gdb::byte_vector temp_buf (this_size); I guess the temp_buf can be declared in the if bellow. If addr_info->valaddr does not contain the data we are looking for, there is no need to create a unused buffer on the stack. > + > + if (offset < addr_info->valaddr.size () > + && offset + this_size <= addr_info->valaddr.size ()) > + { > + memcpy (temp_buf.data (), addr_info->valaddr.data (), this_size); Isn't it addr_info->valaddr.data() + offset ? > + copy_bitwise (buf_ptr, 0, temp_buf.data (), > + m_bit_suboffset, size_in_bits, big_endian); > + passed_in_buf = true; > + } > + } > + > + if (!passed_in_buf) > + { > + int optimized, unavailable; > + > + this->read (frame, buf_ptr, 0, size_in_bits, 0, 0, > + big_endian, &optimized, &unavailable); > + > + if (optimized) > + throw_error (OPTIMIZED_OUT_ERROR, > + _("Can't do read-modify-write to " > + "update bitfield; containing word " > + "has been optimized out")); The message should also be about dereferencing I guess. Best, Lancelot. > + if (unavailable) > + throw_error (NOT_AVAILABLE_ERROR, > + _("Can't dereference " > + "update bitfield; containing word " > + "is unavailable")); > + } > + > + return make_unique > + (gdb::array_view (read_buf), type); > +} > + > /* Register location description entry. */ > > class dwarf_register final : public dwarf_location > -- > 2.17.1 >