Message ID | 20111012135008.GA24037@host1.jankratochvil.net |
---|---|
State | New |
Headers | show |
On Oct 12, 2011, at 3:50 PM, Jan Kratochvil wrote: > Hi, > > dropping the optional DWARF attribute DW_AT_sibling has only advantages and no > disadvantages: > > For files with .gdb_index GDB initial scan does not use DW_AT_sibling at all. > For files without .gdb_index GDB initial scan has 1.79% time _improvement_. > For .debug files it brings 3.49% size decrease (7.84% for rpm compressed files). > > I guess DW_AT_sibling had real performance gains on CPUs with 1x (=no) clock > multipliers. Nowadays mostly only the data size transferred over FSB matters. > > I do not think there would be any DWARF consumers compatibility problems as > DW_AT_sibling has always been optional but I admit I have tested only GDB. I fear that this may degrade performance of other debuggers. What about adding a command line option ? Tristan.
On Wed, 12 Oct 2011 16:07:24 +0200, Tristan Gingold wrote: > I fear that this may degrade performance of other debuggers. What about > adding a command line option ? I can test idb, there aren't so many DWARF debuggers out there I think. If the default is removed DW_AT_sibling a new options may make sense as some compatibility safeguard. Thanks, Jan
On Wed, 12 Oct 2011 16:18:07 +0200, Jan Kratochvil wrote: > On Wed, 12 Oct 2011 16:07:24 +0200, Tristan Gingold wrote: > > I fear that this may degrade performance of other debuggers. What about > > adding a command line option ? > > I can test idb, I do not find the difference measurable. Dropping DW_AT_sibling is 0.25% performance _improvement_ but I guess it is just less than the measurement error. libstdc++ built with gcc -gdwarf-2 as with gcc -gdwarf-4 -fdebug-types-section it crashes. i7-920 x86_64 used for testing: Intel(R) Debugger for applications running on Intel(R) 64, Version 12.1, Build [76.472.14] with DW_AT_sibling real 2m34.206s 2m31.822s 2m31.709s 2m32.316s avg = 152.51325 seconds patched GCC without DW_AT_sibling real 2m32.528s 2m30.524s 2m33.767s 2m31.719s avg = 152.1345 seconds I do not see a point in keeping DW_AT_sibling there. Regards, Jan
On Oct 13, 2011, at 10:40 PM, Jan Kratochvil wrote: > On Wed, 12 Oct 2011 16:18:07 +0200, Jan Kratochvil wrote: >> On Wed, 12 Oct 2011 16:07:24 +0200, Tristan Gingold wrote: >>> I fear that this may degrade performance of other debuggers. What about >>> adding a command line option ? >> >> I can test idb, > > I do not find the difference measurable. Dropping DW_AT_sibling is 0.25% > performance _improvement_ but I guess it is just less than the measurement > error. > > libstdc++ built with gcc -gdwarf-2 as with gcc -gdwarf-4 -fdebug-types-section > it crashes. i7-920 x86_64 used for testing: > Intel(R) Debugger for applications running on Intel(R) 64, Version 12.1, Build [76.472.14] > > with DW_AT_sibling > real 2m34.206s 2m31.822s 2m31.709s 2m32.316s > avg = 152.51325 seconds > > patched GCC without DW_AT_sibling > real 2m32.528s 2m30.524s 2m33.767s 2m31.719s > avg = 152.1345 seconds > > I do not see a point in keeping DW_AT_sibling there. I am not against this patch, my only concern is that there are many many dwarf consumers and I have no idea how they will react to this change. Tristan.
>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes:
Tristan> I am not against this patch, my only concern is that there are many
Tristan> many dwarf consumers and I have no idea how they will react to this
Tristan> change.
I tend to think that this is the wrong standard to apply. In this case
we would be avoiding a beneficial change -- as measured in both
performance in a couple of cases, and in size -- for the sake of unknown
and possibly nonexistent consumers. I think instead the burden of proof
should be on those consumers, both to give their evidence and reasoning
and to engage with GCC.
Another way to look at it is that there have been many changes to GCC's
DWARF output in the last few years. Surely these have broken these
DWARF consumers more than this change possibly could.
Tom
On Oct 14, 2011, at 4:02 PM, Tom Tromey wrote: >>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes: > > Tristan> I am not against this patch, my only concern is that there are many > Tristan> many dwarf consumers and I have no idea how they will react to this > Tristan> change. > > I tend to think that this is the wrong standard to apply. In this case > we would be avoiding a beneficial change -- as measured in both > performance in a couple of cases, and in size -- I am not against this patch. I think it would be useful to add an option (-fdwarf-emit-sibling ?) to keep the old behavior. > for the sake of unknown > and possibly nonexistent consumers. I think instead the burden of proof > should be on those consumers, both to give their evidence and reasoning > and to engage with GCC. You know the story here: they don't use the latest gcc version and start to complain years later. > Another way to look at it is that there have been many changes to GCC's > DWARF output in the last few years. Surely these have broken these > DWARF consumers more than this change possibly could. Yes, but there is -gstrict-dwarf to stay compatible with old behavior. Tristan.
>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes:
Tom> Another way to look at it is that there have been many changes to GCC's
Tom> DWARF output in the last few years. Surely these have broken these
Tom> DWARF consumers more than this change possibly could.
Tristan> Yes, but there is -gstrict-dwarf to stay compatible with old behavior.
Yes, but this change is strictly compliant. What makes it different
from any other change that was made to make GCC more compliant?
Hypothetical consumers could also break on those changes.
Tom
On Oct 17, 2011, at 3:16 PM, Tom Tromey wrote: >>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes: > > Tom> Another way to look at it is that there have been many changes to GCC's > Tom> DWARF output in the last few years. Surely these have broken these > Tom> DWARF consumers more than this change possibly could. > > Tristan> Yes, but there is -gstrict-dwarf to stay compatible with old behavior. > > Yes, but this change is strictly compliant. Agreed. > What makes it different > from any other change that was made to make GCC more compliant? > Hypothetical consumers could also break on those changes. The others changes were often corner cases, while this one is very visible. What is wrong with my suggestion of adding a command line option to keep the siblings ? This option could be removed in a few years if nobody complained about sibling removal. Tristan.
--- a/gcc/dwarf2out.c +++ b/gcc/dwarf2out.c @@ -3316,7 +3316,6 @@ static int htab_cu_eq (const void *, const void *); static void htab_cu_del (void *); static int check_duplicate_cu (dw_die_ref, htab_t, unsigned *); static void record_comdat_symbol_number (dw_die_ref, htab_t, unsigned); -static void add_sibling_attributes (dw_die_ref); static void build_abbrev_table (dw_die_ref); static void output_location_lists (dw_die_ref); static int constant_size (unsigned HOST_WIDE_INT); @@ -7482,24 +7481,6 @@ copy_decls_for_unworthy_types (dw_die_ref unit) unmark_dies (unit); } -/* Traverse the DIE and add a sibling attribute if it may have the - effect of speeding up access to siblings. To save some space, - avoid generating sibling attributes for DIE's without children. */ - -static void -add_sibling_attributes (dw_die_ref die) -{ - dw_die_ref c; - - if (! die->die_child) - return; - - if (die->die_parent && die != die->die_parent->die_child) - add_AT_die_ref (die, DW_AT_sibling, die->die_sib); - - FOR_EACH_CHILD (die, c, add_sibling_attributes (c)); -} - /* Output all location lists for the DIE and its children. */ static void @@ -22496,14 +22477,6 @@ dwarf2out_finish (const char *filename) prune_unused_types (); } - /* Traverse the DIE's and add add sibling attributes to those DIE's - that have children. */ - add_sibling_attributes (comp_unit_die ()); - for (node = limbo_die_list; node; node = node->next) - add_sibling_attributes (node->die); - for (ctnode = comdat_type_list; ctnode != NULL; ctnode = ctnode->next) - add_sibling_attributes (ctnode->root_die); - /* Output a terminator label for the .text section. */ switch_to_section (text_section); targetm.asm_out.internal_label (asm_out_file, TEXT_END_LABEL, 0);