public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: "Kempke, Nils-Christian" <nils-christian.kempke@intel.com>
To: Simon Marchi <simark@simark.ca>,
	"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Cc: "tom@tromey.com" <tom@tromey.com>,
	Bernhard Heckel <bernhard.heckel@intel.com>
Subject: RE: [PATCH 2/4] gdb/types: Resolve pointer types dynamically
Date: Thu, 29 Sep 2022 12:39:29 +0000	[thread overview]
Message-ID: <CY4PR1101MB2071BF3BDC6724C300F0E2FBB8579@CY4PR1101MB2071.namprd11.prod.outlook.com> (raw)
In-Reply-To: <33ee2a4d-0d7b-4183-c94c-2d9107e1b6ca@simark.ca>

Hi Simon,

Thanks for the review!

> 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.

Yes, you are right.  All the others should not match here.

The reason this shortcut was added is that ifort/icc emit the DW_AT_associated
for Fortran pointers.  I thought this was correct (until the discussion on PATCH 3),
but looking at the DWARF spec it seems to be unexpected.

Further down in this Patch I actually added resolution of the DW_AT_associated
for dynamic pointers - which should not even be there.  It is not listed under
the attributes applicable for pointers.

Here, icc/ifort pointers would run into an infinite loop ever resolving their
pointer types if we have a cyclic pointer dependency (as added in one of the
tests).

I am not sure how to fix this while making clear that it is an exception for
icc/ifort but it should be made clear as this is not at all obvious.  As we have a
similar discussion in PATCH 3 (I agree that the handling of ifort/icc's
pointer/reference DWARF should be behind some compiler check),
I would remove this shortcut from this patch and move its treatment over to
PATCH 3.  Similarly I am now inclined to remove the resolution of
DW_AT_associated from resolve_dynamic_pointer as it is not an expected attribute
for pointers to have.
 
> >
> >    /* 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

Fixed in V2.
 
> > +    pinfo.addr = extract_typed_address (addr_stack->valaddr.data (),
> > +					type);
> 
> "type" would fit on the same line
>

Fixed in V2.
 
> > +  else
> > +    pinfo.addr = read_memory_typed_address (addr_stack->addr, type);
> > +  pinfo.next = addr_stack;
> > +
> > +  struct type* resolved_type = copy_type (type);
> 
> Space before *
> 

Fixed in V2

> > +
> > +  /* 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);

This is the part I was referencing above..

> > +
> > +  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.
>

Fixed in V2.

> > 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.
> 

Fixed in V2.

I'll update this locally for now - and send a V2 soon with all above mentioned
changes incorporated.

Thanks again!
Nils
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

  reply	other threads:[~2022-09-29 12:39 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-20  7:26 [PATCH 0/4] Dynamic properties of pointers Nils-Christian Kempke
2022-09-20  7:26 ` [PATCH 1/4] gdb, testsuite: handle icc and icpc deprecated remarks Nils-Christian Kempke
2022-09-26 14:32   ` Simon Marchi
2022-09-20  7:26 ` [PATCH 2/4] gdb/types: Resolve pointer types dynamically Nils-Christian Kempke
2022-09-26 15:33   ` Simon Marchi
2022-09-29 12:39     ` Kempke, Nils-Christian [this message]
2022-09-20  7:26 ` [PATCH 3/4] gdb, typeprint: fix pointer/reference typeprint for icc/ifort Nils-Christian Kempke
2022-09-26 16:02   ` Simon Marchi
2022-09-26 17:18     ` Kempke, Nils-Christian
2022-09-27  9:14       ` Zaric, Zoran (Zare)
2022-09-27 12:48         ` Simon Marchi
2022-09-20  7:26 ` [PATCH 4/4] gdb/fortran: Fix sizeof intrinsic for Fortran Nils-Christian Kempke
2022-09-26 17:06   ` Simon Marchi
2022-09-26 17:22     ` Kempke, Nils-Christian
2022-09-26 17:24     ` Kempke, Nils-Christian

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CY4PR1101MB2071BF3BDC6724C300F0E2FBB8579@CY4PR1101MB2071.namprd11.prod.outlook.com \
    --to=nils-christian.kempke@intel.com \
    --cc=bernhard.heckel@intel.com \
    --cc=gdb-patches@sourceware.org \
    --cc=simark@simark.ca \
    --cc=tom@tromey.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).