[Linux-ia64] Re: gdb null ptr

[Linux-ia64] Re: gdb null ptr

[Jim Wilson]

I don't see how this can happen from looking at the source.  We only cache
a type in dwarf2_cached_types if it has a DW_AT_name attribute, and if it
does have such an attribute, then the type should have a name or tag name.

Can you provide me with a testcase?  A binary that I can run under gdb should
be sufficient.  You could put it in ftp.cygnus.com/incoming and then mail
the file name to me.

Since this doesn't appear to be an ia64 specific problem, if I don't have a
testcase then I will just punt to the gdb developers
	 http://sources.redhat.com/gdb/#bugs

Jim

Re: [Linux-ia64] Re: gdb null ptr

[Pete Wyckoff]

wilson@cygnus.com said:
> I don't see how this can happen from looking at the source.  We only cache
> a type in dwarf2_cached_types if it has a DW_AT_name attribute, and if it
> does have such an attribute, then the type should have a name or tag name.
> 
> Can you provide me with a testcase?  A binary that I can run under gdb should
> be sufficient.  You could put it in ftp.cygnus.com/incoming and then mail
> the file name to me.
> 
> Since this doesn't appear to be an ia64 specific problem, if I don't have a
> testcase then I will just punt to the gdb developers
> 	 http://sources.redhat.com/gdb/#bugs

It may very well be a gdb bug.  Anyway, I put a huge executable in
the incoming dir as pw-gdb-bug-ex.

    3078a3f848cba6d1f76fbfe3707b0464  pw-gdb-bug-ex

Here's what happens for the non-patched gdb:

ia1$ /home/pw/cygnus/build/gdb/gdb pw-gdb-bug-ex 
GNU gdb 5.0-ia64-000717
Copyright 2000 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.  This version of GDB is supported
for customers of Cygnus Solutions.  Type "show warranty" for details.
This GDB was configured as "ia64-unknown-linux"...
(gdb) b output
Segmentation fault

And with the patch:

ia1$ /usr/local/cygnus/bin/gdb pw-gdb-bug-ex
GNU gdb 5.0-ia64-000717
Copyright 2000 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.  This version of GDB is supported
for customers of Cygnus Solutions.  Type "show warranty" for details.
This GDB was configured as "ia64-unknown-linux"...
(gdb) b output
Breakpoint 1 at 0x400000000002a190: file put.F, line 32.
(gdb) 

Running gdb on itself:

ia1$ /usr/local/cygnus/bin/gdb /home/pw/cygnus/build/gdb/gdb
GNU gdb 5.0-ia64-000717
Copyright 2000 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.  This version of GDB is supported
for customers of Cygnus Solutions.  Type "show warranty" for details.
This GDB was configured as "ia64-unknown-linux"...
(gdb) run pw-gdb-bug-ex
GNU gdb 5.0-ia64-000717
Copyright 2000 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.  This version of GDB is supported
for customers of Cygnus Solutions.  Type "show warranty" for details.
This GDB was configured as "ia64-unknown-linux"...
(gdb) b output

Program received signal SIGSEGV, Segmentation fault.
0x20000000001a2661 in strcmp () from /lib/libc.so.0
(gdb) where
#0  0x20000000001a2661 in strcmp () from /lib/libc.so.0
#1  0x40000000001e7c20 in tag_type_to_type (die=0x6000000000120460, 
    objfile=0x60000000000bdf10) at ../../src/gdb/dwarf2read.c:4526
#2  0x40000000001e7910 in die_type (die=0x6000000000045130, 
    objfile=0x60000000000bdf10) at ../../src/gdb/dwarf2read.c:4440
#3  0x40000000001e2290 in read_subroutine_type (die=0x1, 
    objfile=0x60000000000bdf10) at ../../src/gdb/dwarf2read.c:2788
#4  0x40000000001dcda0 in process_die (die=0x6000000000120070, 
    objfile=0x60000000000bdf10) at ../../src/gdb/dwarf2read.c:1393
#5  0x40000000001dd590 in read_file_scope (die=0x60000000000f7630, 
    objfile=0x60000000000bdf10) at ../../src/gdb/dwarf2read.c:1556
#6  0x40000000001dcd70 in process_die (die=0x60000000000f7630, 
    objfile=0x60000000000bdf10) at ../../src/gdb/dwarf2read.c:1390
#7  0x40000000001dc8e0 in psymtab_to_symtab_1 (pst=0x600000000011b9f8)
    at ../../src/gdb/dwarf2read.c:1335
#8  0x40000000001dc540 in dwarf2_psymtab_to_symtab (pst=0x600000000011b9f8)
    at ../../src/gdb/dwarf2read.c:1270
#9  0x400000000009c8c0 in psymtab_to_symtab (pst=0x600000000011b9f8)
    at ../../src/gdb/symfile.c:393
#10 0x400000000008e120 in lookup_symbol (name=0x9fffffffffffe420 "output", 
    block=0x0, namespace=VAR_NAMESPACE, is_a_field_of_this=0x0, 
    symtab=0x9fffffffffffe458) at ../../src/gdb/symtab.c:782
#11 0x4000000000095410 in decode_line_1 (argptr=0x9fffffffffffe728, 
    funfirstline=1, default_symtab=0x0, default_line=0, 
    canonical=0x9fffffffffffe720) at ../../src/gdb/symtab.c:3201
#12 0x400000000016d630 in parse_breakpoint_sals (address=0x9fffffffffffe728, 
    sals=0x9fffffffffffe730, addr_string=0x9fffffffffffe720)
    at ../../src/gdb/breakpoint.c:4680
#13 0x400000000016d980 in break_command_1 (arg=0x6000000000079a38 "", flag=0, 
    from_tty=0) at ../../src/gdb/breakpoint.c:4763
#14 0x400000000016f7b0 in break_command (arg=0x6000000000079a32 "output", 
    from_tty=0) at ../../src/gdb/breakpoint.c:5217
#15 0x4000000000144df0 in execute_command (p=0x6000000000079a37 "t", 
    from_tty=1) at ../../src/gdb/top.c:1552
#16 0x40000000000cd7b0 in command_handler (
    command=0x6000000000079a30 "b output") at ../../src/gdb/event-top.c:515
#17 0x40000000000ceac0 in command_line_handler (
    rl=0x60000000000d0b50 "b output") at ../../src/gdb/event-top.c:811
#18 0x4000000000309020 in rl_callback_read_char ()
    at ../../src/readline/callback.c:114
#19 0x40000000000cae60 in rl_callback_read_char_wrapper (
    client_data=0x9fffffffffffea90) at ../../src/gdb/event-top.c:166
#20 0x40000000000cd3c0 in stdin_event_handler (error=0, client_data=0x0)
    at ../../src/gdb/event-top.c:422
#21 0x400000000017e980 in handle_file_event (event_file_descF87702)
    at ../../src/gdb/event-loop.c:742
#22 0x400000000017d8a0 in process_event () at ../../src/gdb/event-loop.c:377
#23 0x400000000017d9b0 in gdb_do_one_event (data=0x6000000000045130)
    at ../../src/gdb/event-loop.c:414
#24 0x40000000001424f0 in catch_errors (
    func=0x40000000003a8e00 <ia64_dis_names+101264>, args=0x0, 
    errstring=0x400000000035def8 "", mask=6) at ../../src/gdb/top.c:616
#25 0x400000000017da50 in start_event_loop () at ../../src/gdb/event-loop.c:438
#26 0x40000000000cb0c0 in cli_command_loop () at ../../src/gdb/event-top.c:196
#27 0x400000000003eb50 in captured_command_loop (data=0x6000000000045130)
    at ../../src/gdb/main.c:104
#28 0x40000000001424f0 in catch_errors (
    func=0x40000000003a8b60 <ia64_dis_names+100592>, args=0x0, 
    errstring=0x4000000000337dd8 "", mask=6) at ../../src/gdb/top.c:616
#29 0x4000000000040720 in captured_main (data=0x9ffffffffffff800)
    at ../../src/gdb/main.c:749
#30 0x40000000001424f0 in catch_errors (
    func=0x40000000003a8990 <ia64_dis_names+100128>, args=0x9ffffffffffff800, 
    errstring=0x4000000000337dd8 "", mask=6) at ../../src/gdb/top.c:616
#31 0x4000000000040790 in main (argc=2, argv=0x9ffffffffffff838)
    at ../../src/gdb/main.c:761
(gdb) 

We can shift this to the gdb developers if you're satisfied it's not
ia64 specific.

		-- Pete

Re: [Linux-ia64] Re: gdb null ptr

[Kevin Buettner]

On Nov 3,  8:32am, Pete Wyckoff wrote:

> wilson@cygnus.com said:
> > I don't see how this can happen from looking at the source.  We only cache
> > a type in dwarf2_cached_types if it has a DW_AT_name attribute, and if it
> > does have such an attribute, then the type should have a name or tag name.
> > 
> > Can you provide me with a testcase?  A binary that I can run under gdb should
> > be sufficient.  You could put it in ftp.cygnus.com/incoming and then mail
> > the file name to me.
> > 
> > Since this doesn't appear to be an ia64 specific problem, if I don't have a
> > testcase then I will just punt to the gdb developers
> > 	 http://sources.redhat.com/gdb/#bugs
> 
> It may very well be a gdb bug.  Anyway, I put a huge executable in
> the incoming dir as pw-gdb-bug-ex.
> 
>     3078a3f848cba6d1f76fbfe3707b0464  pw-gdb-bug-ex

Peter,

Thanks for sending us this test case.

I think this problem is arising because the dwarf2 reader had
problems earlier on.  I.e, I see

During symbol reading, non-constant array bounds form 'DW_FORM_ref4' ignored.
During symbol reading, unsupported tag: 'DW_TAG_constant'.

My guess is that when these (at the moment) unsupported cases arise,
the reader fails to properly initialize a name or tag name.

I'll look into it some more and get back to you when I understand the
problem better.

Thanks again,

Kevin

Re: [Linux-ia64] Re: gdb null ptr

[Jim Wilson]

>...ia64-linux gdb core dumps on SGI F90 compiler output in tag_type_to_type
>in dwarf2read.c because a NULL nameoftype is passed to strcmp...

The SGI compiler emitted debug info for an array type with the name ".array.".
Gdb ignores DW_AT_name attributes attached to array types.  So we see the
name, try to use the name to index into our type cache, and then fail the
check to weed out hash collisions because we did not save the name in the
type info.

I see 4 ways to fix this:
1) Handle DW_AT_name when attached to an array type.
2) Only cache types that we expect to have names, presumably enums, structures,
   unions, and typedefs.
3) Check for a null string before doing the hash collision check.
4) Use something other than the type name to check for hash collisions.

1 doesn't solve all possible problems, since arrays may not be the only types
with unexpected names.  2 seems like a good choice to me.  3 is inefficient,
because we are storing a value in the cache that we will never be able to use.
I think 4 would require changing struct type to have a pointer back to the
original die, so that we can use a die check for the hash collision check.
This seems like the best choice to me, though it is more work.

Unfortunately, the problem gets a little more complicated.  I looked at the
debug info emitted by the SGI F90 compiler using readelf -w.  The type names
it is using aren't distinct, and hence it is unsafe to use type names for the
hash collision check.  I think there is some sort of C++ template like
expansion going on here (I don't know F90), and instead of using mangled names
for the expanded types it is using the base type name.  I see the same problem
with structure type names.  This may be a problem with the SGI compiler.
Or perhaps it is a problem with gdb not having F90 support yet.

In any case, this is something the gdb developers will have to look at.

Jim

Re: [Linux-ia64] Re: gdb null ptr

[Kevin Buettner]

On Nov 3,  1:42pm, Jim Wilson wrote:

> The SGI compiler emitted debug info for an array type with the name ".array.".
> Gdb ignores DW_AT_name attributes attached to array types.  So we see the
> name, try to use the name to index into our type cache, and then fail the
> check to weed out hash collisions because we did not save the name in the
> type info.
> 
> I see 4 ways to fix this:
> 1) Handle DW_AT_name when attached to an array type.
> 2) Only cache types that we expect to have names, presumably enums, structures,
>    unions, and typedefs.
> 3) Check for a null string before doing the hash collision check.
> 4) Use something other than the type name to check for hash collisions.
> 
> 1 doesn't solve all possible problems, since arrays may not be the only types
> with unexpected names.  2 seems like a good choice to me.  3 is inefficient,
> because we are storing a value in the cache that we will never be able to use.
> I think 4 would require changing struct type to have a pointer back to the
> original die, so that we can use a die check for the hash collision check.
> This seems like the best choice to me, though it is more work.
> 
> Unfortunately, the problem gets a little more complicated.  I looked at the
> debug info emitted by the SGI F90 compiler using readelf -w.  The type names
> it is using aren't distinct, and hence it is unsafe to use type names for the
> hash collision check.  I think there is some sort of C++ template like
> expansion going on here (I don't know F90), and instead of using mangled names
> for the expanded types it is using the base type name.  I see the same problem
> with structure type names.  This may be a problem with the SGI compiler.
> Or perhaps it is a problem with gdb not having F90 support yet.

Nice analysis.

The patch below effectively implements Jim's suggestion #2 above. 
This patch was made with respect to sourceware and might not apply
cleanly to sources from which the current linux/ia64 gdb is being
built.  (I can provide such patches if desired however.)

I've tested it against the program provided by Pete Wyckoff and
it does fix the segfault.

I will leave it to the dwarf2 maintainers to decide whether this
patch is acceptable or if it would be better to implement one of
Jim's other suggestions.

	* dwarf2read.c (tag_type_to_type): Don't cache the type
	unless it has a name or a tag name.

Index: dwarf2read.c
=================================RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.17
diff -u -p -r1.17 dwarf2read.c
--- dwarf2read.c	2000/11/03 22:38:38	1.17
+++ dwarf2read.c	2000/11/03 22:45:19
@@ -4538,7 +4538,9 @@ tag_type_to_type (struct die_info *die, 
 	  if (dwarf2_cached_types[hashval] != NULL)
 	    {
 	      const char *nameoftype;
-	      nameoftype = TYPE_NAME(dwarf2_cached_types[hashval]) = NULL ? TYPE_TAG_NAME(dwarf2_cached_types[hashval]) : TYPE_NAME(dwarf2_cached_types[hashval]);
+	      nameoftype = TYPE_NAME(dwarf2_cached_types[hashval]) = NULL 
+	                    ? TYPE_TAG_NAME(dwarf2_cached_types[hashval])
+			    : TYPE_NAME(dwarf2_cached_types[hashval]);
 	      if (strcmp(attrname, nameoftype) = 0)
 		{
 		  die->type=dwarf2_cached_types[hashval];
@@ -4546,13 +4548,17 @@ tag_type_to_type (struct die_info *die, 
 	      else
 		{
 		  read_type_die (die, objfile, cu_header);
-		  dwarf2_cached_types[hashval] = die->type;
+ 		  if (TYPE_NAME (die->type) != NULL 
+ 		      || TYPE_TAG_NAME (die->type) != NULL)
+ 		    dwarf2_cached_types[hashval] = die->type;
 		}
 	    }
 	  else
 	    {
 	      read_type_die (die, objfile, cu_header);
-	      dwarf2_cached_types[hashval] = die->type;
+ 	      if (TYPE_NAME (die->type) != NULL 
+ 		  || TYPE_TAG_NAME (die->type) != NULL)
+ 		dwarf2_cached_types[hashval] = die->type;
 	    }
 	}
       else

Re: [Linux-ia64] Re: gdb null ptr

[Daniel Berlin]

Kevin Buettner <kevinb@cygnus.com> writes:

> 
> Nice analysis.
> 
> The patch below effectively implements Jim's suggestion #2 above. 
> This patch was made with respect to sourceware and might not apply
> cleanly to sources from which the current linux/ia64 gdb is being
> built.  (I can provide such patches if desired however.)
> 
> I've tested it against the program provided by Pete Wyckoff and
> it does fix the segfault.
> 
> I will leave it to the dwarf2 maintainers to decide whether this
> patch is acceptable or if it would be better to implement one of
> Jim's other suggestions.
> 

Unfortuantely , this is actually still wrong for languages other than
C++, because we don't have the same guarantees about uniqueness in the name.

I was actually in the process of readying patches that add the same
type of name based caching (based on mangled name) to partial and
normal symbol reading, which gives us an amazing win for C++.

These patches also moved all of the name caching into "if (cu_language
= language_cplus)" blocks, doing what we used to do in the old case
(IE no caching).

Rather than let this stay broken until i finish cleaning up those
patches, here is a patch that moves the type caching so it only
happens for C++ CU's.

Unless other languages make the same guarantees, we can't do the same
optimization.

Diff is making it into more than it is, because moving it into the
cu_language = language_cplus block, changes the indentation of all
the code under it.

I have added the same type of code kevin has to the patches i am
readying.


--Dan


Index: dwarf2read.c
=================================RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.17
diff -c -3 -p -r1.17 dwarf2read.c
*** dwarf2read.c	2000/11/03 22:38:38	1.17
--- dwarf2read.c	2000/11/04 03:05:24
*************** tag_type_to_type (struct die_info *die, 
*** 4532,4547 ****
        attr = dwarf_attr (die, DW_AT_name);
        if (attr && DW_STRING (attr))
  	{
! 	  char *attrname=DW_STRING (attr);
! 	  unsigned long hashval=hash(attrname, strlen(attrname)) % TYPE_HASH_SIZE;
! 
! 	  if (dwarf2_cached_types[hashval] != NULL)
  	    {
! 	      const char *nameoftype;
! 	      nameoftype = TYPE_NAME(dwarf2_cached_types[hashval]) = NULL ? TYPE_TAG_NAME(dwarf2_cached_types[hashval]) : TYPE_NAME(dwarf2_cached_types[hashval]);
! 	      if (strcmp(attrname, nameoftype) = 0)
  		{
! 		  die->type=dwarf2_cached_types[hashval];
  		}
  	      else
  		{
--- 4532,4554 ----
        attr = dwarf_attr (die, DW_AT_name);
        if (attr && DW_STRING (attr))
  	{
! 	  if (cu_language = language_cplus)
  	    {
! 	      char *attrname=DW_STRING (attr);
! 	      unsigned long hashval=hash(attrname, strlen(attrname)) % TYPE_HASH_SIZE;
! 	      if (dwarf2_cached_types[hashval] != NULL)
  		{
! 		  const char *nameoftype;
! 		  nameoftype = TYPE_NAME(dwarf2_cached_types[hashval]) = NULL ? TYPE_TAG_NAME(dwarf2_cached_types[hashval]) : TYPE_NAME(dwarf2_cached_types[hashval]);
! 		  if (strcmp(attrname, nameoftype) = 0)
! 		    {
! 		      die->type=dwarf2_cached_types[hashval];
! 		    }
! 		  else
! 		    {
! 		      read_type_die (die, objfile, cu_header);
! 		      dwarf2_cached_types[hashval] = die->type;
! 		    }
  		}
  	      else
  		{
*************** tag_type_to_type (struct die_info *die, 
*** 4552,4558 ****
  	  else
  	    {
  	      read_type_die (die, objfile, cu_header);
- 	      dwarf2_cached_types[hashval] = die->type;
  	    }
  	}
        else
--- 4559,4564 ----

Re: [Linux-ia64] Re: gdb null ptr

[Jim Blandy]

I don't think limiting the caching to C++ programs solves the problem.

If I understand the situation, the SGI compiler attaches a DW_AT_name
attribute to array types.  We don't know whether this behavior occurs
only in C compilation units, or in C++ compilation units as well.

If the SGI compiler emits bogus names for array types in C++ code too,
then the same bug will reappear when debugging C++ code, even with
this patch applied.

I think the change to tag_type_to_type needs to be reverted
altogether.  I understand that it provides substantial savings in
storage, but it's simply not correct.

Re: [Linux-ia64] Re: gdb null ptr

[Daniel Berlin]

Jim Blandy <jimb@cygnus.com> writes:

> I don't think limiting the caching to C++ programs solves the problem.
> 
> If I understand the situation, the SGI compiler attaches a DW_AT_name
> attribute to array types.  We don't know whether this behavior occurs
> only in C compilation units, or in C++ compilation units as well.
> 
> If the SGI compiler emits bogus names for array types in C++ code too,
> then the same bug will reappear when debugging C++ code, even with
> this patch applied.
> 
> I think the change to tag_type_to_type needs to be reverted
> altogether.  I understand that it provides substantial savings in
> storage, but it's simply not correct.

I'll revert it.

Okay, but we could still cache symbols and psymbols, that we have a
mangled name for, without any issues, in C++.

We could also cache most types in C++, using the mangled names of the
always existing operator or copy constructor that must be present in
the type, as the hash key.
This won't work for non-structs/classes, but we'll just not cache
them.

Anytime we have a mangled name we can use, we can cache using it,
since it's guaranteed to be unique.

--Dan