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 26EA83857C52 for ; Thu, 21 Oct 2021 23:43:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 26EA83857C52 Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id AE228819D4; Thu, 21 Oct 2021 23:43:43 +0000 (UTC) Date: Thu, 21 Oct 2021 23:43:39 +0000 From: Lancelot SIX To: Zoran Zaric Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v3 03/28] Add new classes that model DWARF stack element Message-ID: <20211021234253.3n5proehdawuwy4l@ubuntu.lan> References: <20211014093235.69756-1-zoran.zaric@amd.com> <20211014093235.69756-4-zoran.zaric@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211014093235.69756-4-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]); Thu, 21 Oct 2021 23:43:43 +0000 (UTC) X-Spam-Status: No, score=-11.4 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: Thu, 21 Oct 2021 23:43:47 -0000 Hi, I have some remarks/comments/questions I have included in the patch. I am far from having reached the end of your series. I am not entirely sure if I should send comments as I progress or go through all of it and send all my comments at once when I have reached the end (which can take a while). Is there a way you prefer? Best, Lancelot. > --- > gdb/dwarf2/expr.c | 234 ++++++++++++++++++++++++++++++++++++++++++++++ > gdb/value.c | 2 +- > gdb/value.h | 2 + > 3 files changed, 237 insertions(+), 1 deletion(-) > > diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c > index c0ca78ce236..f44dfd1b260 100644 > --- a/gdb/dwarf2/expr.c > +++ b/gdb/dwarf2/expr.c > @@ -271,6 +271,240 @@ write_to_memory (CORE_ADDR address, const gdb_byte *buffer, > length, buffer); > } > > +/* Base class that describes entries found on a DWARF expression > + evaluation stack. */ > + > +class dwarf_entry > +{ > +public: > + /* Not expected to be called on it's own. */ > + dwarf_entry () = default; If this constructor is not meant to be called directly, I guest it could be possible to make it protected. This way it can no longer be used by a user of the class while still be usable by the child of this class. > + > + virtual ~dwarf_entry () = default; > +}; > + > +/* Location description entry found on a DWARF expression evaluation > + stack. > + > + Types of locations descirbed can be: register location, memory > + location, implicit location, implicit pointer location, undefined > + location and composite location (composed out of any of the location > + types including another composite location). */ > + > +class dwarf_location : public dwarf_entry > +{ > +public: > + /* Not expected to be called on it's own. */ > + dwarf_location (gdbarch *arch, LONGEST offset) > + : m_arch (arch), m_offset (offset) > + {} Same remark on the visibility of the constructor here (based on the same comment). > + > + virtual ~dwarf_location () = default; > + > + /* Add bit offset to the location description. */ > + void add_bit_offset (LONGEST bit_offset) > + { > + LONGEST bit_total_offset = m_bit_suboffset + bit_offset; > + > + m_offset += bit_total_offset / HOST_CHAR_BIT; > + m_bit_suboffset = bit_total_offset % HOST_CHAR_BIT; > + }; > + > + void set_initialised (bool initialised) > + { > + m_initialised = initialised; > + }; > + > +protected: > + /* Architecture of the location. */ > + gdbarch *m_arch; > + > + /* Byte offset into the location. */ > + LONGEST m_offset; > + > + /* Bit suboffset of the last byte. */ > + LONGEST m_bit_suboffset = 0; > + > + /* Whether the location is initialized. Used for non-standard > + DW_OP_GNU_uninit operation. */ > + bool m_initialised = true; > +}; > + > +/* Value entry found on a DWARF expression evaluation stack. */ > + > +class dwarf_value : public dwarf_entry It does not look like dwarf_value has child classes. It could be declared final as you have done for dwarf_undefined for example. > +{ > +public: > + dwarf_value (gdb::array_view contents, struct type *type) > + : m_contents (contents.begin (), contents.end ()), m_type (type) > + {} > + > + dwarf_value (ULONGEST value, struct type *type) > + : m_contents (TYPE_LENGTH (type)), m_type (type) > + { > + pack_unsigned_long (m_contents.data (), type, value); > + } > + > + dwarf_value (LONGEST value, struct type *type) > + : m_contents (TYPE_LENGTH (type)), m_type (type) > + { > + pack_unsigned_long (m_contents.data (), type, value); Shouldn't it be pack_long instead here? > + } > + > + gdb::array_view contents () const > + { > + return m_contents; > + } > + > + struct type* type () const Minor remark first so I do not forget: I think GDB coding style prefers 'type *' over 'type* ' (space before the '*', no space after). Unless there is a reason not to, I would be tempted to return a pointer to const (i.e. const struct type *) in order to maintain the const correctness here. One way out could be to have two versions of the type method: struct type *type (); const struct type *type () const; I have tested this on top of the entire series, it seems to be OK. > + { > + return m_type; > + } > + > + LONGEST to_long () const > + { > + return unpack_long (m_type, m_contents.data ()); > + } > + > +private: > + /* Value contents as a stream of bytes in target byte order. */ > + gdb::byte_vector m_contents; > + > + /* Type of the value held by the entry. */ > + struct type *m_type; > +}; > + > +/* Undefined location description entry. This is a special location > + description type that describes the location description that is > + not known. */ > + > +class dwarf_undefined final : public dwarf_location > +{ > +public: > + dwarf_undefined (gdbarch *arch) > + : dwarf_location (arch, 0) > + {} > +}; > + > +class dwarf_memory final : public dwarf_location > +{ > +public: > + dwarf_memory (gdbarch *arch, LONGEST offset, bool stack = false) > + : dwarf_location (arch, offset), m_stack (stack) > + {} > + > + void set_stack (bool stack) > + { > + m_stack = stack; > + }; > + > +private: > + /* True if the location belongs to a stack memory region. */ > + bool m_stack; > +}; > + > +/* Register location description entry. */ > + > +class dwarf_register final : public dwarf_location > +{ > +public: > + dwarf_register (gdbarch *arch, unsigned int regnum, LONGEST offset = 0) > + : dwarf_location (arch, offset), m_regnum (regnum) > + {} > + > +private: > + /* DWARF register number. */ > + unsigned int m_regnum; > +}; > + > +/* Implicit location description entry. Describes a location > + description not found on the target but instead saved in a > + gdb-allocated buffer. */ > + > +class dwarf_implicit final : public dwarf_location > +{ > +public: > + > + dwarf_implicit (gdbarch *arch, gdb::array_view contents, > + enum bfd_endian byte_order) > + : dwarf_location (arch, 0), > + m_contents (contents.begin (), contents.end ()), > + m_byte_order (byte_order) > + {} > + > +private: > + /* Implicit location contents as a stream of bytes in target byte-order. */ > + gdb::byte_vector m_contents; > + > + /* Contents original byte order. */ > + bfd_endian m_byte_order; > +}; > + > +/* Implicit pointer location description entry. */ > + > +class dwarf_implicit_pointer final : public dwarf_location > +{ > +public: > + dwarf_implicit_pointer (gdbarch *arch, > + dwarf2_per_objfile *per_objfile, > + dwarf2_per_cu_data *per_cu, > + int addr_size, sect_offset die_offset, > + LONGEST offset) > + : dwarf_location (arch, offset), > + m_per_objfile (per_objfile), m_per_cu (per_cu), > + m_addr_size (addr_size), m_die_offset (die_offset) > + {} > + > +private: > + /* Per object file data of the implicit pointer. */ > + dwarf2_per_objfile *m_per_objfile; > + > + /* Compilation unit context of the implicit pointer. */ > + dwarf2_per_cu_data *m_per_cu; > + > + /* Address size for the evaluation. */ > + int m_addr_size; > + > + /* DWARF die offset pointed by the implicit pointer. */ > + sect_offset m_die_offset; > +}; > + > +/* Composite location description entry. */ > + > +class dwarf_composite final : public dwarf_location > +{ > +public: > + dwarf_composite (gdbarch *arch, dwarf2_per_cu_data *per_cu) > + : dwarf_location (arch, 0), m_per_cu (per_cu) > + {} > + > + void add_piece (std::unique_ptr location, ULONGEST bit_size) > + { > + gdb_assert (location != nullptr); > + m_pieces.emplace_back (std::move (location), bit_size); > + } > + > +private: > + /* Composite piece that contains a piece location > + description and it's size. */ > + struct piece > + { > + public: > + piece (std::unique_ptr location, ULONGEST size) > + : location (std::move (location)), size (size) > + {} > + > + std::unique_ptr location; > + ULONGEST size; > + }; > + > + /* Compilation unit context of the pointer. */ > + dwarf2_per_cu_data *m_per_cu; > + > + /* Vector of composite pieces. */ > + std::vector m_pieces; > +}; > + > struct piece_closure > { > /* Reference count. */ > diff --git a/gdb/value.c b/gdb/value.c > index bb2adae0a51..0623d29a595 100644 > --- a/gdb/value.c > +++ b/gdb/value.c > @@ -3465,7 +3465,7 @@ pack_long (gdb_byte *buf, struct type *type, LONGEST num) > > /* Pack NUM into BUF using a target format of TYPE. */ > > -static void > +void > pack_unsigned_long (gdb_byte *buf, struct type *type, ULONGEST num) > { > LONGEST len; > diff --git a/gdb/value.h b/gdb/value.h > index 45012372dbf..a01dc8de9f7 100644 > --- a/gdb/value.h > +++ b/gdb/value.h > @@ -678,6 +678,8 @@ extern struct value *value_field_bitfield (struct type *type, int fieldno, > const struct value *val); > > extern void pack_long (gdb_byte *buf, struct type *type, LONGEST num); > +extern void pack_unsigned_long (gdb_byte *buf, struct type *type, > + ULONGEST num); > > extern struct value *value_from_longest (struct type *type, LONGEST num); > extern struct value *value_from_ulongest (struct type *type, ULONGEST num); > -- > 2.17.1 >