[patch] Fix dwarf2read to crash again

[patch] Fix dwarf2read to crash again

[Jan Kratochvil]

Hi,

there are multiple related patches:

[modula2]
Re: recursive bug in dwarf2read.c
From: Gaius Mulley <gaius at glam dot ac dot uk>
http://sourceware.org/ml/gdb-patches/2009-12/msg00152.html
http://sourceware.org/ml/gdb-cvs/2009-12/msg00068.html
2d04d865270162e7119b668cc4b2865fb75d5cd1
Attached here a new testcase for it:
gdb.dwarf2/dw2-modula2-self-type.exp

[physname]
Re: [RFA] dwarf2_physname FINAL
http://sourceware.org/ml/gdb-patches/2010-03/msg00359.html
42284fdf9d8cdb20c8e833bdbdb2b56977fea525

[readerror]
[patch] Fix crash on NULL function's TYPE_FIELD_TYPE
http://sourceware.org/ml/gdb-patches/2010-03/msg01039.html
http://sourceware.org/ml/gdb-cvs/2010-03/msg00302.html
7e8e956c34e0fd4f0b59ef1c46f8966c66fffbf3
The checked-in testcase is:
gdb.dwarf2/dw2-bad-parameter-type.exp

[delay]
[RFA] Delayed physname computation
http://sourceware.org/ml/gdb-patches/2010-04/msg00641.html
Posted now, the new pending testcase is:
gdb.dwarf2/pr11465.exp

[this]
This attached patch.

              dw2-modula2-self-type.exp dw2-bad-parameter-type.exp pr11465.exp
2d04d865^     CRASH                     pass                       unaware:pass
[modula2]
42284fdf^     pass                      CRASH                      unaware:FAIL
[physname]
7e8e956c^     pass                      CRASH                      CRASH
[readerror]
HEAD          pass                      pass                       FAIL
HEAD+[this]   pass                      pass                       CRASH
HEAD+[delay]  pass                      pass                       pass
[delay][this] pass                      pass                       pass


Therefore the patch of mine [readerror] turned CRASH->FAIL in some cases. The
FAIL case is difficult to understand and occurs only in very rare C++ cases
(one can -readnow libwebkit.so) so it could hide for a long time the [delay]
bug confusing C++ debugging.

The purpose of this patch is to make GDB crash again when the [delay] fix is
not applied.  It has been a regression GDB no longer crashed due to its buggy
code [physname]-before-[delay].

No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


gdb/
2010-04-21  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* dwarf2read.c: Include exceptions.h.
	(read_subroutine_type): Move void_type into a more inner block.  New
	variable e.  Wrap a die_type call by TRY_CATCH.  Delay parameters
	filling only to the failure exit path.

gdb/testsuite/
2010-04-21  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.dwarf2/dw2-modula2-self-type.exp,
	gdb.dwarf2/dw2-modula2-self-type.S: New.

--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -51,6 +51,7 @@
 #include "typeprint.h"
 #include "jv-lang.h"
 #include "psympriv.h"
+#include "exceptions.h"
 
 #include <fcntl.h>
 #include "gdb_string.h"
@@ -5943,7 +5944,6 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
   
   if (die->child != NULL)
     {
-      struct type *void_type = objfile_type (cu->objfile)->builtin_void;
       struct die_info *child_die;
       int nparams, iparams;
 
@@ -5966,17 +5966,14 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
       TYPE_FIELDS (ftype) = (struct field *)
 	TYPE_ZALLOC (ftype, nparams * sizeof (struct field));
 
-      /* TYPE_FIELD_TYPE must never be NULL.  Pre-fill the array to ensure it
-	 even if we error out during the parameters reading below.  */
-      for (iparams = 0; iparams < nparams; iparams++)
-	TYPE_FIELD_TYPE (ftype, iparams) = void_type;
-
       iparams = 0;
       child_die = die->child;
       while (child_die && child_die->tag)
 	{
 	  if (child_die->tag == DW_TAG_formal_parameter)
 	    {
+	      volatile struct gdb_exception e;
+
 	      /* Dwarf2 has no clean way to discern C++ static and non-static
 	         member functions. G++ helps GDB by marking the first
 	         parameter for non-static member functions (which is the
@@ -5998,7 +5995,35 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
 			TYPE_FIELD_ARTIFICIAL (ftype, iparams) = 1;
 		    }
 		}
-	      TYPE_FIELD_TYPE (ftype, iparams) = die_type (child_die, cu);
+
+	      TRY_CATCH (e, RETURN_MASK_ALL)
+		{
+		  TYPE_FIELD_TYPE (ftype, iparams) = die_type (child_die, cu);
+		}
+	      if (e.reason < 0)
+		{
+		  struct type *void_type;
+		  
+		  void_type = objfile_type (cu->objfile)->builtin_void;
+
+		  /* TYPE_FIELD_TYPE must never be NULL.  As this TYPE has been
+		     already installed by set_die_type above we can no longer
+		     withdraw it.  At least fill-in the parameters types by
+		     stub types to ensure its consumers will not crash on it
+		     later.
+
+		     On the other hand do not fill in the parameters types in
+		     advance as the DWARF reader is fragile regarding DIEs
+		     reading order and we want the possible ordering bugs to be
+		     caught.  If anyone tries to access the TYPE parameters
+		     before this function finalizes TYPE it should at least
+		     crash.  Filling in the parameters types in advance could
+		     make the early readers quietly interpret this type as with
+		     no (void) parameters.  */
+		  for (; iparams < nparams; iparams++)
+		    TYPE_FIELD_TYPE (ftype, iparams) = void_type;
+		  throw_exception (e);
+		}
 	      iparams++;
 	    }
 	  child_die = sibling_die (child_die);
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-modula2-self-type.S
@@ -0,0 +1,123 @@
+/* Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Compiled from:
+   MODULE m;
+   TYPE t = PROCEDURE (t);
+   VAR v: t;
+   BEGIN
+   END m.
+   But "v" has been removed afterwards.  */
+
+	.data
+v:	.long	0
+
+	.section	.debug_info
+d:
+	.long	.Ldebug_info_end - 1f	/* Length of Compilation Unit Info */
+1:
+	.2byte	0x3	/* DWARF version number */
+	.long	.Ldebug_abbrev0	/* Offset Into Abbrev. Section */
+	.byte	0x4	/* Pointer Size (in bytes) */
+dieb:
+	.uleb128 0x1	/* (DIE (0xb) DW_TAG_compile_unit) */
+	.long	.LASF0	/* DW_AT_producer */
+	.byte	0xa	/* DW_AT_language = DW_LANG_Modula2 */
+	.long	.LASF1	/* DW_AT_name: "2.mod" */
+	.long	.LASF2	/* DW_AT_comp_dir: "" */
+
+die210:
+	.uleb128 0x5	# (DIE (0x210) DW_TAG_subroutine_type)
+die215:
+	.uleb128 0x6	# (DIE (0x215) DW_TAG_formal_parameter)
+	.long	die21b-d	# DW_AT_type
+	.byte	0x0	# end of children of DIE 0x210
+die21b:
+	.uleb128 0x7	# (DIE (0x21b) DW_TAG_pointer_type)
+	.byte	0x4	# DW_AT_byte_size
+	.long	die210-d	# DW_AT_type
+die221:
+	.uleb128 0x8	# (DIE (0x221) DW_TAG_variable)
+	.ascii "v\0"	# DW_AT_name
+	.long	die21b-d	# DW_AT_type
+	.byte	2f-1f	# DW_AT_location
+1:	.byte	0x3	# DW_OP_addr
+	.long	v
+2:
+
+	.byte	0x0	# end of children of DIE 0xb
+.Ldebug_info_end:
+	.section	.debug_abbrev
+.Ldebug_abbrev0:
+
+	.uleb128 0x1	/* (abbrev code) */
+	.uleb128 0x11	/* (TAG: DW_TAG_compile_unit) */
+	.byte	0x1	/* DW_children_yes */
+	.uleb128 0x25	/* (DW_AT_producer) */
+	.uleb128 0xe	/* (DW_FORM_strp) */
+	.uleb128 0x13	/* (DW_AT_language) */
+	.uleb128 0xb	/* (DW_FORM_data1) */
+	.uleb128 0x3	/* (DW_AT_name) */
+	.uleb128 0xe	/* (DW_FORM_strp) */
+	.uleb128 0x1b	/* (DW_AT_comp_dir) */
+	.uleb128 0xe	/* (DW_FORM_strp) */
+	.byte	0x0
+	.byte	0x0
+
+	.uleb128 0x5	# (abbrev code)
+	.uleb128 0x15	# (TAG: DW_TAG_subroutine_type)
+	.byte	0x1	# DW_children_yes
+	.byte	0x0
+	.byte	0x0
+
+	.uleb128 0x6	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+
+	.uleb128 0x7	# (abbrev code)
+	.uleb128 0xf	# (TAG: DW_TAG_pointer_type)
+	.byte	0x0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+
+	.uleb128 0x8	# (abbrev code)
+	.uleb128 0x34	# (TAG: DW_TAG_variable)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.byte	0x0
+	.byte	0x0
+
+	.byte	0x0
+
+	.section	.debug_str
+.LASF1:
+	.string	"2.mod"
+.LASF0:
+	.string	"GNU Modula-2 0.78 (20100402) grafted onto GCC 4.1.2"
+.LASF2:
+	.string	""
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-modula2-self-type.exp
@@ -0,0 +1,48 @@
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+# For now pick a sampling of likely targets.
+if {![istarget *-*-linux*]
+    && ![istarget *-*-gnu*]
+    && ![istarget *-*-elf*]
+    && ![istarget *-*-openbsd*]
+    && ![istarget arm-*-eabi*]
+    && ![istarget powerpc-*-eabi*]} {
+    return 0  
+}
+
+set testfile "dw2-modula2-self-type"
+set srcfile ${testfile}.S
+set executable ${testfile}.x
+set binfile ${objdir}/${subdir}/${executable}
+
+# First try referencing DW_AT_frame_base which is not defined.
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" object {}] != "" } {
+    return -1
+}
+
+clean_restart $executable
+
+# C language would just naively print:
+# type = void (*)(void (*)(void (*)(void (*)(void (*)(...
+# While modula-2 prints:
+# type = POINTER TO PROCEDURE
+gdb_test "set language modula-2"
+
+# The first access (as we do not use -readnow) crashes GDB.
+gdb_test "ptype v"
+
+gdb_test "p 1" " = 1" "alive"

Re: [patch] Fix dwarf2read to crash again

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> Therefore the patch of mine [readerror] turned CRASH->FAIL in some
Jan> cases. The FAIL case is difficult to understand and occurs only in
Jan> very rare C++ cases (one can -readnow libwebkit.so) so it could
Jan> hide for a long time the [delay] bug confusing C++ debugging.

It took me a while to really understand this rationale, but I get it
now.  For us as gdb developers, a crash is preferable because it is
simpler to notice.

I think, however, that the opposite is true for users.  I was talking
with Graydon recently about a different GDB dwarf reader bug he had run
into: if tag_type_to_type fails, then an entire objfile's worth of
debuginfo can be dropped, leading to weird behavior.  This is pretty
unfriendly when the type DIE in question is in the vendor range.

My conclusion from these cases is that gdb ought to fail gracefully.  I
suspect we should even go so far as to wrap process_die (or possibly
some more appropriate function) in a TRY_CATCH and skip DIEs that
confuse gdb.

For the testing case, I think the best thing would be to add a complaint
anywhere we encounter invalid DWARF.  Then, we can enable complaints
when testing -- either in the GDB test suite or whatever other QA
anybody does.

On the QA topic: the particular bug in question was found in a deployed
Fedora system.  We've been discussing an automated debuginfo smoke test,
consisting of running "gdb -readnow" on all the debuginfo in the distro.

The above is why I think this patch should no go in as-is.
I am interested in reactions to the above.

Tom

Re: [patch] Fix dwarf2read to crash again

[Joel Brobecker]

> My conclusion from these cases is that gdb ought to fail gracefully.

Agreed.

> I suspect we should even go so far as to wrap process_die (or possibly
> some more appropriate function) in a TRY_CATCH and skip DIEs that
> confuse gdb.

Sounds like a good idea to me.

-- 
Joel

Re: [patch] Fix dwarf2read to crash again

[Jan Kratochvil]

On Fri, 23 Apr 2010 21:50:54 +0200, Tom Tromey wrote:
> The above is why I think this patch should no go in as-is.
> I am interested in reactions to the above.

I share your opinion dwarf2read.c could properly handle buggy DWARF content.

Currenly is it at least OK to check-in gdb.dwarf2/dw2-modula2-self-type.exp?
It PASSes on any recent GDBs.


Thanks,
Jan

Re: [patch] Fix dwarf2read to crash again

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I share your opinion dwarf2read.c could properly handle buggy DWARF
Jan> content.

Great.  I am going to work on that, after I finish with my current bug.

Jan> Currenly is it at least OK to check-in
Jan> gdb.dwarf2/dw2-modula2-self-type.exp?  It PASSes on any recent
Jan> GDBs.

Yes, thanks.

Tom

Re: [patch] Fix dwarf2read to crash again

[Jan Kratochvil]

On Sat, 08 May 2010 00:33:47 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> Jan> Currenly is it at least OK to check-in
> Jan> gdb.dwarf2/dw2-modula2-self-type.exp?  It PASSes on any recent
> Jan> GDBs.
> 
> Yes, thanks.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-05/msg00089.html


Thanks,
Jan