[RFA] dwarf2read.c: set TYPE_DOMAIN_TYPE correctly for methods

[RFA] dwarf2read.c: set TYPE_DOMAIN_TYPE correctly for methods

[David Carlton]

I've figured out what caused the regression that I turned up in PR
gdb/653; here's a patch that fixes it.

David Carlton
carlton@math.stanford.edu

2002-08-22  David Carlton  <carlton@math.stanford.edu>

	* dwarf2read.c (dwarf2_add_member_fn): Add back in the type
	argument that was deleted on 2002-06-14: it was needed after all,
	as PR gdb/653 demonstrates.  Update call to smash_to_method_type.
	(read_structure_scope): Update call to dwarf2_add_member_fn.

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.65
diff -u -p -r1.65 dwarf2read.c
--- dwarf2read.c	20 Aug 2002 18:45:30 -0000	1.65
+++ dwarf2read.c	22 Aug 2002 20:13:53 -0000
@@ -803,7 +803,8 @@ static void dwarf2_attach_fields_to_type
 					  struct type *, struct objfile *);
 
 static void dwarf2_add_member_fn (struct field_info *,
-				  struct die_info *, struct objfile *objfile,
+				  struct die_info *, struct type *,
+				  struct objfile *objfile,
 				  const struct comp_unit_head *);
 
 static void dwarf2_attach_fn_fields_to_type (struct field_info *,
@@ -2259,7 +2260,7 @@ dwarf2_attach_fields_to_type (struct fie
 
 static void
 dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
-		      struct objfile *objfile,
+		      struct type *type, struct objfile *objfile,
 		      const struct comp_unit_head *cu_header)
 {
   struct attribute *attr;
@@ -2327,7 +2328,7 @@ dwarf2_add_member_fn (struct field_info 
       struct type *return_type = TYPE_TARGET_TYPE (die->type);
       int nparams = TYPE_NFIELDS (die->type);
 
-      smash_to_method_type (fnp->type, die->type,
+      smash_to_method_type (fnp->type, type,
 			    TYPE_TARGET_TYPE (die->type),
 			    TYPE_FIELDS (die->type),
 			    TYPE_NFIELDS (die->type),
@@ -2516,7 +2517,7 @@ read_structure_scope (struct die_info *d
 	    {
 	      /* C++ member function. */
 	      process_die (child_die, objfile, cu_header);
-	      dwarf2_add_member_fn (&fi, child_die, objfile, cu_header);
+	      dwarf2_add_member_fn (&fi, child_die, type, objfile, cu_header);
 	    }
 	  else if (child_die->tag == DW_TAG_inheritance)
 	    {

Re: [RFA] dwarf2read.c: set TYPE_DOMAIN_TYPE correctly for methods

[Daniel Jacobowitz]

On Thu, Aug 22, 2002 at 01:35:50PM -0700, David Carlton wrote:
> I've figured out what caused the regression that I turned up in PR
> gdb/653; here's a patch that fixes it.
> 
> David Carlton
> carlton@math.stanford.edu
> 
> 2002-08-22  David Carlton  <carlton@math.stanford.edu>
> 
> 	* dwarf2read.c (dwarf2_add_member_fn): Add back in the type
> 	argument that was deleted on 2002-06-14: it was needed after all,
> 	as PR gdb/653 demonstrates.  Update call to smash_to_method_type.
> 	(read_structure_scope): Update call to dwarf2_add_member_fn.

Can you explain why this is necessary?  I could not find any path to
that point where type != die->type.

> 
> Index: dwarf2read.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/dwarf2read.c,v
> retrieving revision 1.65
> diff -u -p -r1.65 dwarf2read.c
> --- dwarf2read.c	20 Aug 2002 18:45:30 -0000	1.65
> +++ dwarf2read.c	22 Aug 2002 20:13:53 -0000
> @@ -803,7 +803,8 @@ static void dwarf2_attach_fields_to_type
>  					  struct type *, struct objfile *);
>  
>  static void dwarf2_add_member_fn (struct field_info *,
> -				  struct die_info *, struct objfile *objfile,
> +				  struct die_info *, struct type *,
> +				  struct objfile *objfile,
>  				  const struct comp_unit_head *);
>  
>  static void dwarf2_attach_fn_fields_to_type (struct field_info *,
> @@ -2259,7 +2260,7 @@ dwarf2_attach_fields_to_type (struct fie
>  
>  static void
>  dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
> -		      struct objfile *objfile,
> +		      struct type *type, struct objfile *objfile,
>  		      const struct comp_unit_head *cu_header)
>  {
>    struct attribute *attr;
> @@ -2327,7 +2328,7 @@ dwarf2_add_member_fn (struct field_info 
>        struct type *return_type = TYPE_TARGET_TYPE (die->type);
>        int nparams = TYPE_NFIELDS (die->type);
>  
> -      smash_to_method_type (fnp->type, die->type,
> +      smash_to_method_type (fnp->type, type,
>  			    TYPE_TARGET_TYPE (die->type),
>  			    TYPE_FIELDS (die->type),
>  			    TYPE_NFIELDS (die->type),
> @@ -2516,7 +2517,7 @@ read_structure_scope (struct die_info *d
>  	    {
>  	      /* C++ member function. */
>  	      process_die (child_die, objfile, cu_header);
> -	      dwarf2_add_member_fn (&fi, child_die, objfile, cu_header);
> +	      dwarf2_add_member_fn (&fi, child_die, type, objfile, cu_header);
>  	    }
>  	  else if (child_die->tag == DW_TAG_inheritance)
>  	    {
> 

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] dwarf2read.c: set TYPE_DOMAIN_TYPE correctly for methods

[David Carlton]

In article <20020822204211.GA31727@nevyn.them.org>, Daniel Jacobowitz <drow@mvista.com> writes:

>> * dwarf2read.c (dwarf2_add_member_fn): Add back in the type
>> argument that was deleted on 2002-06-14: it was needed after all,
>> as PR gdb/653 demonstrates.  Update call to smash_to_method_type.
>> (read_structure_scope): Update call to dwarf2_add_member_fn.

> Can you explain why this is necessary?  I could not find any path to
> that point where type != die->type.

dwarf2_add_member_fn is called by read_structure_scope.  The 'die'
argument to dwarf2_add_member_fn is the 'child_die' variable of
read_structure_scope.  So what dwarf2_add_member_fn thinks of as
'die->type' is what read_structure_scope thinks of as
'child_die->type', in other words the type associated to the method
that is being added to the structure in question.

But the second argument to smash_to_method_type is supposed to be the
domain of the given method, i.e. the type of the structure owning the
method.  This is dramatically different from read_structure_scope's
child_die->type; it equals read_structure_scope's die->type.

So that's why we want to pass in read_structure_scope's die->type as
an argument.  (I don't see any way to recover die->type from
child_die, though admittedly I didn't try very hard.)

For a reasonably dramatic illustration of the effects of this, try
compiling the following program

/* Create some objects, and try to print out their methods.  */

class A {
public:
  virtual void virt() {};
  void nonvirt() {};
};

int main()
{
  A *theA = new A;

  return 0;
}

and debug it with GDB from the current CVS.  Run it until theA has
been created, and then try 'p theA->virt'.

David Carlton
carlton@math.stanford.edu

Re: [RFA] dwarf2read.c: set TYPE_DOMAIN_TYPE correctly for methods

[Andrew Cagney]

> On Thu, Aug 22, 2002 at 01:35:50PM -0700, David Carlton wrote:
> 
>> I've figured out what caused the regression that I turned up in PR
>> gdb/653; here's a patch that fixes it.
>> 
>> David Carlton
>> carlton@math.stanford.edu
>> 
>> 2002-08-22  David Carlton  <carlton@math.stanford.edu>
>> 
>> 	* dwarf2read.c (dwarf2_add_member_fn): Add back in the type
>> 	argument that was deleted on 2002-06-14: it was needed after all,
>> 	as PR gdb/653 demonstrates.  Update call to smash_to_method_type.
>> 	(read_structure_scope): Update call to dwarf2_add_member_fn.
> 
> 
> Can you explain why this is necessary?  I could not find any path to
> that point where type != die->type.

Just a general reminder for people comming up with patches.

The place to put comments explaining changes is in the source code. 
Something like:

       /* NOTE: cagney/2002-08-12: Replaced a call to
          regcache_raw_read_as_address() with a call to
          regcache_cooked_read_unsigned().  The old, ...as_address
          function was eventually calling extract_unsigned_integer (via
          extract_address) to unpack the registers value.  The below is
          doing an unsigned extract so that it is functionally
          equivalent.  The read needs to be cooked as, otherwise, it
          will never correctly return the value of a register in the
          [NUM_REGS .. NUM_REGS+NUM_PSEUDO_REGS) range.  */

(although, yes, the example is a bit overboard).  This might feel 
un-natural but it is correct and very very important.  The ChangeLog 
need only state what changed, not why.

Someone studying the code and trying to figure out why things currently 
don't work should be able to do so by just examining the current code 
and its commentary.  Hopefully that commentry will explain what was 
tried in the past and why it failed or why it needed to be changed.

Oh, and adding more comments to the code is always ``obvious'' :-) (Is 
there a way to add ``comments'' to the doco?).

enjoy,
Andrew

Re: [RFA] dwarf2read.c: set TYPE_DOMAIN_TYPE correctly for methods

[David Carlton]

In article <3D655311.6030603@ges.redhat.com>, Andrew Cagney <ac131313@ges.redhat.com> writes:

> The place to put comments explaining changes is in the source
> code.
...
> The ChangeLog need only state what changed, not why.

Whoops, sorry about that.  How about this patch?

By the way, am I correct in thinking that the ChangeLog is supposed
to mention the PR number?  I noticed that GNATS picked up some of my
earlier changes, and I'm assuming it noticed the PR number from the
log message.

David Carlton
carlton@math.stanford.edu

2002-08-22  David Carlton  <carlton@math.stanford.edu>

	* dwarf2read.c (dwarf2_add_member_fn): Add the 'type'
	argument (PR gdb/653).  Update call to smash_to_method_type.
	(read_structure_scope): Update call to dwarf2_add_member_fn.

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.65
diff -u -p -r1.65 dwarf2read.c
--- dwarf2read.c	20 Aug 2002 18:45:30 -0000	1.65
+++ dwarf2read.c	22 Aug 2002 21:22:51 -0000
@@ -803,7 +803,8 @@ static void dwarf2_attach_fields_to_type
 					  struct type *, struct objfile *);
 
 static void dwarf2_add_member_fn (struct field_info *,
-				  struct die_info *, struct objfile *objfile,
+				  struct die_info *, struct type *,
+				  struct objfile *objfile,
 				  const struct comp_unit_head *);
 
 static void dwarf2_attach_fn_fields_to_type (struct field_info *,
@@ -2259,7 +2260,7 @@ dwarf2_attach_fields_to_type (struct fie
 
 static void
 dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
-		      struct objfile *objfile,
+		      struct type *type, struct objfile *objfile,
 		      const struct comp_unit_head *cu_header)
 {
   struct attribute *attr;
@@ -2327,7 +2328,15 @@ dwarf2_add_member_fn (struct field_info 
       struct type *return_type = TYPE_TARGET_TYPE (die->type);
       int nparams = TYPE_NFIELDS (die->type);
 
-      smash_to_method_type (fnp->type, die->type,
+      /* NOTE: carlton/2002-08-22: Previously, the second argument to
+	 smash_to_method_type was die->type rather than type, and the
+	 type argument to dwarf2_add_member_fn didn't exst.  This is
+	 incorrect: the second argument to smash_to_method_type should
+	 be the type of the class that this is a method of, whereas
+	 die->type is the type of the method itself.  So we need to
+	 pass that type in from read_structure_scope explicitly.  See
+	 PR gdb/653.  */
+      smash_to_method_type (fnp->type, type,
 			    TYPE_TARGET_TYPE (die->type),
 			    TYPE_FIELDS (die->type),
 			    TYPE_NFIELDS (die->type),
@@ -2516,7 +2525,7 @@ read_structure_scope (struct die_info *d
 	    {
 	      /* C++ member function. */
 	      process_die (child_die, objfile, cu_header);
-	      dwarf2_add_member_fn (&fi, child_die, objfile, cu_header);
+	      dwarf2_add_member_fn (&fi, child_die, type, objfile, cu_header);
 	    }
 	  else if (child_die->tag == DW_TAG_inheritance)
 	    {

Re: [RFA] dwarf2read.c: set TYPE_DOMAIN_TYPE correctly for methods

[Andrew Cagney]

> The ChangeLog need only state what changed, not why.
> 
> 
> Whoops, sorry about that.  How about this patch?

Like I said, it feels un-natural so it definitly takes getting 
accustomed to :-)  Anyway, the comment, yes like it.  In case you're 
wondering, the username/date convention came about because people 
realised they couldn't identify when they were reading 15(!?)  year old 
comments that described problems on systems that no longer exist!

As for the change proper, something for a debug info maintainer.

> By the way, am I correct in thinking that the ChangeLog is supposed
> to mention the PR number?  I noticed that GNATS picked up some of my
> earlier changes, and I'm assuming it noticed the PR number from the
> log message.

It's looking in the CVS commit message (which is typically a duplicate 
of the changelog anyway).  Hmm, this means that you've figured out that 
the best thing to stick in the CVS commit message is the changelog! :-)

enjoy,
Andrew