From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 0AF053858C2D for ; Fri, 19 Aug 2022 16:39:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0AF053858C2D Received: from [172.16.0.95] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id B8DD11E222; Fri, 19 Aug 2022 12:39:43 -0400 (EDT) Message-ID: <0895de32-0b1b-4053-7a90-21d12fe81b6a@simark.ca> Date: Fri, 19 Aug 2022 12:39:43 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH 1/1] gdb/dwarf2: Fix 'rw_pieced_value' for values casted to different type. Content-Language: fr To: Stephan Rohr , gdb-patches@sourceware.org Cc: tom@tromey.com References: <20220422144420.3545190-1-stephan.rohr@intel.com> <20220422144420.3545190-2-stephan.rohr@intel.com> From: Simon Marchi In-Reply-To: <20220422144420.3545190-2-stephan.rohr@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Fri, 19 Aug 2022 16:39:46 -0000 On 4/22/22 10:44, Stephan Rohr via Gdb-patches wrote: > From: "Rohr, Stephan" > > The 'rw_pieced_value' function is executed when fetching a (lazy) > variable described by 'DW_OP_piece' or 'DW_OP_bit_piece'. The > function checks the 'type' and 'enclosing_type' fields of the value > for identity. > > * The 'type' field describes the type of a value. > * In most cases, the 'enclosing_type' field is identical to the > 'type' field. > * Scenarios where the 'type' and 'enclosing_type' of an object > differ are described in 'gdb/value.c'. Possible cases are: > * If a value represents a C++ object, then the 'type' field > gives the object's compile-time type. If the object actually > belongs to some class derived from `type', perhaps with other > base classes and additional members, then `type' is just a > subobject of the real thing, and the full object is probably > larger than `type' would suggest. > * If 'type' is a dynamic class (i.e. one with a vtable), then GDB > can actually determine the object's run-time type by looking at > the run-time type information in the vtable. GDB may then elect > to read the entire object. > * If the user casts a variable to a different type > (e.g. 'print ( []) '), the value's type is > updated before reading the value. Out of curiosity, where do you see these scenarios described? I could not find where value.c talks about the case where the user casts the variable. > > If a lazy value is fetched, GDB allocates space based on the > enclosing type's length and typically reads the 'full' object. > This is not implemented for pieced values and causes an internal > error if 'type' and 'enclosing_type' of a value are not identical. > > However, GDB can read the value based on its type. Thus, it should be > sufficient to check if the type's length (potentially shifted by > 'embedded_offset') does not exceed the enclosing type's length which was > used for memory allocation. I'm trying to understand a bit better what happens here. The current assertion states that it's incorrect to have a lazy value where the type does not match the enclosing_type (I don't really know why), but it clearly happens here. So I'd like to determine who's right, the assertion or the code that produces such a value. For reference, the assertion was added here: https://pi.simark.ca/gdb-patches/m3hbmbviko.fsf@fleche.redhat.com/#t I was wondering at which point exactly we end up with such a value. It's here that we assign a new type (the array type) to the value, while the value is lazy, when handling the value cast: #0 deprecated_set_value_type (value=0x61100020a700, type=0x621000506890) at /home/smarchi/src/binutils-gdb/gdb/value.c:1105 #1 0x0000564249d99092 in value_cast (type=0x621000506730, arg2=0x61100020a700) at /home/smarchi/src/binutils-gdb/gdb/valops.c:496 #2 0x000056424838294c in expr::var_value_operation::evaluate_for_cast (this=0x6030002a6220, to_type=0x621000506730, exp=0x60300011cbc0, noside=EVAL_NORMAL) at /home/smarchi/src/binutils-gdb/gdb/eval.c:2885 #3 0x0000564247412e1e in expr::unop_cast_type_operation::evaluate (this=0x6030002a6250, expect_type=0x0, exp=0x60300011cbc0, noside=EVAL_NORMAL) at /home/smarchi/src/binutils-gdb/gdb/expop.h:2036 #4 0x000056424836092c in expression::evaluate (this=0x60300011cbc0, expect_type=0x0, noside=EVAL_NORMAL) at /home/smarchi/src/binutils-gdb/gdb/eval.c:101 #5 0x0000564248360a9d in evaluate_expression (exp=0x60300011cbc0, expect_type=0x0) at /home/smarchi/src/binutils-gdb/gdb/eval.c:115 #6 0x0000564248e56c95 in process_print_command_args (args=0x60200006fb52 "(int []) s1", print_opts=0x7ffe0fb62200, voidprint=true) at /home/smarchi/src/binutils-gdb/gdb/printcmd.c:1307 #7 0x0000564248e56e79 in print_command_1 (args=0x60200006fb52 "(int []) s1", voidprint=1) at /home/smarchi/src/binutils-gdb/gdb/printcmd.c:1320 #8 0x0000564248e57d15 in print_command (exp=0x60200006fb52 "(int []) s1", from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/printcmd.c:1453 Meanwhile, the enclosing_type remains the original struct type. So my question is: when doing the value cast, do we also want to overwrite the value's enclosing type, in addition to the value's type? Is it useful to retain the original enclosing type? In other words, we could do this change to also set the enclosing type when casting: diff --git a/gdb/valops.c b/gdb/valops.c index 27e84d9f6b3..cb3fb49e50d 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -493,10 +493,10 @@ value_cast (struct type *type, struct value *arg2) TYPE_TARGET_TYPE (range_type), low_bound, new_length + low_bound - 1); - deprecated_set_value_type (arg2, - create_array_type (NULL, - element_type, - range_type)); + struct type *new_type + = create_array_type (NULL, element_type, range_type); + deprecated_set_value_type (arg2, new_type); + set_value_enclosing_type (arg2, new_type); return arg2; } } I don't know if that change causes other things to fail, and I really don't know what the answer is. One case I wonder about is when the size of the original object is not a multiple of the array's element size. For instance, imagine s1 is 9 bytes long and you do: (gdb) print (int []) s1 where an int is 4 bytes. The pieces of s1 will still describe 9 bytes, but the type and enclosing type will be 8 bytes long. That might be a problem down the road, and a reason we want to keep the original enclosing type whose length matches the described content length. Some additional tests for that would be nice. > --- > gdb/dwarf2/expr.c | 6 ++---- > gdb/testsuite/gdb.dwarf2/shortpiece.exp | 12 ++++++++++++ > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c > index 99862583336..6330a5787fc 100644 > --- a/gdb/dwarf2/expr.c > +++ b/gdb/dwarf2/expr.c > @@ -174,10 +174,8 @@ rw_pieced_value (value *v, value *from, bool check_optimized) > } > else > { > - 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")); > + gdb_assert ((TYPE_LENGTH (value_type (v)) + value_embedded_offset (v)) > + <= TYPE_LENGTH (value_enclosing_type (v))); This looks like an assertion that should hold for all values, all the time. When a value has an enclosing type different than its type, the type should be strictly a subset of the enclosing type. So, it seems a bit odd to check this here specifically. It would maybe make more sense to check it when modifying the type or enclosing_type of a value. In any case, that's separate from the current patch. If we find out that it is correct to remove the existing internal_error call, then I would just simply remove it (replace it with nothing). Simon