From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 8D3163858005 for ; Mon, 25 Oct 2021 21:38:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8D3163858005 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 19PLb9XV004407 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 25 Oct 2021 17:37:13 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 19PLb9XV004407 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id E565B1ECEB; Mon, 25 Oct 2021 17:37:08 -0400 (EDT) Message-ID: <7baf311a-d347-b5d3-e07b-b53c87ef1d38@polymtl.ca> Date: Mon, 25 Oct 2021 17:37:08 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.1 Subject: Re: [PATCH v3 06/28] Add read method to location description classes Content-Language: en-US To: Lancelot SIX , Zoran Zaric Cc: gdb-patches@sourceware.org References: <20211014093235.69756-1-zoran.zaric@amd.com> <20211014093235.69756-7-zoran.zaric@amd.com> <20211025183356.4cw5xqpvyprgayfs@ubuntu.lan> From: Simon Marchi In-Reply-To: <20211025183356.4cw5xqpvyprgayfs@ubuntu.lan> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 25 Oct 2021 21:37:09 +0000 X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, 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 21:38:19 -0000 On 2021-10-25 14:33, Lancelot SIX via Gdb-patches wrote: > Hi, > > On Thu, Oct 14, 2021 at 10:32:13AM +0100, Zoran Zaric via Gdb-patches wrote: >> From: Zoran Zaric >> >> After adding the interface for register and memory location access, a >> new method for reading from any location derived from a dwarf_location >> class, can now be defined. >> >> In the case of implicit pointer location description the existing >> indirect_synthetic_pointer interface requiers a type of the pointer >> to be specified, so for a generic read interface the only type that >> makes sense is the DWARF generic type. This means that the existing >> address_type method of a dwarf_expr_context class needs to be exposed >> outside of that class. >> >> gdb/ChangeLog: >> >> * dwarf2/expr.c (dwarf_location::read): New method. >> (dwarf_undefined::read): New method. >> (dwarf_memory::read): New method. >> (dwarf_register::read): New method. >> (dwarf_implicit::read): New method. >> (dwarf_implicit_pointer::read): New method. >> (dwarf_composite::read): New method. >> (dwarf_expr_context::address_type): Change to use the new >> address_type function. >> (address_type): New function. >> --- >> gdb/dwarf2/expr.c | 305 ++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 284 insertions(+), 21 deletions(-) >> >> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c >> index 56f17f52756..3f3e09db7cc 100644 >> --- a/gdb/dwarf2/expr.c >> +++ b/gdb/dwarf2/expr.c >> @@ -289,6 +289,36 @@ write_to_memory (CORE_ADDR address, const gdb_byte *buffer, >> length, buffer); >> } >> >> +/* Return the type used for DWARF operations where the type is >> + generic in the DWARF spec, the ARCH is a target architecture. > > There is an extra '.' at the end of this line. > >> + of the type and ADDR_SIZE is expected size of the address. >> + Only certain sizes are supported. */ >> + >> +static type * >> +address_type (gdbarch *arch, int addr_size) >> +{ >> + dwarf_gdbarch_types *types >> + = (dwarf_gdbarch_types *) gdbarch_data (arch, dwarf_arch_cookie); >> + int ndx; >> + >> + if (addr_size == 2) >> + ndx = 0; >> + else if (addr_size == 4) >> + ndx = 1; >> + else if (addr_size == 8) >> + ndx = 2; >> + else >> + error (_("Unsupported address size in DWARF expressions: %d bits"), >> + HOST_CHAR_BIT * addr_size); >> + >> + if (types->dw_types[ndx] == NULL) > > It is not a big deal, but I think nullptr is preferred over NULL for > new/updated code (there are few across the series, I can point them out > if you want). > >> + types->dw_types[ndx] >> + = arch_integer_type (arch, HOST_CHAR_BIT * addr_size, >> + 0, ""); >> + >> + return types->dw_types[ndx]; >> +} >> + >> class dwarf_location; >> class dwarf_memory; >> class dwarf_value; >> @@ -349,6 +379,27 @@ class dwarf_location : public dwarf_entry >> ill_formed_expression (); >> } >> >> + /* Read contents from the described location. >> + >> + The read operation is performed in the context of a FRAME. >> + BIT_SIZE is the number of bits to read. The data read is copied >> + to the caller-managed buffer BUF. BIG_ENDIAN defines the >> + endianness of the target. BITS_TO_SKIP is a bit offset into the >> + location and BUF_BIT_OFFSET is buffer BUF's bit offset. >> + LOCATION_BIT_LIMIT is a maximum number of bits that location can >> + hold, where value zero signifies that there is no such >> + restriction. >> + >> + Note that some location types can be read without a FRAME context. >> + >> + If the location is optimized out or unavailable, the OPTIMIZED and >> + UNAVAILABLE outputs are set accordingly. */ >> + virtual void read (frame_info *frame, gdb_byte *buf, >> + int buf_bit_offset, size_t bit_size, >> + LONGEST bits_to_skip, size_t location_bit_limit, >> + bool big_endian, int *optimized, >> + int *unavailable) const = 0; >> + >> protected: >> /* Architecture of the location. */ >> gdbarch *m_arch; >> @@ -426,6 +477,14 @@ class dwarf_undefined final : public dwarf_location >> dwarf_undefined (gdbarch *arch) >> : dwarf_location (arch, 0) >> {} >> + >> + void read (frame_info *frame, gdb_byte *buf, int buf_bit_offset, >> + size_t bit_size, LONGEST bits_to_skip, size_t location_bit_limit, >> + bool big_endian, int *optimized, int *unavailable) const override >> + { >> + *unavailable = 0; >> + *optimized = 1; >> + } >> }; >> >> class dwarf_memory final : public dwarf_location >> @@ -442,6 +501,11 @@ class dwarf_memory final : public dwarf_location >> >> dwarf_value_up to_value (struct type *type) const override; >> >> + void read (frame_info *frame, gdb_byte *buf, int buf_bit_offset, >> + size_t bit_size, LONGEST bits_to_skip, >> + size_t location_bit_limit, bool big_endian, >> + int *optimized, int *unavailable) const override; >> + >> private: >> /* True if the location belongs to a stack memory region. */ >> bool m_stack; >> @@ -468,6 +532,46 @@ dwarf_memory::to_value (struct type *type) const >> return make_unique (m_offset, type); >> } >> >> +void >> +dwarf_memory::read (frame_info *frame, gdb_byte *buf, >> + int buf_bit_offset, size_t bit_size, >> + LONGEST bits_to_skip, size_t location_bit_limit, >> + bool big_endian, int *optimized, int *unavailable) const >> +{ >> + LONGEST total_bits_to_skip = bits_to_skip; >> + CORE_ADDR start_address >> + = m_offset + (m_bit_suboffset + total_bits_to_skip) / HOST_CHAR_BIT; > > I am not entirely sure of what follows, so if a maintainer can step in, > I would appreciate. > > I am not sure HOST_CHAR_BIT is what should be used here. This is meant > to compute an address on the target. The number of bits per byte might > (in theory) be different on the host and the target. From what I > understand, this is where gdbarch_addressable_memory_unit_size could > come into play (it gives the number of octets associated with a memory > address for the given arch). > > In practice however, I do not know what target has non 8-bit bytes. > set_gdbarch_addressable_memory_unit_size seems to never be called in the > codebase, so I guess if such target has gdb support, it is off-tree. > > As I said before, I am not entirely sure to what extent such target > are currently supported. Is this something you have considered? It's true that there isn't an architecture upstream which uses that feature. I was the one who introduced gdbarch_addressable_memory_unit_size, while at my previous job, because we had a platform with non-8-bit-bytes. And this was merged, as it was seen as an improvement over using TARGET_CHAR_BIT (TARGET_CHAR_BIT is a compile-time constant, so if you are building a GDB that debugs architectures with different byte sizes... that doesn't work). So yeah, gdbarch_addressable_memory_unit_size looks like the right thing to use here. Note that this function returns 1 for x86, and 2 for an architecture with 16-bit bytes. So you would divide by: gdbarch_addressable_memory_unit_size (arch) * 8 I am fine with using the magical number 8 here, because gdbarch_addressable_memory_unit_size is explicitly defined as "the size in 8-bit bytes of...". Using CHAR_BIT or something like that would defeat the purpose because it could *in theory* be something else than 8. So cases like this one, we do a best effort to write things in a way that works for non-8-bit-bytes platform, but don't sweat it too much. Users downstream who actually use that will fix it and provide patches if needed. Simon