* [patch] Fix duplicate types for single DIE
@ 2010-05-13 13:18 Jan Kratochvil
2010-05-20 23:35 ` Joel Brobecker
2010-06-04 22:47 ` Tom Tromey
0 siblings, 2 replies; 6+ messages in thread
From: Jan Kratochvil @ 2010-05-13 13:18 UTC (permalink / raw)
To: gdb-patches; +Cc: Keith Seitz
Hi,
this patch can be considered a cleanup, two set_die_type for single DIE should
not hurt. It just started to break new assertion in the pending patch:
Re: [RFA] Delayed physname computation
http://sourceware.org/ml/gdb-patches/2010-05/msg00248.html
# If you put a printf in set_die_type, you'll see that this function is called
# twice (with different type structs) for the same DIE.
Keith has provided "obstack-leak.patch" there as a prerequisite for the
physname patch there. Attaching here a more complete solution of the problem.
On 250MB webkit debuginfo it reduced memory consumption by 3MB from 4616MB
(0.06%) (which is less than I hoped for; checked with Keith's physname patch).
I have not found measureable any performance difference.
No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu, on FSF GDB HEAD
and on FSF GDB HEAD with the Keith's physname patch referenced above.
Tom Tromey may not agree with new internal_error as expressed in
Re: [patch] Fix dwarf2read to crash again
http://sourceware.org/ml/gdb-patches/2010-04/msg00812.html
I am not sure what are his specific plans; if some TRY_CATCH should be
involved the new internal_error call should OK there.
With the new internal_error call I have verified at least the fixes of
read_tag_pointer_type and read_tag_const_type fixes (and not just these two)
to be required. Other fixes are just written artificially to cover all the
cases of any indirect call of tag_type_to_type, that is die_type,
die_descriptive_type, set_descriptive_type and die_containing_type.
(I believe at least those *_descriptive_type are in fact never needed as their
DWARF usage should never lead to DWARF references loops.)
Thanks,
Jan
2010-05-13 Jan Kratochvil <jan.kratochvil@redhat.com>
* dwarf2read.c (read_structure_type): Move set_descriptive_type after
set_die_type.
(read_array_type): Remove type initialization. Recheck get_die_type
after initial die_type. Move set_die_type before set_descriptive_type.
(read_set_type): New variable domain_type. Recheck get_die_type after
initial die_type.
(read_tag_pointer_type, read_tag_reference_type): New variable
target_type. Recheck get_die_type after initial die_type.
(read_tag_ptr_to_member_type): Recheck get_die_type after initial
die_type and die_containing_type.
(read_tag_const_type, read_tag_volatile_type, read_subroutine_type):
Recheck get_die_type after initial die_type.
(read_subrange_type): Recheck get_die_type after initial die_type.
Move set_die_type before set_descriptive_type.
(set_die_type): Call internal_error if DIE has some type already set.
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -5109,13 +5109,14 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
on incomplete types. */
TYPE_STUB (type) = 1;
- set_descriptive_type (type, die, cu);
-
/* We need to add the type field to the die immediately so we don't
infinitely recurse when dealing with pointers to the structure
type within the structure itself. */
set_die_type (die, type, cu);
+ /* set_die_type should be already done. */
+ set_descriptive_type (type, die, cu);
+
if (die->child != NULL && ! die_is_declaration (die, cu))
{
struct field_info fi;
@@ -5403,7 +5404,7 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
{
struct objfile *objfile = cu->objfile;
struct die_info *child_die;
- struct type *type = NULL;
+ struct type *type;
struct type *element_type, *range_type, *index_type;
struct type **range_types = NULL;
struct attribute *attr;
@@ -5413,6 +5414,11 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
element_type = die_type (die, cu);
+ /* The die_type call above may have already set the type for this DIE. */
+ type = get_die_type (die, cu);
+ if (type)
+ return type;
+
/* Irix 6.2 native cc creates array types without children for
arrays with unspecified length. */
if (die->child == NULL)
@@ -5479,12 +5485,15 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
if (name)
TYPE_NAME (type) = name;
+ /* Install the type in the die. */
+ set_die_type (die, type, cu);
+
+ /* set_die_type should be already done. */
set_descriptive_type (type, die, cu);
do_cleanups (back_to);
- /* Install the type in the die. */
- return set_die_type (die, type, cu);
+ return type;
}
static enum dwarf_array_dim_ordering
@@ -5527,7 +5536,16 @@ read_array_order (struct die_info *die, struct dwarf2_cu *cu)
static struct type *
read_set_type (struct die_info *die, struct dwarf2_cu *cu)
{
- struct type *set_type = create_set_type (NULL, die_type (die, cu));
+ struct type *domain_type, *set_type;
+
+ domain_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ set_type = get_die_type (die, cu);
+ if (set_type)
+ return set_type;
+
+ set_type = create_set_type (NULL, domain_type);
return set_die_type (die, set_type, cu);
}
@@ -5725,8 +5743,16 @@ read_tag_pointer_type (struct die_info *die, struct dwarf2_cu *cu)
struct attribute *attr_byte_size;
struct attribute *attr_address_class;
int byte_size, addr_class;
+ struct type *target_type;
- type = lookup_pointer_type (die_type (die, cu));
+ target_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ type = get_die_type (die, cu);
+ if (type)
+ return type;
+
+ type = lookup_pointer_type (target_type);
attr_byte_size = dwarf2_attr (die, DW_AT_byte_size, cu);
if (attr_byte_size)
@@ -5781,6 +5807,11 @@ read_tag_ptr_to_member_type (struct die_info *die, struct dwarf2_cu *cu)
to_type = die_type (die, cu);
domain = die_containing_type (die, cu);
+ /* The calls above may have already set the type for this DIE. */
+ type = get_die_type (die, cu);
+ if (type)
+ return type;
+
if (TYPE_CODE (check_typedef (to_type)) == TYPE_CODE_METHOD)
type = lookup_methodptr_type (to_type);
else
@@ -5796,10 +5827,17 @@ static struct type *
read_tag_reference_type (struct die_info *die, struct dwarf2_cu *cu)
{
struct comp_unit_head *cu_header = &cu->header;
- struct type *type;
+ struct type *type, *target_type;
struct attribute *attr;
- type = lookup_reference_type (die_type (die, cu));
+ target_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ type = get_die_type (die, cu);
+ if (type)
+ return type;
+
+ type = lookup_reference_type (target_type);
attr = dwarf2_attr (die, DW_AT_byte_size, cu);
if (attr)
{
@@ -5818,6 +5856,12 @@ read_tag_const_type (struct die_info *die, struct dwarf2_cu *cu)
struct type *base_type, *cv_type;
base_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ cv_type = get_die_type (die, cu);
+ if (cv_type)
+ return cv_type;
+
cv_type = make_cv_type (1, TYPE_VOLATILE (base_type), base_type, 0);
return set_die_type (die, cv_type, cu);
}
@@ -5828,6 +5872,12 @@ read_tag_volatile_type (struct die_info *die, struct dwarf2_cu *cu)
struct type *base_type, *cv_type;
base_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ cv_type = get_die_type (die, cu);
+ if (cv_type)
+ return cv_type;
+
cv_type = make_cv_type (TYPE_CONST (base_type), 1, base_type, 0);
return set_die_type (die, cv_type, cu);
}
@@ -5892,6 +5942,12 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
struct attribute *attr;
type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ ftype = get_die_type (die, cu);
+ if (ftype)
+ return ftype;
+
ftype = lookup_function_type (type);
/* All functions in C++, Pascal and Java have prototypes. */
@@ -6103,6 +6159,12 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
LONGEST negative_mask;
base_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ range_type = get_die_type (die, cu);
+ if (range_type)
+ return range_type;
+
if (TYPE_CODE (base_type) == TYPE_CODE_VOID)
{
complaint (&symfile_complaints,
@@ -6170,9 +6232,12 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
if (attr)
TYPE_LENGTH (range_type) = DW_UNSND (attr);
+ set_die_type (die, range_type, cu);
+
+ /* set_die_type should be already done. */
set_descriptive_type (range_type, die, cu);
- return set_die_type (die, range_type, cu);
+ return range_type;
}
static struct type *
@@ -12007,6 +12072,10 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
ofs.type = type;
slot = (struct dwarf2_offset_and_type **)
htab_find_slot_with_hash (cu->type_hash, &ofs, ofs.offset, INSERT);
+ if (*slot)
+ internal_error (__FILE__, __LINE__,
+ _("set_die_type: DIE 0x%x has type already set"),
+ die->offset);
*slot = obstack_alloc (&cu->objfile->objfile_obstack, sizeof (**slot));
**slot = ofs;
return type;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix duplicate types for single DIE
2010-05-13 13:18 [patch] Fix duplicate types for single DIE Jan Kratochvil
@ 2010-05-20 23:35 ` Joel Brobecker
2010-05-21 12:18 ` Jan Kratochvil
2010-06-04 22:47 ` Tom Tromey
1 sibling, 1 reply; 6+ messages in thread
From: Joel Brobecker @ 2010-05-20 23:35 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: gdb-patches, Keith Seitz
> Tom Tromey may not agree with new internal_error as expressed in
> Re: [patch] Fix dwarf2read to crash again
> http://sourceware.org/ml/gdb-patches/2010-04/msg00812.html
> I am not sure what are his specific plans; if some TRY_CATCH should be
> involved the new internal_error call should OK there.
This is a situation where we can recover without much hassle by just
allocating a new slot, so I'd just emit a complaint, and allow the
user to continue debugging.
> 2010-05-13 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * dwarf2read.c (read_structure_type): Move set_descriptive_type after
> set_die_type.
> (read_array_type): Remove type initialization. Recheck get_die_type
> after initial die_type. Move set_die_type before set_descriptive_type.
> (read_set_type): New variable domain_type. Recheck get_die_type after
> initial die_type.
> (read_tag_pointer_type, read_tag_reference_type): New variable
> target_type. Recheck get_die_type after initial die_type.
> (read_tag_ptr_to_member_type): Recheck get_die_type after initial
> die_type and die_containing_type.
> (read_tag_const_type, read_tag_volatile_type, read_subroutine_type):
> Recheck get_die_type after initial die_type.
> (read_subrange_type): Recheck get_die_type after initial die_type.
> Move set_die_type before set_descriptive_type.
> (set_die_type): Call internal_error if DIE has some type already set.
I skimmed through the patch, and it looks reasonable. But I don't
feel comfortable approving it - don't know that file well enough
anymore. Sorry.
There's something I can seem to be able to understand: Why does
set_descriptive_type need to be called after set_die_type? I mean,
from a conceptual point of view, it sort of makes sense. But I can't
see how in practice changing the call order makes any difference...
--
Joel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix duplicate types for single DIE
2010-05-20 23:35 ` Joel Brobecker
@ 2010-05-21 12:18 ` Jan Kratochvil
2010-05-21 15:09 ` Joel Brobecker
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kratochvil @ 2010-05-21 12:18 UTC (permalink / raw)
To: Joel Brobecker; +Cc: gdb-patches, Keith Seitz
On Fri, 21 May 2010 01:31:31 +0200, Joel Brobecker wrote:
> > Tom Tromey may not agree with new internal_error as expressed in
[...]
> This is a situation where we can recover without much hassle by just
> allocating a new slot, so I'd just emit a complaint, and allow the
> user to continue debugging.
OK, that looks better, changed.
> There's something I can seem to be able to understand: Why does
> set_descriptive_type need to be called after set_die_type? I mean,
> from a conceptual point of view, it sort of makes sense. But I can't
> see how in practice changing the call order makes any difference...
Created there a new comment at set_die_type for some general rules of reading
in DIEs/TYPEs and their inter-referencing. I see it was missing there.
Therefore I find it more simple to just follow some paradigm than to think
whether it is safe to violate it in that specific case. Moreover I was not
sure about this Ada descriptive types myself (which you have made clear now).
Also the order does not matter IMO, code efficiency is the same.
I can remove these descriptive_type changes if you think so.
Thanks,
Jan
2010-05-21 Jan Kratochvil <jan.kratochvil@redhat.com>
* dwarf2read.c (read_structure_type): Move set_descriptive_type after
set_die_type.
(read_array_type): Remove type initialization. Recheck get_die_type
after initial die_type. Move set_die_type before set_descriptive_type.
(read_set_type): New variable domain_type. Recheck get_die_type after
initial die_type. Move attr initialization later.
(read_tag_pointer_type, read_tag_reference_type): New variable
target_type. Recheck get_die_type after initial die_type.
(read_tag_ptr_to_member_type): Recheck get_die_type after initial
die_type and die_containing_type.
(read_tag_const_type, read_tag_volatile_type, read_subroutine_type):
Recheck get_die_type after initial die_type.
(read_subrange_type): Recheck get_die_type after initial die_type.
Move set_die_type before set_descriptive_type.
(set_die_type): Extend the function comment. Call complaint if DIE has
some type already set.
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -5125,13 +5125,14 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
on incomplete types. */
TYPE_STUB (type) = 1;
- set_descriptive_type (type, die, cu);
-
/* We need to add the type field to the die immediately so we don't
infinitely recurse when dealing with pointers to the structure
type within the structure itself. */
set_die_type (die, type, cu);
+ /* set_die_type should be already done. */
+ set_descriptive_type (type, die, cu);
+
if (die->child != NULL && ! die_is_declaration (die, cu))
{
struct field_info fi;
@@ -5420,7 +5421,7 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
{
struct objfile *objfile = cu->objfile;
struct die_info *child_die;
- struct type *type = NULL;
+ struct type *type;
struct type *element_type, *range_type, *index_type;
struct type **range_types = NULL;
struct attribute *attr;
@@ -5430,6 +5431,11 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
element_type = die_type (die, cu);
+ /* The die_type call above may have already set the type for this DIE. */
+ type = get_die_type (die, cu);
+ if (type)
+ return type;
+
/* Irix 6.2 native cc creates array types without children for
arrays with unspecified length. */
if (die->child == NULL)
@@ -5498,12 +5504,15 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
if (name)
TYPE_NAME (type) = name;
+ /* Install the type in the die. */
+ set_die_type (die, type, cu);
+
+ /* set_die_type should be already done. */
set_descriptive_type (type, die, cu);
do_cleanups (back_to);
- /* Install the type in the die. */
- return set_die_type (die, type, cu);
+ return type;
}
static enum dwarf_array_dim_ordering
@@ -5546,11 +5555,22 @@ read_array_order (struct die_info *die, struct dwarf2_cu *cu)
static struct type *
read_set_type (struct die_info *die, struct dwarf2_cu *cu)
{
- struct type *set_type = create_set_type (NULL, die_type (die, cu));
- struct attribute *attr = dwarf2_attr (die, DW_AT_byte_size, cu);
+ struct type *domain_type, *set_type;
+ struct attribute *attr;
+ domain_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ set_type = get_die_type (die, cu);
+ if (set_type)
+ return set_type;
+
+ set_type = create_set_type (NULL, domain_type);
+
+ attr = dwarf2_attr (die, DW_AT_byte_size, cu);
if (attr)
TYPE_LENGTH (set_type) = DW_UNSND (attr);
+
return set_die_type (die, set_type, cu);
}
@@ -5749,8 +5769,16 @@ read_tag_pointer_type (struct die_info *die, struct dwarf2_cu *cu)
struct attribute *attr_byte_size;
struct attribute *attr_address_class;
int byte_size, addr_class;
+ struct type *target_type;
+
+ target_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ type = get_die_type (die, cu);
+ if (type)
+ return type;
- type = lookup_pointer_type (die_type (die, cu));
+ type = lookup_pointer_type (target_type);
attr_byte_size = dwarf2_attr (die, DW_AT_byte_size, cu);
if (attr_byte_size)
@@ -5806,6 +5834,11 @@ read_tag_ptr_to_member_type (struct die_info *die, struct dwarf2_cu *cu)
to_type = die_type (die, cu);
domain = die_containing_type (die, cu);
+ /* The calls above may have already set the type for this DIE. */
+ type = get_die_type (die, cu);
+ if (type)
+ return type;
+
if (TYPE_CODE (check_typedef (to_type)) == TYPE_CODE_METHOD)
type = lookup_methodptr_type (to_type);
else
@@ -5821,10 +5854,17 @@ static struct type *
read_tag_reference_type (struct die_info *die, struct dwarf2_cu *cu)
{
struct comp_unit_head *cu_header = &cu->header;
- struct type *type;
+ struct type *type, *target_type;
struct attribute *attr;
- type = lookup_reference_type (die_type (die, cu));
+ target_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ type = get_die_type (die, cu);
+ if (type)
+ return type;
+
+ type = lookup_reference_type (target_type);
attr = dwarf2_attr (die, DW_AT_byte_size, cu);
if (attr)
{
@@ -5843,6 +5883,12 @@ read_tag_const_type (struct die_info *die, struct dwarf2_cu *cu)
struct type *base_type, *cv_type;
base_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ cv_type = get_die_type (die, cu);
+ if (cv_type)
+ return cv_type;
+
cv_type = make_cv_type (1, TYPE_VOLATILE (base_type), base_type, 0);
return set_die_type (die, cv_type, cu);
}
@@ -5853,6 +5899,12 @@ read_tag_volatile_type (struct die_info *die, struct dwarf2_cu *cu)
struct type *base_type, *cv_type;
base_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ cv_type = get_die_type (die, cu);
+ if (cv_type)
+ return cv_type;
+
cv_type = make_cv_type (TYPE_CONST (base_type), 1, base_type, 0);
return set_die_type (die, cv_type, cu);
}
@@ -5917,6 +5969,12 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
struct attribute *attr;
type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ ftype = get_die_type (die, cu);
+ if (ftype)
+ return ftype;
+
ftype = lookup_function_type (type);
/* All functions in C++, Pascal and Java have prototypes. */
@@ -6129,6 +6187,12 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
LONGEST negative_mask;
base_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ range_type = get_die_type (die, cu);
+ if (range_type)
+ return range_type;
+
if (TYPE_CODE (base_type) == TYPE_CODE_VOID)
{
complaint (&symfile_complaints,
@@ -6196,9 +6260,12 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
if (attr)
TYPE_LENGTH (range_type) = DW_UNSND (attr);
+ set_die_type (die, range_type, cu);
+
+ /* set_die_type should be already done. */
set_descriptive_type (range_type, die, cu);
- return set_die_type (die, range_type, cu);
+ return range_type;
}
static struct type *
@@ -11984,7 +12051,22 @@ offset_and_type_eq (const void *item_lhs, const void *item_rhs)
}
/* Set the type associated with DIE to TYPE. Save it in CU's hash
- table if necessary. For convenience, return TYPE. */
+ table if necessary. For convenience, return TYPE.
+
+ The DIEs reading must have careful ordering to:
+ * Not cause infite loops trying to read in DIEs as a prerequisite for
+ reading current DIE.
+ * Not trying to dereference contents of still incompletely read in types
+ while reading in other DIEs.
+ * Enable referencing still incompletely read in types just by a pointer to
+ the type without accessing its fields.
+
+ Therefore caller should follow these rules:
+ * Try to fetch any prerequisite types we may need to build this DIE type
+ before building the type and calling set_die_type.
+ * After building typer call set_die_type for current DIE as soon as
+ possible before fetching more types to complete the current type.
+ * Make the type as complete as possible before fetching more types. */
static struct type *
set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
@@ -12022,6 +12104,10 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
ofs.type = type;
slot = (struct dwarf2_offset_and_type **)
htab_find_slot_with_hash (cu->type_hash, &ofs, ofs.offset, INSERT);
+ if (*slot)
+ complaint (&symfile_complaints,
+ _("A problem internal to GDB: DIE 0x%x has type already set"),
+ die->offset);
*slot = obstack_alloc (&cu->objfile->objfile_obstack, sizeof (**slot));
**slot = ofs;
return type;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix duplicate types for single DIE
2010-05-21 12:18 ` Jan Kratochvil
@ 2010-05-21 15:09 ` Joel Brobecker
0 siblings, 0 replies; 6+ messages in thread
From: Joel Brobecker @ 2010-05-21 15:09 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: gdb-patches, Keith Seitz
> Therefore I find it more simple to just follow some paradigm than to think
> whether it is safe to violate it in that specific case. Moreover I was not
> sure about this Ada descriptive types myself (which you have made clear now).
> Also the order does not matter IMO, code efficiency is the same.
I agree with you on all counts.
--
Joel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix duplicate types for single DIE
2010-05-13 13:18 [patch] Fix duplicate types for single DIE Jan Kratochvil
2010-05-20 23:35 ` Joel Brobecker
@ 2010-06-04 22:47 ` Tom Tromey
2010-06-05 14:55 ` Jan Kratochvil
1 sibling, 1 reply; 6+ messages in thread
From: Tom Tromey @ 2010-06-04 22:47 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: gdb-patches, Keith Seitz
>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
Jan> Keith has provided "obstack-leak.patch" there as a prerequisite for
Jan> the physname patch there. Attaching here a more complete solution
Jan> of the problem.
I think it makes sense. Thanks for doing this.
Jan> On 250MB webkit debuginfo it reduced memory consumption by 3MB from
Jan> 4616MB (0.06%) (which is less than I hoped for; checked with
Jan> Keith's physname patch). I have not found measureable any
Jan> performance difference.
FWIW I've been thinking for a while about how we could "intern" types to
reduce memory use. However, it seems simpler expensive to just wait for
the .debug_types-enabled GCC to be deployed everywhere; also this
approach means no performance hit for GDB.
Jan> Tom Tromey may not agree with new internal_error as expressed in
Jan> Re: [patch] Fix dwarf2read to crash again
Jan> http://sourceware.org/ml/gdb-patches/2010-04/msg00812.html
Jan> I am not sure what are his specific plans; if some TRY_CATCH should be
Jan> involved the new internal_error call should OK there.
I am still going to get back to this eventually -- I took a long detour
through DW_OP_*piece.
I don't have a concrete plan other than to find appropriate places in
the DWARF reader to catch exceptions and arrange to do something
sensible.
I don't have a problem with an internal_error if it can only trigger
when there is a bug in GDB. If strange-but-valid DWARF can trigger it,
then I think it should be a complaint or maybe an ordinary error.
Jan> With the new internal_error call I have verified at least the fixes
Jan> of read_tag_pointer_type and read_tag_const_type fixes (and not
Jan> just these two) to be required. Other fixes are just written
Jan> artificially to cover all the cases of any indirect call of
Jan> tag_type_to_type, that is die_type, die_descriptive_type,
Jan> set_descriptive_type and die_containing_type. (I believe at least
Jan> those *_descriptive_type are in fact never needed as their DWARF
Jan> usage should never lead to DWARF references loops.)
Thanks.
Jan> 2010-05-13 Jan Kratochvil <jan.kratochvil@redhat.com>
Jan> * dwarf2read.c (read_structure_type): Move set_descriptive_type after
Jan> set_die_type.
Jan> (read_array_type): Remove type initialization. Recheck get_die_type
Jan> after initial die_type. Move set_die_type before set_descriptive_type.
Jan> (read_set_type): New variable domain_type. Recheck get_die_type after
Jan> initial die_type.
Jan> (read_tag_pointer_type, read_tag_reference_type): New variable
Jan> target_type. Recheck get_die_type after initial die_type.
Jan> (read_tag_ptr_to_member_type): Recheck get_die_type after initial
Jan> die_type and die_containing_type.
Jan> (read_tag_const_type, read_tag_volatile_type, read_subroutine_type):
Jan> Recheck get_die_type after initial die_type.
Jan> (read_subrange_type): Recheck get_die_type after initial die_type.
Jan> Move set_die_type before set_descriptive_type.
Jan> (set_die_type): Call internal_error if DIE has some type already set.
This patch is ok. I'm sorry for the delay in the review.
I did not look closely to see if this conflicts with Keith's
obstack-leak.patch. If so, it doesn't matter to me if that goes in and
then you back it out, or if Keith just doesn't commit it.
Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix duplicate types for single DIE
2010-06-04 22:47 ` Tom Tromey
@ 2010-06-05 14:55 ` Jan Kratochvil
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kratochvil @ 2010-06-05 14:55 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches, Keith Seitz
On Sat, 05 Jun 2010 00:47:35 +0200, Tom Tromey wrote:
> I don't have a problem with an internal_error if it can only trigger
> when there is a bug in GDB.
While it is only a GDB internal problem it has been changed in the meantime to
complaint() agreed (not sure if approved) by Joel.
Re: [patch] Fix duplicate types for single DIE
http://sourceware.org/ml/gdb-patches/2010-05/msg00477.html
> This patch is ok.
Checked-in.
> I did not look closely to see if this conflicts with Keith's
> obstack-leak.patch.
Keith's patchset is still not in so checking-in mine first.
Thanks,
Jan
http://sourceware.org/ml/gdb-cvs/2010-06/msg00052.html
--- src/gdb/ChangeLog 2010/06/05 05:55:59 1.11877
+++ src/gdb/ChangeLog 2010/06/05 14:11:53 1.11878
@@ -1,3 +1,23 @@
+2010-06-05 Jan Kratochvil <jan.kratochvil@redhat.com>
+
+ Fix duplicate types for single DIE.
+ * dwarf2read.c (read_structure_type): Move set_descriptive_type after
+ set_die_type.
+ (read_array_type): Remove type initialization. Recheck get_die_type
+ after initial die_type. Move set_die_type before set_descriptive_type.
+ (read_set_type): New variable domain_type. Recheck get_die_type after
+ initial die_type. Move attr initialization later.
+ (read_tag_pointer_type, read_tag_reference_type): New variable
+ target_type. Recheck get_die_type after initial die_type.
+ (read_tag_ptr_to_member_type): Recheck get_die_type after initial
+ die_type and die_containing_type.
+ (read_tag_const_type, read_tag_volatile_type, read_subroutine_type):
+ Recheck get_die_type after initial die_type.
+ (read_subrange_type): Recheck get_die_type after initial die_type.
+ Move set_die_type before set_descriptive_type.
+ (set_die_type): Extend the function comment. Call complaint if DIE has
+ some type already set.
+
2010-06-05 Vladimir Prus <vladimir@codesourcery.com>
* mi/mi-main.c (mi_cmd_list_thread_groups): Adjust
--- src/gdb/dwarf2read.c 2010/06/03 15:39:43 1.394
+++ src/gdb/dwarf2read.c 2010/06/05 14:11:54 1.395
@@ -5138,13 +5138,14 @@
on incomplete types. */
TYPE_STUB (type) = 1;
- set_descriptive_type (type, die, cu);
-
/* We need to add the type field to the die immediately so we don't
infinitely recurse when dealing with pointers to the structure
type within the structure itself. */
set_die_type (die, type, cu);
+ /* set_die_type should be already done. */
+ set_descriptive_type (type, die, cu);
+
if (die->child != NULL && ! die_is_declaration (die, cu))
{
struct field_info fi;
@@ -5433,7 +5434,7 @@
{
struct objfile *objfile = cu->objfile;
struct die_info *child_die;
- struct type *type = NULL;
+ struct type *type;
struct type *element_type, *range_type, *index_type;
struct type **range_types = NULL;
struct attribute *attr;
@@ -5443,6 +5444,11 @@
element_type = die_type (die, cu);
+ /* The die_type call above may have already set the type for this DIE. */
+ type = get_die_type (die, cu);
+ if (type)
+ return type;
+
/* Irix 6.2 native cc creates array types without children for
arrays with unspecified length. */
if (die->child == NULL)
@@ -5511,12 +5517,15 @@
if (name)
TYPE_NAME (type) = name;
+ /* Install the type in the die. */
+ set_die_type (die, type, cu);
+
+ /* set_die_type should be already done. */
set_descriptive_type (type, die, cu);
do_cleanups (back_to);
- /* Install the type in the die. */
- return set_die_type (die, type, cu);
+ return type;
}
static enum dwarf_array_dim_ordering
@@ -5559,11 +5568,22 @@
static struct type *
read_set_type (struct die_info *die, struct dwarf2_cu *cu)
{
- struct type *set_type = create_set_type (NULL, die_type (die, cu));
- struct attribute *attr = dwarf2_attr (die, DW_AT_byte_size, cu);
+ struct type *domain_type, *set_type;
+ struct attribute *attr;
+ domain_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ set_type = get_die_type (die, cu);
+ if (set_type)
+ return set_type;
+
+ set_type = create_set_type (NULL, domain_type);
+
+ attr = dwarf2_attr (die, DW_AT_byte_size, cu);
if (attr)
TYPE_LENGTH (set_type) = DW_UNSND (attr);
+
return set_die_type (die, set_type, cu);
}
@@ -5783,8 +5803,16 @@
struct attribute *attr_byte_size;
struct attribute *attr_address_class;
int byte_size, addr_class;
+ struct type *target_type;
+
+ target_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ type = get_die_type (die, cu);
+ if (type)
+ return type;
- type = lookup_pointer_type (die_type (die, cu));
+ type = lookup_pointer_type (target_type);
attr_byte_size = dwarf2_attr (die, DW_AT_byte_size, cu);
if (attr_byte_size)
@@ -5840,6 +5868,11 @@
to_type = die_type (die, cu);
domain = die_containing_type (die, cu);
+ /* The calls above may have already set the type for this DIE. */
+ type = get_die_type (die, cu);
+ if (type)
+ return type;
+
if (TYPE_CODE (check_typedef (to_type)) == TYPE_CODE_METHOD)
type = lookup_methodptr_type (to_type);
else
@@ -5855,10 +5888,17 @@
read_tag_reference_type (struct die_info *die, struct dwarf2_cu *cu)
{
struct comp_unit_head *cu_header = &cu->header;
- struct type *type;
+ struct type *type, *target_type;
struct attribute *attr;
- type = lookup_reference_type (die_type (die, cu));
+ target_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ type = get_die_type (die, cu);
+ if (type)
+ return type;
+
+ type = lookup_reference_type (target_type);
attr = dwarf2_attr (die, DW_AT_byte_size, cu);
if (attr)
{
@@ -5877,6 +5917,12 @@
struct type *base_type, *cv_type;
base_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ cv_type = get_die_type (die, cu);
+ if (cv_type)
+ return cv_type;
+
cv_type = make_cv_type (1, TYPE_VOLATILE (base_type), base_type, 0);
return set_die_type (die, cv_type, cu);
}
@@ -5887,6 +5933,12 @@
struct type *base_type, *cv_type;
base_type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ cv_type = get_die_type (die, cu);
+ if (cv_type)
+ return cv_type;
+
cv_type = make_cv_type (TYPE_CONST (base_type), 1, base_type, 0);
return set_die_type (die, cv_type, cu);
}
@@ -5951,6 +6003,12 @@
struct attribute *attr;
type = die_type (die, cu);
+
+ /* The die_type call above may have already set the type for this DIE. */
+ ftype = get_die_type (die, cu);
+ if (ftype)
+ return ftype;
+
ftype = lookup_function_type (type);
/* All functions in C++, Pascal and Java have prototypes. */
@@ -6164,6 +6222,11 @@
base_type = die_type (die, cu);
+ /* The die_type call above may have already set the type for this DIE. */
+ range_type = get_die_type (die, cu);
+ if (range_type)
+ return range_type;
+
if (cu->language == language_fortran)
{
/* FORTRAN implies a lower bound of 1, if not given. */
@@ -6270,9 +6333,12 @@
if (attr)
TYPE_LENGTH (range_type) = DW_UNSND (attr);
+ set_die_type (die, range_type, cu);
+
+ /* set_die_type should be already done. */
set_descriptive_type (range_type, die, cu);
- return set_die_type (die, range_type, cu);
+ return range_type;
}
static struct type *
@@ -12094,7 +12160,22 @@
}
/* Set the type associated with DIE to TYPE. Save it in CU's hash
- table if necessary. For convenience, return TYPE. */
+ table if necessary. For convenience, return TYPE.
+
+ The DIEs reading must have careful ordering to:
+ * Not cause infite loops trying to read in DIEs as a prerequisite for
+ reading current DIE.
+ * Not trying to dereference contents of still incompletely read in types
+ while reading in other DIEs.
+ * Enable referencing still incompletely read in types just by a pointer to
+ the type without accessing its fields.
+
+ Therefore caller should follow these rules:
+ * Try to fetch any prerequisite types we may need to build this DIE type
+ before building the type and calling set_die_type.
+ * After building typer call set_die_type for current DIE as soon as
+ possible before fetching more types to complete the current type.
+ * Make the type as complete as possible before fetching more types. */
static struct type *
set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
@@ -12132,6 +12213,10 @@
ofs.type = type;
slot = (struct dwarf2_offset_and_type **)
htab_find_slot_with_hash (cu->type_hash, &ofs, ofs.offset, INSERT);
+ if (*slot)
+ complaint (&symfile_complaints,
+ _("A problem internal to GDB: DIE 0x%x has type already set"),
+ die->offset);
*slot = obstack_alloc (&cu->objfile->objfile_obstack, sizeof (**slot));
**slot = ofs;
return type;
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-06-05 14:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-05-13 13:18 [patch] Fix duplicate types for single DIE Jan Kratochvil
2010-05-20 23:35 ` Joel Brobecker
2010-05-21 12:18 ` Jan Kratochvil
2010-05-21 15:09 ` Joel Brobecker
2010-06-04 22:47 ` Tom Tromey
2010-06-05 14:55 ` Jan Kratochvil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox