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 8D41D3858D37 for ; Mon, 26 Sep 2022 15:33:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8D41D3858D37 Received: from [10.0.0.11] (unknown [217.28.27.60]) (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 00A2A1E0D5; Mon, 26 Sep 2022 11:33:13 -0400 (EDT) Message-ID: <33ee2a4d-0d7b-4183-c94c-2d9107e1b6ca@simark.ca> Date: Mon, 26 Sep 2022 11:33:13 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [PATCH 2/4] gdb/types: Resolve pointer types dynamically Content-Language: en-US To: Nils-Christian Kempke , gdb-patches@sourceware.org Cc: tom@tromey.com, Bernhard Heckel References: <20220920072629.2736207-1-nils-christian.kempke@intel.com> <20220920072629.2736207-3-nils-christian.kempke@intel.com> From: Simon Marchi In-Reply-To: <20220920072629.2736207-3-nils-christian.kempke@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.5 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 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: Mon, 26 Sep 2022 15:33:16 -0000 On 2022-09-20 03:26, Nils-Christian Kempke via Gdb-patches wrote: > From: Bernhard Heckel > > This commit allows pointers to be dynamic types (on the outmost > level). Similar to references, a pointer is considered a dynamic type > if its target type is a dynamic type and it is on the outmost level. > > When resolving a dynamic pointer in resolve_dynamic_pointer > we a) try and resolve its target type (in a similar fashion as it is > done for references) and b) resolve the dynamic DW_AT_associated > property, which is emitted by ifx and ifort for pointers. > > This generally makes the GDB output more verbose. We are able to print > more details about a pointer's target like the dimension of an array. > > In Fortran, if we have a pointer to a dynamic type > > type buffer > real, dimension(:), pointer :: ptr > end type buffer > type(buffer), pointer :: buffer_ptr > allocate (buffer_ptr) > allocate (buffer_ptr%ptr (5)) > > which then gets allocated, we now resolve the dynamic type before > printing the pointer's type: > > Before: > > (gdb) ptype buffer_ptr > type = PTR TO -> ( Type buffer > real(kind=4) :: alpha(:) > End Type buffer ) > > After: > > (gdb) ptype buffer_ptr > type = PTR TO -> ( Type buffer > real(kind=4) :: alpha(5) > End Type buffer ) > > Similarly in C++ we can dynamically resolve e.g. pointers to arrays: > > int len = 3; > int arr[len]; > int (*ptr)[len]; > int ptr = &arr; > > Once the pointer is assigned one gets: > > Before: > > (gdb) p ptr > $1 = (int (*)[variable length]) 0x123456 > (gdb) ptype ptr > type = int (*)[variable length] > > After: > > (gdb) p ptr > $1 = (int (*)[3]) 0x123456 > (gdb) ptype ptr > type = int (*)[3] > > For more examples see the modified/added test cases. Hi, That all looks nice to me, that seems like some concrete improvements for the user. I noted some nits below. I am not really familiar with Fortran or the dynamic types thing, so I'd really like if someone more familiar with those could take a look. > --- > gdb/gdbtypes.c | 53 +++++- > gdb/testsuite/gdb.cp/vla-cxx.cc | 4 + > gdb/testsuite/gdb.cp/vla-cxx.exp | 33 ++++ > gdb/testsuite/gdb.dwarf2/dynarr-ptr.exp | 16 +- > .../gdb.fortran/pointer-to-pointer.exp | 2 +- > gdb/testsuite/gdb.fortran/pointers.exp | 178 ++++++++++++++++++ > gdb/testsuite/gdb.fortran/pointers.f90 | 29 +++ > gdb/valprint.c | 6 - > 8 files changed, 303 insertions(+), 18 deletions(-) > create mode 100644 gdb/testsuite/gdb.fortran/pointers.exp > > diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c > index c458b204157..a4d79a64e95 100644 > --- a/gdb/gdbtypes.c > +++ b/gdb/gdbtypes.c > @@ -2083,9 +2083,15 @@ is_dynamic_type_internal (struct type *type, int top_level) > { > type = check_typedef (type); > > - /* We only want to recognize references at the outermost level. */ > - if (top_level && type->code () == TYPE_CODE_REF) > - type = check_typedef (TYPE_TARGET_TYPE (type)); > + /* We only want to recognize references and pointers at the outermost > + level. */ > + if (type->code () == TYPE_CODE_REF || type->code () == TYPE_CODE_PTR) > + { > + if (top_level != 0) > + type = check_typedef (TYPE_TARGET_TYPE (type)); > + else > + return 0; > + } Can you explain this change here, specifically the addition of the "return 0"? My understanding is that for REFs and PTRs, nothing below will match and we will end up returning 0 anyway, so this is just a new shortcut for when the type is a REF or PTR and top_level is false. But I'd like to confirm. > > /* Types that have a dynamic TYPE_DATA_LOCATION are considered > dynamic, even if the type itself is statically defined. > @@ -2787,6 +2793,43 @@ resolve_dynamic_struct (struct type *type, > return resolved_type; > } > > +/* Worker for pointer types. */ > + > +static struct type * > +resolve_dynamic_pointer (struct type *type, > + struct property_addr_info *addr_stack) > +{ > + struct dynamic_prop *prop; > + CORE_ADDR value; > + > + /* Resolve the target type of this type. */ > + struct property_addr_info pinfo; > + pinfo.type = check_typedef (TYPE_TARGET_TYPE (type)); > + pinfo.valaddr = {}; > + if (addr_stack->valaddr.data () != NULL) NULL -> nullptr > + pinfo.addr = extract_typed_address (addr_stack->valaddr.data (), > + type); "type" would fit on the same line > + else > + pinfo.addr = read_memory_typed_address (addr_stack->addr, type); > + pinfo.next = addr_stack; > + > + struct type* resolved_type = copy_type (type); Space before * > + > + /* Resolve associated property. */ > + prop = TYPE_ASSOCIATED_PROP (resolved_type); > + if (prop != nullptr > + && dwarf2_evaluate_property (prop, nullptr, addr_stack, &value)) > + prop->set_const_val (value); > + > + if (pinfo.addr != 0x0 && !type_not_associated (resolved_type)) > + TYPE_TARGET_TYPE (resolved_type) > + = resolve_dynamic_type_internal (TYPE_TARGET_TYPE (type), > + &pinfo, 0); > + > + Remove one newline here. > diff --git a/gdb/testsuite/gdb.cp/vla-cxx.exp b/gdb/testsuite/gdb.cp/vla-cxx.exp > index 3494b5e8b77..e2bb8989212 100644 > --- a/gdb/testsuite/gdb.cp/vla-cxx.exp > +++ b/gdb/testsuite/gdb.cp/vla-cxx.exp > @@ -23,6 +23,36 @@ if ![runto_main] { > return -1 > } > > +gdb_breakpoint [gdb_get_line_number "Before pointer assignment"] > +gdb_continue_to_breakpoint "Before pointer assignment" > + > +set test_name "ptype ptr, Before pointer assignment" > +gdb_test_multiple "ptype ptr" $test_name { > + # gcc/icx > + -re -wrap "= int \\(\\*\\)\\\[variable length\\\]" { > + pass $test_name > + } > + # icc > + -re -wrap "= int \\(\\*\\)\\\[3\\\]" { > + pass $test_name > + } No need to use the test_name variable nowadays, you can use the magic $gdb_test_name inside the gdb_test_multiple body to access the name that was passed to it. Simon