From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id C67723857C5D for ; Thu, 11 Nov 2021 18:21:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C67723857C5D Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-230-_MGo5f6fPDapCn09pHkk-A-1; Thu, 11 Nov 2021 13:21:40 -0500 X-MC-Unique: _MGo5f6fPDapCn09pHkk-A-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B9DB71851724; Thu, 11 Nov 2021 18:21:39 +0000 (UTC) Received: from [10.3.112.120] (ovpn-112-120.phx2.redhat.com [10.3.112.120]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8537960C7F; Thu, 11 Nov 2021 18:21:39 +0000 (UTC) Message-ID: <21d1c1bc-8f09-6657-67f2-b13070ef62c4@redhat.com> Date: Thu, 11 Nov 2021 10:21:38 -0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH] Resolve dynamic types before computing pointer size To: Tom Tromey Cc: Keith Seitz via Gdb-patches References: <20210916164248.2065236-1-keiths@redhat.com> <874k9dx9xf.fsf@tromey.com> <87k0i7tlsw.fsf@tromey.com> <87y26ms0km.fsf@tromey.com> From: Keith Seitz In-Reply-To: <87y26ms0km.fsf@tromey.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-15.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, 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, 11 Nov 2021 18:21:46 -0000 On 10/21/21 11:19, Tom Tromey wrote: > Keith> Unfortunately, my patch to find_size_for_pointer_math interferes with your recently committed > Keith> patch ["Fix bug in dynamic type resolution"], which you "snuck" [:-)] in before I could finish > Keith> rebasing and retesting my patch. > > Tom> Sorry about that. Don't be. I apologize that it has taken me several weeks to get back to this! > I debugged this today and came up with the appended. I appreciate this, but I do have some simple questions. I am *quite* rusty when it comes to working on gdb nowadays... > The ada-lang.c hunk is the important bit. I don't see a reason to call > value_addr here, and this call results in a value that has no address. > > It seems wrong to have a not_lval object with a 0 address that can then > be treated as a pointer. Though in this case, the value has > contents... so I wonder if that's something your patch needs to account > for. I can certainly call value_contents here instead. While gdb.ada/array_of_variant.exp still fails, it does so without erroring out: Expected: print another_array(1) $6 = (initial => 0, rest => (tag => unused, cval => 88 'X')) With my (+ value_contents) patch: $6 = (initial => 0, rest => (tag => unused, ival => 88)) [This is where my previous patch used to error.] While I think this is an improvement, there is obviously something amiss. Your patch to ada-lang.c (ada_funcall_operation::evaluate) fixes this failure. While I may agree that value_addr seems out-of-place here, I will be the first to admit that the value system is probably the one area of gdb that I understand the least. [Ada excepted, of course. I definitely understand that the least!] > The other hunk just changes the failure from 0x4 to 0x0, which seems > more correct in this case. Though I wonder if this should really go in, > since it seems like it could do the wrong thing on platforms where 0 is > a valid address. Yeah, this whole 0 address dichotomy is always concerning. Nonetheless, please allow me to ask, when you say "I wonder if this should really go in," do you mean this particular hunk or both these hunks? [AFAICT, this last hunk is unnecessary from my proposed patch's perspective.] Keith For the record, the proposed patch, including "necessary" bits of yours, is: diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c index e76a3000f61..4bbeee240f2 100644 --- a/gdb/ada-lang.c +++ b/gdb/ada-lang.c @@ -10692,9 +10692,6 @@ ada_funcall_operation::evaluate (struct type *expect_type, well. */ callee = ada_to_fixed_value (coerce_ref (callee)); } - else if (value_type (callee)->code () == TYPE_CODE_ARRAY - && VALUE_LVAL (callee) == lval_memory) - callee = value_addr (callee); struct type *type = ada_check_typedef (value_type (callee)); diff --git a/gdb/valarith.c b/gdb/valarith.c index 140ef448137..fb6b3c0819a 100644 --- a/gdb/valarith.c +++ b/gdb/valarith.c @@ -36,21 +36,32 @@ #define TRUNCATION_TOWARDS_ZERO ((-5 / 2) == -2) #endif -/* Given a pointer, return the size of its target. +/* Given a pointer variable, return the size of its target. If the pointer type is void *, then return 1. If the target type is incomplete, then error out. + PTR_TYPE_OUT will contain the the actual pointer target type, + which may be a resolved dynamic type. This isn't a general purpose function, but just a helper for value_ptradd. */ static LONGEST -find_size_for_pointer_math (struct type *ptr_type) +find_size_for_pointer_math (struct value *ptr_var, struct type **ptr_type_out) { LONGEST sz = -1; struct type *ptr_target; + struct type *ptr_type = check_typedef (value_type (ptr_var)); gdb_assert (ptr_type->code () == TYPE_CODE_PTR); ptr_target = check_typedef (TYPE_TARGET_TYPE (ptr_type)); + if (is_dynamic_type (ptr_target)) + { + gdb::array_view view = value_contents (ptr_var); + ptr_target + = resolve_dynamic_type (ptr_target, view, value_address (ptr_var)); + ptr_type = make_pointer_type (ptr_target, nullptr); + } + sz = type_length_units (ptr_target); if (sz == 0) { @@ -69,6 +80,7 @@ find_size_for_pointer_math (struct type *ptr_type) "try casting to a known type, or void *."), name); } } + *ptr_type_out = ptr_type; return sz; } @@ -83,8 +95,7 @@ value_ptradd (struct value *arg1, LONGEST arg2) struct value *result; arg1 = coerce_array (arg1); - valptrtype = check_typedef (value_type (arg1)); - sz = find_size_for_pointer_math (valptrtype); + sz = find_size_for_pointer_math (arg1, &valptrtype); result = value_from_pointer (valptrtype, value_as_address (arg1) + sz * arg2);