[patch] Fix empty PC range psymtab<->symtab discrepancy

[patch] Fix empty PC range psymtab<->symtab discrepancy

[Jan Kratochvil]

Hi,

it may not have an impact on real world code but at least the testcase of
	[RFA] c++/12506
	http://sourceware.org/ml/gdb-patches/2011-03/msg00189.html
was tricky to debug due to it.

DWARF-4 does not say explicitly what does mean DW_AT_low_pc == DW_AT_high_pc
but for DW_AT_ranges it says
	A range list entry (but not a base address selection or end of list
	entry) whose beginning and ending addresses are equal has no effect
	because the size of the range covered by such an entry is zero.
therefore I believe DW_AT_low_pc == DW_AT_high_pc means such DIE should be
ignored and such DIE is IMO invalid.

For DW_AT_high_pc DWARF-4 has the comment
	The high PC value may be beyond the last valid instruction in the
	executable.
which may suggest DW_AT_low_pc == DW_AT_high_pc == 0 may be valid and it
should mean the whole address space.  I find such case outside of the scope of
this patch, such case already did not work as the partial symtabs reading
already ignored DIEs with DW_AT_low_pc == DW_AT_high_pc.  This patch has the
goal to sync the behavior of psymtabs and symtabs.

No regressions on {x86_64,x86_64-m32,i686}-fedora15-linux-gnu.
gdb.dwarf2/pr11465.S needed a fix up as can be seen.

I am not going to check it in without comments/approval.


Thanks,
Jan


gdb/
2011-03-14  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* dwarf2read.c (dwarf2_get_pc_bounds): Require HIGH strictly higher
	than LOW.  Comment it.
	(read_partial_die): Call complaint for inappropriate zero LOWPC or
	HIGHPC not strictly higher than LOWPC.

gdb/testsuite/
2011-03-14  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.dwarf2/dw2-empty-pc-range.S: New file.
	* gdb.dwarf2/dw2-empty-pc-range.exp: New file.
	* gdb.dwarf2/pr11465.S: New .text labels text_start and text_end.
	Provide a stub byte there.
	(DW_TAG_compile_unit): Set DW_AT_low_pc, DW_AT_high_pc and
	DW_AT_entry_pc.
	(dieb4, dieda): Set DW_AT_high_pc higher than DW_AT_low_pc.

--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -5982,7 +5982,8 @@ dwarf2_get_pc_bounds (struct die_info *die, CORE_ADDR *lowpc,
 	}
     }
 
-  if (high < low)
+  /* read_partial_die has also the strict LOW < HIGH requirement.  */
+  if (high <= low)
     return 0;
 
   /* When using the GNU linker, .gnu.linkonce. sections are used to
@@ -9127,19 +9128,41 @@ read_partial_die (struct partial_die_info *part_die,
 	}
     }
 
-  /* When using the GNU linker, .gnu.linkonce. sections are used to
-     eliminate duplicate copies of functions and vtables and such.
-     The linker will arbitrarily choose one and discard the others.
-     The AT_*_pc values for such functions refer to local labels in
-     these sections.  If the section from that file was discarded, the
-     labels are not in the output, so the relocs get a value of 0.
-     If this is a discarded function, mark the pc bounds as invalid,
-     so that GDB will ignore it.  */
-  if (has_low_pc_attr && has_high_pc_attr
-      && part_die->lowpc < part_die->highpc
-      && (part_die->lowpc != 0
-	  || dwarf2_per_objfile->has_section_at_zero))
-    part_die->has_pc_info = 1;
+  if (has_low_pc_attr && has_high_pc_attr)
+    {
+      /* When using the GNU linker, .gnu.linkonce. sections are used to
+	 eliminate duplicate copies of functions and vtables and such.
+	 The linker will arbitrarily choose one and discard the others.
+	 The AT_*_pc values for such functions refer to local labels in
+	 these sections.  If the section from that file was discarded, the
+	 labels are not in the output, so the relocs get a value of 0.
+	 If this is a discarded function, mark the pc bounds as invalid,
+	 so that GDB will ignore it.  */
+      if (part_die->lowpc == 0 && !dwarf2_per_objfile->has_section_at_zero)
+	{
+	  struct gdbarch *gdbarch = get_objfile_arch (cu->objfile);
+
+	  complaint (&symfile_complaints,
+		     _("DW_AT_low_pc %s is zero "
+		       "for DIE at 0x%x [in module %s]"),
+		     paddress (gdbarch, part_die->lowpc),
+		     part_die->offset, cu->objfile->name);
+	}
+      /* dwarf2_get_pc_bounds has also the strict low < high requirement.  */
+      else if (part_die->lowpc >= part_die->highpc)
+	{
+	  struct gdbarch *gdbarch = get_objfile_arch (cu->objfile);
+
+	  complaint (&symfile_complaints,
+		     _("DW_AT_low_pc %s is not < DW_AT_high_pc %s "
+		       "for DIE at 0x%x [in module %s]"),
+		     paddress (gdbarch, part_die->lowpc),
+		     paddress (gdbarch, part_die->highpc),
+		     part_die->offset, cu->objfile->name);
+	}
+      else
+	part_die->has_pc_info = 1;
+    }
 
   return info_ptr;
 }
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-empty-pc-range.S
@@ -0,0 +1,82 @@
+/* Copyright 2011, 2011 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/>.  */
+
+	.text
+pc_start:
+	.byte	0
+pc_end:
+
+	.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) */
+	.ascii	"GCC\0"	/* DW_AT_producer */
+	.byte	0x2	/* DW_AT_language = DW_LANG_C */
+	.ascii	"1.c\0"	/* DW_AT_name */
+
+	.uleb128 0x2		/* (DIE (0xd3) DW_TAG_subprogram) */
+	.byte	0x1		/* DW_AT_external */
+	.ascii	"realrange\0"	/* DW_AT_name */
+	.4byte	pc_start	/* DW_AT_low_pc */
+	.4byte	pc_end		/* DW_AT_high_pc */
+	.byte	0x1		/* DW_AT_prototyped */
+
+	.uleb128 0x2		/* (DIE (0xd3) DW_TAG_subprogram) */
+	.byte	0x1		/* DW_AT_external */
+	.ascii	"emptyrange\0"	/* DW_AT_name */
+	.4byte	pc_start	/* DW_AT_low_pc */
+	.4byte	pc_start	/* DW_AT_high_pc */
+	.byte	0x1		/* DW_AT_prototyped */
+
+	.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 0x8	/* (DW_FORM_string) */
+	.uleb128 0x13	/* (DW_AT_language) */
+	.uleb128 0xb	/* (DW_FORM_data1) */
+	.uleb128 0x3	/* (DW_AT_name) */
+	.uleb128 0x8	/* (DW_FORM_string) */
+	.byte	0x0
+	.byte	0x0
+
+	.uleb128 0x2	/* (abbrev code) */
+	.uleb128 0x2e	/* (DW_TAG_subprogram) */
+	.byte	0x0	/* DW_children_no */
+	.uleb128 0x3f	/* (DW_AT_external) */
+	.uleb128 0xc	/* (DW_FORM_flag) */
+	.uleb128 0x3	/* (DW_AT_name) */
+	.uleb128 0x8	/* (DW_FORM_string) */
+	.uleb128 0x11	/* (DW_AT_low_pc) */
+	.uleb128 0x1	/* (DW_FORM_addr) */
+	.uleb128 0x12	/* (DW_AT_high_pc) */
+	.uleb128 0x1	/* (DW_FORM_addr) */
+	.uleb128 0x27	/* (DW_AT_prototyped) */
+	.uleb128 0xc	/* (DW_FORM_flag) */
+	.byte	0x0
+	.byte	0x0
+
+	.byte	0x0
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-empty-pc-range.exp
@@ -0,0 +1,40 @@
+# Copyright 2011 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/>.
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0  
+}
+
+set testfile "dw2-empty-pc-range"
+set srcfile ${testfile}.S
+set executable ${testfile}.x
+set binfile ${objdir}/${subdir}/${executable}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" object {}] != "" } {
+    return -1
+}
+
+clean_restart $executable
+gdb_test "ptype emptyrange" {No symbol "emptyrange" in current context\.} \
+	 "empty range before CU load"
+
+# Force loading the CU
+gdb_test "ptype realrange" {type = void \(void\)} \
+	 "valid range after CU load"
+
+gdb_test "ptype emptyrange" {No symbol "emptyrange" in current context\.} \
+	 "empty range after CU load"
--- a/gdb/testsuite/gdb.dwarf2/pr11465.S
+++ b/gdb/testsuite/gdb.dwarf2/pr11465.S
@@ -36,7 +36,12 @@
 */
 
 	.text
-_ZN1N1cE:	
+text_start:
+_ZN1N1cE:
+	/* Valid function must have non-empty PC range.  */
+	.byte 0
+text_end:
+
 	.section	.debug_info
 d:
 	.long	.Ldebug_info_end - 1f /* Length of CU info */
@@ -49,9 +54,9 @@ dieb:	.uleb128 0x1		/* DW_TAG_compile_unit */
 	.byte	0x4		/* DW_AT_language */
 	.long	.LASF5		/* DW_AT_name */
 	.long	.LASF6		/* DW_AT_comp_dir */
-	.long	0x0		/* DW_AT_low_pc */
-	.long	0x0		/* DW_AT_high_pc */
-	.long	0x0		/* DW_AT_entry_pc */
+	.long	text_start	/* DW_AT_low_pc */
+	.long	text_end	/* DW_AT_high_pc */
+	.long	text_start	/* DW_AT_entry_pc */
 die29:	.uleb128 0x2		/* DW_TAG_namespace */
 	.string	"N"		/* DW_AT_name */
 die32:	.uleb128 0x3		/* DW_TAG_class_type */
@@ -112,7 +117,7 @@ dieaf:	.uleb128 0xe		/* DW_TAG_const_type */
 dieb4:	.uleb128 0xf		/* DW_TAG_subprogram */
 	.long	die95-d		/* DW_AT_abstract_origin */
 	.long	_ZN1N1cE	/* DW_AT_low_pc */
-	.long	_ZN1N1cE	/* DW_AT_high_pc */
+	.long	_ZN1N1cE + 1	/* DW_AT_high_pc */
 diec9:	.uleb128 0x10		/* DW_TAG_subprogram */
 	.long	die9f-d		/* DW_AT_abstract_origin */
 	.byte	2f-1f		/* DW_AT_location */
@@ -131,7 +136,7 @@ dieda:	.uleb128 0x11		/* DW_TAG_subprogram */
 	.long	.LASF8		/* DW_AT_name */
 	.long	dief2-d		/* DW_AT_type */
 	.long	_ZN1N1cE	/* DW_AT_low_pc */
-	.long	_ZN1N1cE	/* DW_AT_high_pc */
+	.long	_ZN1N1cE + 1	/* DW_AT_high_pc */
 dief2:	.uleb128 0x12		/* DW_TAG_base_type */
 	.byte	0x4		/* DW_AT_byte_size */
 	.byte	0x5		/* DW_AT_encoding */

Re: [patch] Fix empty PC range psymtab<->symtab discrepancy

[Tom Tromey]

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

Jan> For DW_AT_high_pc DWARF-4 has the comment
Jan> 	The high PC value may be beyond the last valid instruction in the
Jan> 	executable.
Jan> which may suggest DW_AT_low_pc == DW_AT_high_pc == 0 may be valid
Jan> and it should mean the whole address space.  I find such case
Jan> outside of the scope of this patch, such case already did not work
Jan> as the partial symtabs reading already ignored DIEs with
Jan> DW_AT_low_pc == DW_AT_high_pc.

I would not worry about this case at all.

The patch looks good to me.

Jan> +	  complaint (&symfile_complaints,
Jan> +		     _("DW_AT_low_pc %s is not < DW_AT_high_pc %s "
Jan> +		       "for DIE at 0x%x [in module %s]"),
Jan> +		     paddress (gdbarch, part_die->lowpc),
Jan> +		     paddress (gdbarch, part_die->highpc),
Jan> +		     part_die->offset, cu->objfile->name);

Thanks for putting this info into the complaint.
One of my minor wish-list items is that we would do this for all DWARF
complaints.

Tom

DWARF sanity checking [Re: [patch] Fix empty PC range psymtab<->symtab discrepancy]

[Jan Kratochvil]

On Tue, 15 Mar 2011 16:45:05 +0100, Tom Tromey wrote:
> The patch looks good to me.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2011-03/msg00188.html


> Jan> +	  complaint (&symfile_complaints,
> Jan> +		     _("DW_AT_low_pc %s is not < DW_AT_high_pc %s "
> Jan> +		       "for DIE at 0x%x [in module %s]"),
> Jan> +		     paddress (gdbarch, part_die->lowpc),
> Jan> +		     paddress (gdbarch, part_die->highpc),
> Jan> +		     part_die->offset, cu->objfile->name);
> 
> Thanks for putting this info into the complaint.
> One of my minor wish-list items is that we would do this for all DWARF
> complaints.

While not a GNU project this functionality overlaps with the
pmachata/dwarflint branch of elfutils which should be more complete as the
checks are not just a side-effect.  Unaware how easy would be to port it for
non-ELF DWARF and whether the elfutils GPL exception is good enough for FSF.


Thanks,
Jan

Re: DWARF sanity checking [Re: [patch] Fix empty PC range psymtab<->symtab discrepancy]

[Tom Tromey]

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

Tom> Thanks for putting this info into the complaint.
Tom> One of my minor wish-list items is that we would do this for all DWARF
Tom> complaints.

Jan> While not a GNU project this functionality overlaps with the
Jan> pmachata/dwarflint branch of elfutils which should be more complete
Jan> as the checks are not just a side-effect.  Unaware how easy would
Jan> be to port it for non-ELF DWARF and whether the elfutils GPL
Jan> exception is good enough for FSF.

Yeah, I don't think gdb's DWARF reader will or should try to do what
dwarflint does.  But I also doubt we'll ever remove the complaints from
dwarf2read.c; and of course gdb may emit complaints that dwarflint will
not (e.g., for valid but odd stuff not handled by gdb).  And, given that
we will probably always have complaint calls, I think they should
provide enough information to actually find the problem DIE.

Tom