* [PATCH 0/7 v2] Class-fy partial_die_info @ 2018-02-22 15:36 Yao Qi 2018-02-22 15:36 ` [PATCH 3/7] Change find_partial_die_in_comp_unit to dwarf2_cu::find_partial_die Yao Qi ` (7 more replies) 0 siblings, 8 replies; 22+ messages in thread From: Yao Qi @ 2018-02-22 15:36 UTC (permalink / raw) To: gdb-patches When I fix some issues related to dwarf, I class-fy partial_die_info as part of the fix. The class-fication bits can go upstream first. This is the v2 of this patch series, v1 can be found https://sourceware.org/ml/gdb-patches/2018-01/msg00505.html Changes in v2 are, - Don't move the location of partial_die_info::fixup_called, as it may increase the memory usage, - Add some comments to read_partial_die, *** BLURB HERE *** Yao Qi (7): Re-write partial_die_info allocation in load_partial_dies Don't check abbrev is NULL in read_partial_die Change find_partial_die_in_comp_unit to dwarf2_cu::find_partial_die Class-fy partial_die_info Remove one argument abbrev_len in read_partial_die Move fixup_partial_die to partial_die_info::fixup Move read_partial_die to partial_die_info::read gdb/dwarf2read.c | 337 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 183 insertions(+), 154 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/7] Change find_partial_die_in_comp_unit to dwarf2_cu::find_partial_die 2018-02-22 15:36 [PATCH 0/7 v2] Class-fy partial_die_info Yao Qi @ 2018-02-22 15:36 ` Yao Qi 2018-02-22 15:36 ` [PATCH 1/7] Re-write partial_die_info allocation in load_partial_dies Yao Qi ` (6 subsequent siblings) 7 siblings, 0 replies; 22+ messages in thread From: Yao Qi @ 2018-02-22 15:36 UTC (permalink / raw) To: gdb-patches This patch changes find_partial_die_in_comp_unit to a dwarf2_cu method find_partial_die. gdb: 2018-01-11 Yao Qi <yao.qi@linaro.org> * dwarf2read.c (struct dwarf2_cu) <find_partial_die>: New method. (find_partial_die_in_comp_unit): Change it to dwarf2_cu::find_partial_die. (find_partial_die): Update. --- gdb/dwarf2read.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 147c7ca..f386873 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -785,6 +785,8 @@ struct dwarf2_cu this information, but later versions do. */ unsigned int processing_has_namespace_info : 1; + + struct partial_die_info *find_partial_die (sect_offset sect_off); }; /* Persistent data held for a compilation unit, even when not @@ -18701,15 +18703,15 @@ read_partial_die (const struct die_reader_specs *reader, /* Find a cached partial DIE at OFFSET in CU. */ -static struct partial_die_info * -find_partial_die_in_comp_unit (sect_offset sect_off, struct dwarf2_cu *cu) +struct partial_die_info * +dwarf2_cu::find_partial_die (sect_offset sect_off) { struct partial_die_info *lookup_die = NULL; struct partial_die_info part_die; part_die.sect_off = sect_off; lookup_die = ((struct partial_die_info *) - htab_find_with_hash (cu->partial_dies, &part_die, + htab_find_with_hash (partial_dies, &part_die, to_underlying (sect_off))); return lookup_die; @@ -18732,7 +18734,7 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu) if (offset_in_dwz == cu->per_cu->is_dwz && offset_in_cu_p (&cu->header, sect_off)) { - pd = find_partial_die_in_comp_unit (sect_off, cu); + pd = cu->find_partial_die (sect_off); if (pd != NULL) return pd; /* We missed recording what we needed. @@ -18756,7 +18758,7 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu) load_partial_comp_unit (per_cu); per_cu->cu->last_used = 0; - pd = find_partial_die_in_comp_unit (sect_off, per_cu->cu); + pd = per_cu->cu->find_partial_die (sect_off); } /* If we didn't find it, and not all dies have been loaded, @@ -18774,7 +18776,7 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu) set. */ load_partial_comp_unit (per_cu); - pd = find_partial_die_in_comp_unit (sect_off, per_cu->cu); + pd = per_cu->cu->find_partial_die (sect_off); } if (pd == NULL) -- 1.9.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/7] Re-write partial_die_info allocation in load_partial_dies 2018-02-22 15:36 [PATCH 0/7 v2] Class-fy partial_die_info Yao Qi 2018-02-22 15:36 ` [PATCH 3/7] Change find_partial_die_in_comp_unit to dwarf2_cu::find_partial_die Yao Qi @ 2018-02-22 15:36 ` Yao Qi 2018-02-22 15:36 ` [PATCH 5/7] Remove one argument abbrev_len in read_partial_die Yao Qi ` (5 subsequent siblings) 7 siblings, 0 replies; 22+ messages in thread From: Yao Qi @ 2018-02-22 15:36 UTC (permalink / raw) To: gdb-patches load_partial_dies has a "while (1)" loop to visit each die, and create partial_die_info if needed in each iteration, like this, part_die = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info); while (1) { if (foo1) continue; if (foo2) continue; read_partial_die (, , part_die, ,); .... part_die = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info); }; the code was written in a way that spaces are allocated on necessary on cu->comp_unit_obstack. I want to class-fy partial_die_info, but partial_die_info ctor can't follow XOBNEW immediately, so this patch rewrite this loop to: while (1) { if (foo1) continue; if (foo2) continue; struct partial_die_info pdi; read_partial_die (, , &pdi, ,); part_die = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info); memcpy (part_die, &pdi, sizeof (pdi)); }; we create a local variable pdi, if we need it, call XOBNEW, and copy. This also reduce one XOBNEW call. I measured the number of XOBNEW call in load_partial_dies when gdb reads dwarf2read.o, without this patch, it is 18827, and with this patch, it is 18826. gdb: 2018-01-18 Yao Qi <yao.qi@linaro.org> * dwarf2read.c (load_partial_dies): Move the location of XOBNEW. --- gdb/dwarf2read.c | 55 +++++++++++++++++++++++++------------------------------ 1 file changed, 25 insertions(+), 30 deletions(-) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 6cdb963..81b42cf 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -18216,7 +18216,6 @@ load_partial_dies (const struct die_reader_specs *reader, { struct dwarf2_cu *cu = reader->cu; struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile; - struct partial_die_info *part_die; struct partial_die_info *parent_die, *last_die, *first_die = NULL; unsigned int bytes_read; unsigned int load_all = 0; @@ -18238,8 +18237,6 @@ load_partial_dies (const struct die_reader_specs *reader, hashtab_obstack_allocate, dummy_obstack_deallocate); - part_die = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info); - while (1) { abbrev_info *abbrev = peek_die_abbrev (*reader, info_ptr, &bytes_read); @@ -18248,15 +18245,8 @@ load_partial_dies (const struct die_reader_specs *reader, if (abbrev == NULL) { if (--nesting_level == 0) - { - /* PART_DIE was probably the last thing allocated on the - comp_unit_obstack, so we could call obstack_free - here. We don't do that because the waste is small, - and will be cleaned up when we're done with this - compilation unit. This way, we're also more robust - against other users of the comp_unit_obstack. */ - return first_die; - } + return first_die; + info_ptr += bytes_read; last_die = parent_die; parent_die = parent_die->die_parent; @@ -18314,7 +18304,10 @@ load_partial_dies (const struct die_reader_specs *reader, continue; } - info_ptr = read_partial_die (reader, part_die, abbrev, bytes_read, + struct partial_die_info pdi; + + memset (&pdi, 0, sizeof (pdi)); + info_ptr = read_partial_die (reader, &pdi, abbrev, bytes_read, info_ptr); /* This two-pass algorithm for processing partial symbols has a @@ -18334,18 +18327,18 @@ load_partial_dies (const struct die_reader_specs *reader, of them, for a language without namespaces), can be processed directly. */ if (parent_die == NULL - && part_die->has_specification == 0 - && part_die->is_declaration == 0 - && ((part_die->tag == DW_TAG_typedef && !part_die->has_children) - || part_die->tag == DW_TAG_base_type - || part_die->tag == DW_TAG_subrange_type)) - { - if (building_psymtab && part_die->name != NULL) - add_psymbol_to_list (part_die->name, strlen (part_die->name), 0, + && pdi.has_specification == 0 + && pdi.is_declaration == 0 + && ((pdi.tag == DW_TAG_typedef && !pdi.has_children) + || pdi.tag == DW_TAG_base_type + || pdi.tag == DW_TAG_subrange_type)) + { + if (building_psymtab && pdi.name != NULL) + add_psymbol_to_list (pdi.name, strlen (pdi.name), 0, VAR_DOMAIN, LOC_TYPEDEF, &objfile->static_psymbols, 0, cu->language, objfile); - info_ptr = locate_pdi_sibling (reader, part_die, info_ptr); + info_ptr = locate_pdi_sibling (reader, &pdi, info_ptr); continue; } @@ -18357,37 +18350,41 @@ load_partial_dies (const struct die_reader_specs *reader, it could not find the child DIEs referenced later, this is checked above. In correct DWARF DW_TAG_typedef should have no children. */ - if (part_die->tag == DW_TAG_typedef && part_die->has_children) + if (pdi.tag == DW_TAG_typedef && pdi.has_children) complaint (&symfile_complaints, _("DW_TAG_typedef has childen - GCC PR debug/47510 bug " "- DIE at 0x%x [in module %s]"), - to_underlying (part_die->sect_off), objfile_name (objfile)); + to_underlying (pdi.sect_off), objfile_name (objfile)); /* If we're at the second level, and we're an enumerator, and our parent has no specification (meaning possibly lives in a namespace elsewhere), then we can add the partial symbol now instead of queueing it. */ - if (part_die->tag == DW_TAG_enumerator + if (pdi.tag == DW_TAG_enumerator && parent_die != NULL && parent_die->die_parent == NULL && parent_die->tag == DW_TAG_enumeration_type && parent_die->has_specification == 0) { - if (part_die->name == NULL) + if (pdi.name == NULL) complaint (&symfile_complaints, _("malformed enumerator DIE ignored")); else if (building_psymtab) - add_psymbol_to_list (part_die->name, strlen (part_die->name), 0, + add_psymbol_to_list (pdi.name, strlen (pdi.name), 0, VAR_DOMAIN, LOC_CONST, cu->language == language_cplus ? &objfile->global_psymbols : &objfile->static_psymbols, 0, cu->language, objfile); - info_ptr = locate_pdi_sibling (reader, part_die, info_ptr); + info_ptr = locate_pdi_sibling (reader, &pdi, info_ptr); continue; } + struct partial_die_info *part_die + = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info); + + memcpy (part_die, &pdi, sizeof (pdi)); /* We'll save this DIE so link it in. */ part_die->die_parent = parent_die; part_die->die_sibling = NULL; @@ -18439,8 +18436,6 @@ load_partial_dies (const struct die_reader_specs *reader, *slot = part_die; } - part_die = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info); - /* For some DIEs we want to follow their children (if any). For C we have no reason to follow the children of structures; for other languages we have to, so that we can get at method physnames -- 1.9.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 5/7] Remove one argument abbrev_len in read_partial_die 2018-02-22 15:36 [PATCH 0/7 v2] Class-fy partial_die_info Yao Qi 2018-02-22 15:36 ` [PATCH 3/7] Change find_partial_die_in_comp_unit to dwarf2_cu::find_partial_die Yao Qi 2018-02-22 15:36 ` [PATCH 1/7] Re-write partial_die_info allocation in load_partial_dies Yao Qi @ 2018-02-22 15:36 ` Yao Qi 2018-02-22 15:36 ` [PATCH 4/7] Class-fy partial_die_info Yao Qi ` (4 subsequent siblings) 7 siblings, 0 replies; 22+ messages in thread From: Yao Qi @ 2018-02-22 15:36 UTC (permalink / raw) To: gdb-patches gdb: 2018-01-11 Yao Qi <yao.qi@linaro.org> * dwarf2read.c (read_partial_die): Update the declaration. (load_partial_dies): Caller update. (read_partial_die): Remove one argument abbrev_len. --- gdb/dwarf2read.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index b01d9f3..333a890 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -1834,7 +1834,6 @@ static struct partial_die_info *load_partial_dies static const gdb_byte *read_partial_die (const struct die_reader_specs *, struct partial_die_info *, struct abbrev_info *, - unsigned int, const gdb_byte *); static struct partial_die_info *find_partial_die (sect_offset, int, @@ -18348,8 +18347,8 @@ load_partial_dies (const struct die_reader_specs *reader, struct partial_die_info pdi ((sect_offset) (info_ptr - reader->buffer), abbrev); - info_ptr = read_partial_die (reader, &pdi, abbrev, bytes_read, - info_ptr); + info_ptr = read_partial_die (reader, &pdi, abbrev, + (const gdb_byte *) info_ptr + bytes_read); /* This two-pass algorithm for processing partial symbols has a high cost in cache pressure. Thus, handle some simple cases @@ -18524,13 +18523,13 @@ partial_die_info::partial_die_info (sect_offset sect_off_, { } -/* Read a minimal amount of information into the minimal die structure. */ +/* Read a minimal amount of information into the minimal die structure. + INFO_PTR should point just after the initial uleb128 of a DIE. */ static const gdb_byte * read_partial_die (const struct die_reader_specs *reader, struct partial_die_info *part_die, - struct abbrev_info *abbrev, unsigned int abbrev_len, - const gdb_byte *info_ptr) + struct abbrev_info *abbrev, const gdb_byte *info_ptr) { struct dwarf2_cu *cu = reader->cu; struct dwarf2_per_objfile *dwarf2_per_objfile @@ -18543,8 +18542,6 @@ read_partial_die (const struct die_reader_specs *reader, int has_high_pc_attr = 0; int high_pc_relative = 0; - info_ptr += abbrev_len; - for (i = 0; i < abbrev->num_attrs; ++i) { info_ptr = read_attribute (reader, &attr, &abbrev->attrs[i], info_ptr); -- 1.9.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/7] Class-fy partial_die_info 2018-02-22 15:36 [PATCH 0/7 v2] Class-fy partial_die_info Yao Qi ` (2 preceding siblings ...) 2018-02-22 15:36 ` [PATCH 5/7] Remove one argument abbrev_len in read_partial_die Yao Qi @ 2018-02-22 15:36 ` Yao Qi 2018-02-22 15:36 ` [PATCH 7/7] Move read_partial_die to partial_die_info::read Yao Qi ` (3 subsequent siblings) 7 siblings, 0 replies; 22+ messages in thread From: Yao Qi @ 2018-02-22 15:36 UTC (permalink / raw) To: gdb-patches This patch is to class-fy partial_die_info. Two things special here, - disable assignment operator, but keep copy ctor, which is used in load_partial_dies, - have a private ctor which is only used by dwarf2_cu::find_partial_die, I don't want other code use it, so make it private, gdb: 2018-01-11 Yao Qi <yao.qi@linaro.org> * dwarf2read.c (struct partial_die_info): Add ctor, delete assignment operator. (load_partial_dies): Use ctor and copy ctor. (read_partial_die): Update. (dwarf2_cu::find_partial_die): Use ctor. --- gdb/dwarf2read.c | 88 +++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 62 insertions(+), 26 deletions(-) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index f386873..b01d9f3 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -1402,16 +1402,23 @@ file_entry::include_dir (const line_header *lh) const /* When we construct a partial symbol table entry we only need this much information. */ -struct partial_die_info +struct partial_die_info : public allocate_on_obstack { + partial_die_info (sect_offset sect_off, struct abbrev_info *abbrev); + + /* Disable assign but still keep copy ctor, which is needed + load_partial_dies. */ + partial_die_info& operator=(const partial_die_info& rhs) = delete; + /* Offset of this DIE. */ - sect_offset sect_off; + const sect_offset sect_off; /* DWARF-2 tag for this DIE. */ - ENUM_BITFIELD(dwarf_tag) tag : 16; + const ENUM_BITFIELD(dwarf_tag) tag : 16; /* Assorted flags describing the data found in this DIE. */ - unsigned int has_children : 1; + const unsigned int has_children : 1; + unsigned int is_external : 1; unsigned int is_declaration : 1; unsigned int has_type : 1; @@ -1446,15 +1453,15 @@ struct partial_die_info /* The name of this DIE. Normally the value of DW_AT_name, but sometimes a default name for unnamed DIEs. */ - const char *name; + const char *name = nullptr; /* The linkage name, if present. */ - const char *linkage_name; + const char *linkage_name = nullptr; /* The scope to prepend to our children. This is generally allocated on the comp_unit_obstack, so will disappear when this compilation unit leaves the cache. */ - const char *scope; + const char *scope = nullptr; /* Some data associated with the partial DIE. The tag determines which field is live. */ @@ -1464,26 +1471,58 @@ struct partial_die_info struct dwarf_block *locdesc; /* The offset of an import, for DW_TAG_imported_unit. */ sect_offset sect_off; - } d; + } d {}; /* If HAS_PC_INFO, the PC range associated with this DIE. */ - CORE_ADDR lowpc; - CORE_ADDR highpc; + CORE_ADDR lowpc = 0; + CORE_ADDR highpc = 0; /* Pointer into the info_buffer (or types_buffer) pointing at the target of DW_AT_sibling, if any. */ /* NOTE: This member isn't strictly necessary, read_partial_die could return DW_AT_sibling values to its caller load_partial_dies. */ - const gdb_byte *sibling; + const gdb_byte *sibling = nullptr; /* If HAS_SPECIFICATION, the offset of the DIE referred to by DW_AT_specification (or DW_AT_abstract_origin or DW_AT_extension). */ - sect_offset spec_offset; + sect_offset spec_offset {}; /* Pointers to this DIE's parent, first child, and next sibling, if any. */ - struct partial_die_info *die_parent, *die_child, *die_sibling; + struct partial_die_info *die_parent = nullptr; + struct partial_die_info *die_child = nullptr; + struct partial_die_info *die_sibling = nullptr; + + friend struct partial_die_info * + dwarf2_cu::find_partial_die (sect_offset sect_off); + + private: + /* Only need to do look up in dwarf2_cu::find_partial_die. */ + partial_die_info (sect_offset sect_off) + : partial_die_info (sect_off, DW_TAG_padding, 0) + { + } + + partial_die_info (sect_offset sect_off_, enum dwarf_tag tag_, + int has_children_) + : sect_off (sect_off_), tag (tag_), has_children (has_children_) + { + is_external = 0; + is_declaration = 0; + has_type = 0; + has_specification = 0; + has_pc_info = 0; + may_be_inlined = 0; + main_subprogram = 0; + scope_set = 0; + has_byte_size = 0; + has_const_value = 0; + has_template_arguments = 0; + fixup_called = 0; + is_dwz = 0; + spec_is_dwz = 0; + } }; /* This data structure holds the information of an abbrev. */ @@ -18306,9 +18345,9 @@ load_partial_dies (const struct die_reader_specs *reader, continue; } - struct partial_die_info pdi; + struct partial_die_info pdi ((sect_offset) (info_ptr - reader->buffer), + abbrev); - memset (&pdi, 0, sizeof (pdi)); info_ptr = read_partial_die (reader, &pdi, abbrev, bytes_read, info_ptr); @@ -18384,9 +18423,8 @@ load_partial_dies (const struct die_reader_specs *reader, } struct partial_die_info *part_die - = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info); + = new (&cu->comp_unit_obstack) partial_die_info (pdi); - memcpy (part_die, &pdi, sizeof (pdi)); /* We'll save this DIE so link it in. */ part_die->die_parent = parent_die; part_die->die_sibling = NULL; @@ -18480,6 +18518,12 @@ load_partial_dies (const struct die_reader_specs *reader, } } +partial_die_info::partial_die_info (sect_offset sect_off_, + struct abbrev_info *abbrev) + : partial_die_info (sect_off_, abbrev->tag, abbrev->has_children) +{ +} + /* Read a minimal amount of information into the minimal die structure. */ static const gdb_byte * @@ -18499,15 +18543,8 @@ read_partial_die (const struct die_reader_specs *reader, int has_high_pc_attr = 0; int high_pc_relative = 0; - memset (part_die, 0, sizeof (struct partial_die_info)); - - part_die->sect_off = (sect_offset) (info_ptr - buffer); - info_ptr += abbrev_len; - part_die->tag = abbrev->tag; - part_die->has_children = abbrev->has_children; - for (i = 0; i < abbrev->num_attrs; ++i) { info_ptr = read_attribute (reader, &attr, &abbrev->attrs[i], info_ptr); @@ -18707,9 +18744,8 @@ struct partial_die_info * dwarf2_cu::find_partial_die (sect_offset sect_off) { struct partial_die_info *lookup_die = NULL; - struct partial_die_info part_die; + struct partial_die_info part_die (sect_off); - part_die.sect_off = sect_off; lookup_die = ((struct partial_die_info *) htab_find_with_hash (partial_dies, &part_die, to_underlying (sect_off))); -- 1.9.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 7/7] Move read_partial_die to partial_die_info::read 2018-02-22 15:36 [PATCH 0/7 v2] Class-fy partial_die_info Yao Qi ` (3 preceding siblings ...) 2018-02-22 15:36 ` [PATCH 4/7] Class-fy partial_die_info Yao Qi @ 2018-02-22 15:36 ` Yao Qi 2018-02-22 15:36 ` [PATCH 2/7] Don't check abbrev is NULL in read_partial_die Yao Qi ` (2 subsequent siblings) 7 siblings, 0 replies; 22+ messages in thread From: Yao Qi @ 2018-02-22 15:36 UTC (permalink / raw) To: gdb-patches gdb: 2018-01-11 Yao Qi <yao.qi@linaro.org> * dwarf2read.c (struct partial_die_info) <read>: New method. (read_partial_die): Remove the declaration. (load_partial_dies): Update. (partial_die_info::partial_die_info): (read_partial_die): Change it to partial_die_info::read. --- gdb/dwarf2read.c | 107 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 56 insertions(+), 51 deletions(-) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 7823c3b..d9eb648 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -1415,6 +1415,12 @@ struct partial_die_info : public allocate_on_obstack name. */ void fixup (struct dwarf2_cu *cu); + /* Read a minimal amount of information into the minimal die + structure. */ + const gdb_byte *read (const struct die_reader_specs *reader, + struct abbrev_info *abbrev, + const gdb_byte *info_ptr); + /* Offset of this DIE. */ const sect_offset sect_off; @@ -1484,8 +1490,8 @@ struct partial_die_info : public allocate_on_obstack /* Pointer into the info_buffer (or types_buffer) pointing at the target of DW_AT_sibling, if any. */ - /* NOTE: This member isn't strictly necessary, read_partial_die could - return DW_AT_sibling values to its caller load_partial_dies. */ + /* NOTE: This member isn't strictly necessary, partial_die_info::read + could return DW_AT_sibling values to its caller load_partial_dies. */ const gdb_byte *sibling = nullptr; /* If HAS_SPECIFICATION, the offset of the DIE referred to by @@ -1836,11 +1842,6 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *); static struct partial_die_info *load_partial_dies (const struct die_reader_specs *, const gdb_byte *, int); -static const gdb_byte *read_partial_die (const struct die_reader_specs *, - struct partial_die_info *, - struct abbrev_info *, - const gdb_byte *); - static struct partial_die_info *find_partial_die (sect_offset, int, struct dwarf2_cu *); @@ -14837,7 +14838,7 @@ dwarf2_get_pc_bounds (struct die_info *die, CORE_ADDR *lowpc, return PC_BOUNDS_NOT_PRESENT; } - /* read_partial_die has also the strict LOW < HIGH requirement. */ + /* partial_die_info::read has also the strict LOW < HIGH requirement. */ if (high <= low) return PC_BOUNDS_INVALID; @@ -18349,8 +18350,7 @@ load_partial_dies (const struct die_reader_specs *reader, struct partial_die_info pdi ((sect_offset) (info_ptr - reader->buffer), abbrev); - info_ptr = read_partial_die (reader, &pdi, abbrev, - (const gdb_byte *) info_ptr + bytes_read); + info_ptr = pdi.read (reader, abbrev, info_ptr + bytes_read); /* This two-pass algorithm for processing partial symbols has a high cost in cache pressure. Thus, handle some simple cases @@ -18528,24 +18528,22 @@ partial_die_info::partial_die_info (sect_offset sect_off_, /* Read a minimal amount of information into the minimal die structure. INFO_PTR should point just after the initial uleb128 of a DIE. */ -static const gdb_byte * -read_partial_die (const struct die_reader_specs *reader, - struct partial_die_info *part_die, - struct abbrev_info *abbrev, const gdb_byte *info_ptr) +const gdb_byte * +partial_die_info::read (const struct die_reader_specs *reader, + struct abbrev_info *abbrev, const gdb_byte *info_ptr) { struct dwarf2_cu *cu = reader->cu; struct dwarf2_per_objfile *dwarf2_per_objfile = cu->per_cu->dwarf2_per_objfile; - struct objfile *objfile = dwarf2_per_objfile->objfile; - const gdb_byte *buffer = reader->buffer; unsigned int i; - struct attribute attr; int has_low_pc_attr = 0; int has_high_pc_attr = 0; int high_pc_relative = 0; for (i = 0; i < abbrev->num_attrs; ++i) { + struct attribute attr; + info_ptr = read_attribute (reader, &attr, &abbrev->attrs[i], info_ptr); /* Store the data if it is of an attribute we want to keep in a @@ -18553,7 +18551,7 @@ read_partial_die (const struct die_reader_specs *reader, switch (attr.name) { case DW_AT_name: - switch (part_die->tag) + switch (tag) { case DW_TAG_compile_unit: case DW_TAG_partial_unit: @@ -18564,12 +18562,16 @@ read_partial_die (const struct die_reader_specs *reader, case DW_TAG_enumerator: /* These tags always have simple identifiers already; no need to canonicalize them. */ - part_die->name = DW_STRING (&attr); + name = DW_STRING (&attr); break; default: - part_die->name - = dwarf2_canonicalize_name (DW_STRING (&attr), cu, - &objfile->per_bfd->storage_obstack); + { + struct objfile *objfile = dwarf2_per_objfile->objfile; + + name + = dwarf2_canonicalize_name (DW_STRING (&attr), cu, + &objfile->per_bfd->storage_obstack); + } break; } break; @@ -18579,16 +18581,16 @@ read_partial_die (const struct die_reader_specs *reader, assume they will be the same, and we only store the last one we see. */ if (cu->language == language_ada) - part_die->name = DW_STRING (&attr); - part_die->linkage_name = DW_STRING (&attr); + name = DW_STRING (&attr); + linkage_name = DW_STRING (&attr); break; case DW_AT_low_pc: has_low_pc_attr = 1; - part_die->lowpc = attr_value_as_address (&attr); + lowpc = attr_value_as_address (&attr); break; case DW_AT_high_pc: has_high_pc_attr = 1; - part_die->highpc = attr_value_as_address (&attr); + highpc = attr_value_as_address (&attr); if (cu->header.version >= 4 && attr_form_is_constant (&attr)) high_pc_relative = 1; break; @@ -18596,7 +18598,7 @@ read_partial_die (const struct die_reader_specs *reader, /* Support the .debug_loc offsets. */ if (attr_form_is_block (&attr)) { - part_die->d.locdesc = DW_BLOCK (&attr); + d.locdesc = DW_BLOCK (&attr); } else if (attr_form_is_section_offset (&attr)) { @@ -18609,20 +18611,20 @@ read_partial_die (const struct die_reader_specs *reader, } break; case DW_AT_external: - part_die->is_external = DW_UNSND (&attr); + is_external = DW_UNSND (&attr); break; case DW_AT_declaration: - part_die->is_declaration = DW_UNSND (&attr); + is_declaration = DW_UNSND (&attr); break; case DW_AT_type: - part_die->has_type = 1; + has_type = 1; break; case DW_AT_abstract_origin: case DW_AT_specification: case DW_AT_extension: - part_die->has_specification = 1; - part_die->spec_offset = dwarf2_get_ref_die_offset (&attr); - part_die->spec_is_dwz = (attr.form == DW_FORM_GNU_ref_alt + has_specification = 1; + spec_offset = dwarf2_get_ref_die_offset (&attr); + spec_is_dwz = (attr.form == DW_FORM_GNU_ref_alt || cu->per_cu->is_dwz); break; case DW_AT_sibling: @@ -18633,6 +18635,7 @@ read_partial_die (const struct die_reader_specs *reader, _("ignoring absolute DW_AT_sibling")); else { + const gdb_byte *buffer = reader->buffer; sect_offset off = dwarf2_get_ref_die_offset (&attr); const gdb_byte *sibling_ptr = buffer + to_underlying (off); @@ -18642,14 +18645,14 @@ read_partial_die (const struct die_reader_specs *reader, else if (sibling_ptr > reader->buffer_end) dwarf2_section_buffer_overflow_complaint (reader->die_section); else - part_die->sibling = sibling_ptr; + sibling = sibling_ptr; } break; case DW_AT_byte_size: - part_die->has_byte_size = 1; + has_byte_size = 1; break; case DW_AT_const_value: - part_die->has_const_value = 1; + has_const_value = 1; break; case DW_AT_calling_convention: /* DWARF doesn't provide a way to identify a program's source-level @@ -18668,25 +18671,25 @@ read_partial_die (const struct die_reader_specs *reader, compatibility. */ if (DW_UNSND (&attr) == DW_CC_program && cu->language == language_fortran) - part_die->main_subprogram = 1; + main_subprogram = 1; break; case DW_AT_inline: if (DW_UNSND (&attr) == DW_INL_inlined || DW_UNSND (&attr) == DW_INL_declared_inlined) - part_die->may_be_inlined = 1; + may_be_inlined = 1; break; case DW_AT_import: - if (part_die->tag == DW_TAG_imported_unit) + if (tag == DW_TAG_imported_unit) { - part_die->d.sect_off = dwarf2_get_ref_die_offset (&attr); - part_die->is_dwz = (attr.form == DW_FORM_GNU_ref_alt + d.sect_off = dwarf2_get_ref_die_offset (&attr); + is_dwz = (attr.form == DW_FORM_GNU_ref_alt || cu->per_cu->is_dwz); } break; case DW_AT_main_subprogram: - part_die->main_subprogram = DW_UNSND (&attr); + main_subprogram = DW_UNSND (&attr); break; default: @@ -18695,7 +18698,7 @@ read_partial_die (const struct die_reader_specs *reader, } if (high_pc_relative) - part_die->highpc += part_die->lowpc; + highpc += lowpc; if (has_low_pc_attr && has_high_pc_attr) { @@ -18707,31 +18710,33 @@ read_partial_die (const struct die_reader_specs *reader, 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) + if (lowpc == 0 && !dwarf2_per_objfile->has_section_at_zero) { + struct objfile *objfile = dwarf2_per_objfile->objfile; struct gdbarch *gdbarch = get_objfile_arch (objfile); complaint (&symfile_complaints, _("DW_AT_low_pc %s is zero " "for DIE at 0x%x [in module %s]"), - paddress (gdbarch, part_die->lowpc), - to_underlying (part_die->sect_off), objfile_name (objfile)); + paddress (gdbarch, lowpc), + to_underlying (sect_off), objfile_name (objfile)); } /* dwarf2_get_pc_bounds has also the strict low < high requirement. */ - else if (part_die->lowpc >= part_die->highpc) + else if (lowpc >= highpc) { + struct objfile *objfile = dwarf2_per_objfile->objfile; struct gdbarch *gdbarch = get_objfile_arch (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), - to_underlying (part_die->sect_off), + paddress (gdbarch, lowpc), + paddress (gdbarch, highpc), + to_underlying (sect_off), objfile_name (objfile)); } else - part_die->has_pc_info = 1; + has_pc_info = 1; } return info_ptr; -- 1.9.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/7] Don't check abbrev is NULL in read_partial_die 2018-02-22 15:36 [PATCH 0/7 v2] Class-fy partial_die_info Yao Qi ` (4 preceding siblings ...) 2018-02-22 15:36 ` [PATCH 7/7] Move read_partial_die to partial_die_info::read Yao Qi @ 2018-02-22 15:36 ` Yao Qi 2018-02-25 23:33 ` Simon Marchi 2018-02-22 15:36 ` [PATCH 6/7] Move fixup_partial_die to partial_die_info::fixup Yao Qi 2018-02-25 23:52 ` [PATCH 0/7 v2] Class-fy partial_die_info Simon Marchi 7 siblings, 1 reply; 22+ messages in thread From: Yao Qi @ 2018-02-22 15:36 UTC (permalink / raw) To: gdb-patches 'abbrev' won't be NULL, so don't check it. gdb: 2018-01-11 Yao Qi <yao.qi@linaro.org> * dwarf2read.c (read_partial_die): Remove the code checking abbrev is NULL. --- gdb/dwarf2read.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 81b42cf..147c7ca 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -18503,9 +18503,6 @@ read_partial_die (const struct die_reader_specs *reader, info_ptr += abbrev_len; - if (abbrev == NULL) - return info_ptr; - part_die->tag = abbrev->tag; part_die->has_children = abbrev->has_children; -- 1.9.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/7] Don't check abbrev is NULL in read_partial_die 2018-02-22 15:36 ` [PATCH 2/7] Don't check abbrev is NULL in read_partial_die Yao Qi @ 2018-02-25 23:33 ` Simon Marchi 0 siblings, 0 replies; 22+ messages in thread From: Simon Marchi @ 2018-02-25 23:33 UTC (permalink / raw) To: Yao Qi, gdb-patches On 2018-02-22 10:36 AM, Yao Qi wrote: > 'abbrev' won't be NULL, so don't check it. > > gdb: I would suggest changing abbrev type to a const reference then. Simon ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 6/7] Move fixup_partial_die to partial_die_info::fixup 2018-02-22 15:36 [PATCH 0/7 v2] Class-fy partial_die_info Yao Qi ` (5 preceding siblings ...) 2018-02-22 15:36 ` [PATCH 2/7] Don't check abbrev is NULL in read_partial_die Yao Qi @ 2018-02-22 15:36 ` Yao Qi 2018-02-25 23:52 ` [PATCH 0/7 v2] Class-fy partial_die_info Simon Marchi 7 siblings, 0 replies; 22+ messages in thread From: Yao Qi @ 2018-02-22 15:36 UTC (permalink / raw) To: gdb-patches fixup_partial_die can be a partial_die_info method fixup. gdb: 2018-01-11 Yao Qi <yao.qi@linaro.org> * dwarf2read.c (struct partial_die_info) <fixup>: New method. (fixup_partial_die): Remove declaration. (scan_partial_symbols): Update. (partial_die_parent_scope): Likewise. (partial_die_full_name): Likewise. (fixup_partial_die): Change it to partial_die_info::fixup. --- gdb/dwarf2read.c | 73 +++++++++++++++++++++++++++----------------------------- 1 file changed, 35 insertions(+), 38 deletions(-) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 333a890..7823c3b 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -1410,6 +1410,11 @@ struct partial_die_info : public allocate_on_obstack load_partial_dies. */ partial_die_info& operator=(const partial_die_info& rhs) = delete; + /* Adjust the partial die before generating a symbol for it. This + function may set the is_external flag or change the DIE's + name. */ + void fixup (struct dwarf2_cu *cu); + /* Offset of this DIE. */ const sect_offset sect_off; @@ -1442,7 +1447,7 @@ struct partial_die_info : public allocate_on_obstack /* Flag set if any of the DIE's children are template arguments. */ unsigned int has_template_arguments : 1; - /* Flag set if fixup_partial_die has been called on this die. */ + /* Flag set if fixup has been called on this die. */ unsigned int fixup_called : 1; /* Flag set if DW_TAG_imported_unit uses DW_FORM_GNU_ref_alt. */ @@ -1839,9 +1844,6 @@ static const gdb_byte *read_partial_die (const struct die_reader_specs *, static struct partial_die_info *find_partial_die (sect_offset, int, struct dwarf2_cu *); -static void fixup_partial_die (struct partial_die_info *, - struct dwarf2_cu *); - static const gdb_byte *read_attribute (const struct die_reader_specs *, struct attribute *, struct attr_abbrev *, const gdb_byte *); @@ -9082,7 +9084,7 @@ scan_partial_symbols (struct partial_die_info *first_die, CORE_ADDR *lowpc, while (pdi != NULL) { - fixup_partial_die (pdi, cu); + pdi->fixup (cu); /* Anonymous namespaces or modules have no name but have interesting children, so we need to look at them. Ditto for anonymous @@ -9218,7 +9220,7 @@ partial_die_parent_scope (struct partial_die_info *pdi, if (parent->scope_set) return parent->scope; - fixup_partial_die (parent, cu); + parent->fixup (cu); grandparent_scope = partial_die_parent_scope (parent, cu); @@ -9283,7 +9285,7 @@ partial_die_full_name (struct partial_die_info *pdi, types here will be reused if full symbols are loaded later. */ if (pdi->has_template_arguments) { - fixup_partial_die (pdi, cu); + pdi->fixup (cu); if (pdi->name != NULL && strchr (pdi->name, '<') == NULL) { @@ -9592,7 +9594,7 @@ add_partial_subprogram (struct partial_die_info *pdi, pdi = pdi->die_child; while (pdi != NULL) { - fixup_partial_die (pdi, cu); + pdi->fixup (cu); if (pdi->tag == DW_TAG_subprogram || pdi->tag == DW_TAG_inlined_subroutine || pdi->tag == DW_TAG_lexical_block) @@ -18874,45 +18876,40 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi, } } -/* Adjust PART_DIE before generating a symbol for it. This function - may set the is_external flag or change the DIE's name. */ - -static void -fixup_partial_die (struct partial_die_info *part_die, - struct dwarf2_cu *cu) +void +partial_die_info::fixup (struct dwarf2_cu *cu) { /* Once we've fixed up a die, there's no point in doing so again. This also avoids a memory leak if we were to call guess_partial_die_structure_name multiple times. */ - if (part_die->fixup_called) + if (fixup_called) return; /* If we found a reference attribute and the DIE has no name, try to find a name in the referred to DIE. */ - if (part_die->name == NULL && part_die->has_specification) + if (name == NULL && has_specification) { struct partial_die_info *spec_die; - spec_die = find_partial_die (part_die->spec_offset, - part_die->spec_is_dwz, cu); + spec_die = find_partial_die (spec_offset, spec_is_dwz, cu); - fixup_partial_die (spec_die, cu); + spec_die->fixup (cu); if (spec_die->name) { - part_die->name = spec_die->name; + name = spec_die->name; /* Copy DW_AT_external attribute if it is set. */ if (spec_die->is_external) - part_die->is_external = spec_die->is_external; + is_external = spec_die->is_external; } } /* Set default names for some unnamed DIEs. */ - if (part_die->name == NULL && part_die->tag == DW_TAG_namespace) - part_die->name = CP_ANONYMOUS_NAMESPACE_STR; + if (name == NULL && tag == DW_TAG_namespace) + name = CP_ANONYMOUS_NAMESPACE_STR; /* If there is no parent die to provide a namespace, and there are children, see if we can determine the namespace from their linkage @@ -18920,25 +18917,25 @@ fixup_partial_die (struct partial_die_info *part_die, if (cu->language == language_cplus && !VEC_empty (dwarf2_section_info_def, cu->per_cu->dwarf2_per_objfile->types) - && part_die->die_parent == NULL - && part_die->has_children - && (part_die->tag == DW_TAG_class_type - || part_die->tag == DW_TAG_structure_type - || part_die->tag == DW_TAG_union_type)) - guess_partial_die_structure_name (part_die, cu); + && die_parent == NULL + && has_children + && (tag == DW_TAG_class_type + || tag == DW_TAG_structure_type + || tag == DW_TAG_union_type)) + guess_partial_die_structure_name (this, cu); /* GCC might emit a nameless struct or union that has a linkage name. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47510. */ - if (part_die->name == NULL - && (part_die->tag == DW_TAG_class_type - || part_die->tag == DW_TAG_interface_type - || part_die->tag == DW_TAG_structure_type - || part_die->tag == DW_TAG_union_type) - && part_die->linkage_name != NULL) + if (name == NULL + && (tag == DW_TAG_class_type + || tag == DW_TAG_interface_type + || tag == DW_TAG_structure_type + || tag == DW_TAG_union_type) + && linkage_name != NULL) { char *demangled; - demangled = gdb_demangle (part_die->linkage_name, DMGL_TYPES); + demangled = gdb_demangle (linkage_name, DMGL_TYPES); if (demangled) { const char *base; @@ -18952,7 +18949,7 @@ fixup_partial_die (struct partial_die_info *part_die, base = demangled; struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile; - part_die->name + name = ((const char *) obstack_copy0 (&objfile->per_bfd->storage_obstack, base, strlen (base))); @@ -18960,7 +18957,7 @@ fixup_partial_die (struct partial_die_info *part_die, } } - part_die->fixup_called = 1; + fixup_called = 1; } /* Read an attribute value described by an attribute form. */ -- 1.9.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/7 v2] Class-fy partial_die_info 2018-02-22 15:36 [PATCH 0/7 v2] Class-fy partial_die_info Yao Qi ` (6 preceding siblings ...) 2018-02-22 15:36 ` [PATCH 6/7] Move fixup_partial_die to partial_die_info::fixup Yao Qi @ 2018-02-25 23:52 ` Simon Marchi 2018-02-26 15:39 ` Yao Qi 7 siblings, 1 reply; 22+ messages in thread From: Simon Marchi @ 2018-02-25 23:52 UTC (permalink / raw) To: Yao Qi, gdb-patches On 2018-02-22 10:36 AM, Yao Qi wrote: > When I fix some issues related to dwarf, I class-fy partial_die_info as > part of the fix. The class-fication bits can go upstream first. > > This is the v2 of this patch series, v1 can be found > https://sourceware.org/ml/gdb-patches/2018-01/msg00505.html Changes > in v2 are, > > - Don't move the location of partial_die_info::fixup_called, as it may > increase the memory usage, > - Add some comments to read_partial_die, > > *** BLURB HERE *** > > Yao Qi (7): > Re-write partial_die_info allocation in load_partial_dies > Don't check abbrev is NULL in read_partial_die > Change find_partial_die_in_comp_unit to dwarf2_cu::find_partial_die > Class-fy partial_die_info > Remove one argument abbrev_len in read_partial_die > Move fixup_partial_die to partial_die_info::fixup > Move read_partial_die to partial_die_info::read > > gdb/dwarf2read.c | 337 ++++++++++++++++++++++++++++++------------------------- > 1 file changed, 183 insertions(+), 154 deletions(-) > I went through the series again, I sent a comment on patch 2, otherwise LGTM. Simon ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/7 v2] Class-fy partial_die_info 2018-02-25 23:52 ` [PATCH 0/7 v2] Class-fy partial_die_info Simon Marchi @ 2018-02-26 15:39 ` Yao Qi 0 siblings, 0 replies; 22+ messages in thread From: Yao Qi @ 2018-02-26 15:39 UTC (permalink / raw) To: Simon Marchi; +Cc: GDB Patches On Sun, Feb 25, 2018 at 11:52 PM, Simon Marchi <simark@simark.ca> wrote: > > I went through the series again, I sent a comment on patch 2, > otherwise LGTM. > Thanks for the review, Simon. Patch 2 is updated per your comment. I pushed them in. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 0/7] Class-fy partial_die_info @ 2018-01-25 9:38 Yao Qi 2018-01-25 9:38 ` [PATCH 4/7] " Yao Qi 0 siblings, 1 reply; 22+ messages in thread From: Yao Qi @ 2018-01-25 9:38 UTC (permalink / raw) To: gdb-patches When I fix some issue related to dwarf, I class-fy partial_die_info as part of the fix. The class-fy bits can go upstream first. Regression tested on x86_64-linux. *** BLURB HERE *** Yao Qi (7): Re-write partial_die_info allocation in load_partial_dies Don't check abbrev is NULL in read_partial_die Change find_partial_die_in_comp_unit to dwarf2_cu::find_partial_die Class-fy partial_die_info Remove one argument abbrev_len in read_partial_die Move fixup_partial_die to partial_die_info::fixup Move read_partial_die to partial_die_info::read gdb/dwarf2read.c | 339 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 185 insertions(+), 154 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/7] Class-fy partial_die_info 2018-01-25 9:38 [PATCH 0/7] " Yao Qi @ 2018-01-25 9:38 ` Yao Qi 2018-01-25 16:19 ` Tom Tromey 2018-01-29 1:15 ` Simon Marchi 0 siblings, 2 replies; 22+ messages in thread From: Yao Qi @ 2018-01-25 9:38 UTC (permalink / raw) To: gdb-patches This patch is to class-fy partial_die_info. Two things special here, - disable assignment operator, but keep copy ctor, which is used in load_partial_dies, - have a private ctor which is only used by dwarf2_cu::find_partial_die, I don't want other code use it, so make it private, gdb: 2018-01-11 Yao Qi <yao.qi@linaro.org> * dwarf2read.c (struct partial_die_info): Add ctor, delete assignment operator. (load_partial_dies): Use ctor and copy ctor. (read_partial_die): Update. (dwarf2_cu::find_partial_die): Use ctor. --- gdb/dwarf2read.c | 86 ++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 62 insertions(+), 24 deletions(-) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 937faa2..9b4d09e 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -1404,14 +1404,21 @@ file_entry::include_dir (const line_header *lh) const need this much information. */ struct partial_die_info { + partial_die_info (sect_offset sect_off, struct abbrev_info *abbrev); + + /* Disable assign but still keep copy ctor, which is needed + load_partial_dies. */ + partial_die_info& operator=(const partial_die_info& rhs) = delete; + /* Offset of this DIE. */ - sect_offset sect_off; + const sect_offset sect_off; /* DWARF-2 tag for this DIE. */ - ENUM_BITFIELD(dwarf_tag) tag : 16; + const ENUM_BITFIELD(dwarf_tag) tag : 16; /* Assorted flags describing the data found in this DIE. */ - unsigned int has_children : 1; + const unsigned int has_children : 1; + unsigned int is_external : 1; unsigned int is_declaration : 1; unsigned int has_type : 1; @@ -1446,15 +1453,15 @@ struct partial_die_info /* The name of this DIE. Normally the value of DW_AT_name, but sometimes a default name for unnamed DIEs. */ - const char *name; + const char *name = nullptr; /* The linkage name, if present. */ - const char *linkage_name; + const char *linkage_name = nullptr; /* The scope to prepend to our children. This is generally allocated on the comp_unit_obstack, so will disappear when this compilation unit leaves the cache. */ - const char *scope; + const char *scope = nullptr; /* Some data associated with the partial DIE. The tag determines which field is live. */ @@ -1464,26 +1471,58 @@ struct partial_die_info struct dwarf_block *locdesc; /* The offset of an import, for DW_TAG_imported_unit. */ sect_offset sect_off; - } d; + } d {}; /* If HAS_PC_INFO, the PC range associated with this DIE. */ - CORE_ADDR lowpc; - CORE_ADDR highpc; + CORE_ADDR lowpc = 0; + CORE_ADDR highpc = 0; /* Pointer into the info_buffer (or types_buffer) pointing at the target of DW_AT_sibling, if any. */ /* NOTE: This member isn't strictly necessary, read_partial_die could return DW_AT_sibling values to its caller load_partial_dies. */ - const gdb_byte *sibling; + const gdb_byte *sibling = nullptr; /* If HAS_SPECIFICATION, the offset of the DIE referred to by DW_AT_specification (or DW_AT_abstract_origin or DW_AT_extension). */ - sect_offset spec_offset; + sect_offset spec_offset {}; /* Pointers to this DIE's parent, first child, and next sibling, if any. */ - struct partial_die_info *die_parent, *die_child, *die_sibling; + struct partial_die_info *die_parent = nullptr; + struct partial_die_info *die_child = nullptr; + struct partial_die_info *die_sibling = nullptr; + + friend struct partial_die_info * + dwarf2_cu::find_partial_die (sect_offset sect_off); + + private: + /* Only need to do look up in dwarf2_cu::find_partial_die. */ + partial_die_info (sect_offset sect_off) + : partial_die_info (sect_off, DW_TAG_padding, 0) + { + } + + partial_die_info (sect_offset sect_off_, enum dwarf_tag tag_, + int has_children_) + : sect_off (sect_off_), tag (tag_), has_children (has_children_) + { + is_external = 0; + is_declaration = 0; + has_type = 0; + has_specification = 0; + has_pc_info = 0; + may_be_inlined = 0; + main_subprogram = 0; + scope_set = 0; + has_byte_size = 0; + has_const_value = 0; + has_template_arguments = 0; + fixup_called = 0; + is_dwz = 0; + spec_is_dwz = 0; + } }; /* This data structure holds the information of an abbrev. */ @@ -18273,9 +18312,9 @@ load_partial_dies (const struct die_reader_specs *reader, continue; } - struct partial_die_info pdi; + struct partial_die_info pdi ((sect_offset) (info_ptr - reader->buffer), + abbrev); - memset (&pdi, 0, sizeof (pdi)); info_ptr = read_partial_die (reader, &pdi, abbrev, bytes_read, info_ptr); @@ -18353,7 +18392,8 @@ load_partial_dies (const struct die_reader_specs *reader, struct partial_die_info *part_die = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info); - memcpy (part_die, &pdi, sizeof (pdi)); + part_die = new (part_die) partial_die_info (pdi); + /* We'll save this DIE so link it in. */ part_die->die_parent = parent_die; part_die->die_sibling = NULL; @@ -18447,6 +18487,12 @@ load_partial_dies (const struct die_reader_specs *reader, } } +partial_die_info::partial_die_info (sect_offset sect_off_, + struct abbrev_info *abbrev) + : partial_die_info (sect_off_, abbrev->tag, abbrev->has_children) +{ +} + /* Read a minimal amount of information into the minimal die structure. */ static const gdb_byte * @@ -18466,15 +18512,8 @@ read_partial_die (const struct die_reader_specs *reader, int has_high_pc_attr = 0; int high_pc_relative = 0; - memset (part_die, 0, sizeof (struct partial_die_info)); - - part_die->sect_off = (sect_offset) (info_ptr - buffer); - info_ptr += abbrev_len; - part_die->tag = abbrev->tag; - part_die->has_children = abbrev->has_children; - for (i = 0; i < abbrev->num_attrs; ++i) { info_ptr = read_attribute (reader, &attr, &abbrev->attrs[i], info_ptr); @@ -18674,9 +18713,8 @@ struct partial_die_info * dwarf2_cu::find_partial_die (sect_offset sect_off) { struct partial_die_info *lookup_die = NULL; - struct partial_die_info part_die; + struct partial_die_info part_die (sect_off); - part_die.sect_off = sect_off; lookup_die = ((struct partial_die_info *) htab_find_with_hash (partial_dies, &part_die, to_underlying (sect_off))); -- 1.9.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] Class-fy partial_die_info 2018-01-25 9:38 ` [PATCH 4/7] " Yao Qi @ 2018-01-25 16:19 ` Tom Tromey 2018-01-26 17:25 ` Yao Qi 2018-01-29 1:15 ` Simon Marchi 1 sibling, 1 reply; 22+ messages in thread From: Tom Tromey @ 2018-01-25 16:19 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches >>>>> "Yao" == Yao Qi <qiyaoltc@gmail.com> writes: Yao> @@ -18353,7 +18392,8 @@ load_partial_dies (const struct die_reader_specs *reader, Yao> struct partial_die_info *part_die Yao> = XOBNEW (&cu->comp_unit_obstack, struct partial_die_info); Yao> - memcpy (part_die, &pdi, sizeof (pdi)); Yao> + part_die = new (part_die) partial_die_info (pdi); Yao> + I wonder if it would make sense to have an "operator new" implementation that allocates directly on an obstack. It could use std::is_trivially_destructible to enforce the rule that objects on an obstack can't really be destroyed. This would eliminate the separate XOBNEW, which is maybe a potential source of errors; and would also make it harder to accidentally add a destructor to objects allocated this way. Tom ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] Class-fy partial_die_info 2018-01-25 16:19 ` Tom Tromey @ 2018-01-26 17:25 ` Yao Qi 2018-01-26 20:55 ` Tom Tromey 0 siblings, 1 reply; 22+ messages in thread From: Yao Qi @ 2018-01-26 17:25 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches Tom Tromey <tom@tromey.com> writes: > I wonder if it would make sense to have an "operator new" implementation > that allocates directly on an obstack. It could use Yes, we can have an "operator new", > std::is_trivially_destructible to enforce the rule that objects on an > obstack can't really be destroyed. This would eliminate the separate > XOBNEW, which is maybe a potential source of errors; and would also make > it harder to accidentally add a destructor to objects allocated this way but why dtor must be trivial? We can have "operator new" and "operator delete", the former allocate spaces on obstack and the latter doesn't de-allocate space. It doesn't matter dtor is trivial or not. I may miss something here. Further, I think IWBN to have a class which has new/delete operator, and other classes can inherit it. What do you think the patch below? -- Yao (齐尧) From 9f20b2690cd1c83a3bdcd41ea59f07dfef0da522 Mon Sep 17 00:00:00 2001 From: Yao Qi <yao.qi@linaro.org> Date: Fri, 26 Jan 2018 11:57:34 +0000 Subject: [PATCH] New class allocate_on_obstack This patch adds a new class allocate_on_obstack, and let dwarf2_per_objfile inherit it, so that dwarf2_per_objfile is automatically allocated on obstack, and "delete dwarf2_per_objfile" doesn't de-allocate any space. gdb: 2018-01-26 Yao Qi <yao.qi@linaro.org> * dwarf2read.c (dwarf2_per_objfile): Inherit allocate_on_obstack. (dwarf2_free_objfile): Use delete. * gdb_obstack.h (allocate_on_obstack): New. diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 96026a8..5c45bdf 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -380,7 +380,7 @@ struct tu_stats /* Collection of data recorded per objfile. This hangs off of dwarf2_objfile_data_key. */ -struct dwarf2_per_objfile +struct dwarf2_per_objfile : public allocate_on_obstack { /* Construct a dwarf2_per_objfile for OBJFILE. NAMES points to the dwarf2 section names, or is NULL if the standard ELF names are @@ -2467,10 +2467,9 @@ dwarf2_has_info (struct objfile *objfile, if (dwarf2_per_objfile == NULL) { /* Initialize per-objfile state. */ - struct dwarf2_per_objfile *data - = XOBNEW (&objfile->objfile_obstack, struct dwarf2_per_objfile); - - dwarf2_per_objfile = new (data) struct dwarf2_per_objfile (objfile, names); + dwarf2_per_objfile + = new (&objfile->objfile_obstack) struct dwarf2_per_objfile (objfile, + names); set_dwarf2_per_objfile (objfile, dwarf2_per_objfile); } return (!dwarf2_per_objfile->info.is_virtual @@ -25168,10 +25167,7 @@ dwarf2_free_objfile (struct objfile *objfile) struct dwarf2_per_objfile *dwarf2_per_objfile = get_dwarf2_per_objfile (objfile); - if (dwarf2_per_objfile == NULL) - return; - - dwarf2_per_objfile->~dwarf2_per_objfile (); + delete dwarf2_per_objfile; } /* A set of CU "per_cu" pointer, DIE offset, and GDB type pointer. diff --git a/gdb/gdb_obstack.h b/gdb/gdb_obstack.h index 12a90c3..b239ce6 100644 --- a/gdb/gdb_obstack.h +++ b/gdb/gdb_obstack.h @@ -78,4 +78,18 @@ struct auto_obstack : obstack { obstack_free (this, obstack_base (this)); } }; +/* Objects are allocated on obstack instead of heap. */ + +struct allocate_on_obstack +{ + void* operator new (size_t size, struct obstack *obstack) + { + return obstack_alloc (obstack, size); + } + + void operator delete (void* memory) + { + } +}; + #endif ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] Class-fy partial_die_info 2018-01-26 17:25 ` Yao Qi @ 2018-01-26 20:55 ` Tom Tromey 0 siblings, 0 replies; 22+ messages in thread From: Tom Tromey @ 2018-01-26 20:55 UTC (permalink / raw) To: Yao Qi; +Cc: Tom Tromey, gdb-patches >> std::is_trivially_destructible to enforce the rule that objects on an >> obstack can't really be destroyed. This would eliminate the separate >> XOBNEW, which is maybe a potential source of errors; and would also make >> it harder to accidentally add a destructor to objects allocated this way Yao> but why dtor must be trivial? We can have "operator new" and "operator Yao> delete", the former allocate spaces on obstack and the latter doesn't Yao> de-allocate space. It doesn't matter dtor is trivial or not. I may Yao> miss something here. My thinking was that if something is allocated on an obstack, then presumably the destructor will never be run. So, it's best to avoid confusion by requiring a trivial destructor. I suppose it would be possible to track objects and destroy them, but if that's done, then it sort of eliminates the point of an obstack -- it would be just as convenient at that point to use the ordinary new. Yao> Further, I think IWBN to have a class which has new/delete operator, Yao> and other classes can inherit it. What do you think the patch below? I suppose this makes sense if you know that all objects of a given type must be allocated on an obstack. Tom ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] Class-fy partial_die_info 2018-01-25 9:38 ` [PATCH 4/7] " Yao Qi 2018-01-25 16:19 ` Tom Tromey @ 2018-01-29 1:15 ` Simon Marchi 2018-01-30 10:49 ` Yao Qi 2018-01-30 11:39 ` Pedro Alves 1 sibling, 2 replies; 22+ messages in thread From: Simon Marchi @ 2018-01-29 1:15 UTC (permalink / raw) To: Yao Qi, gdb-patches On 2018-01-25 04:38 AM, Yao Qi wrote: > This patch is to class-fy partial_die_info. Two things special here, > > - disable assignment operator, but keep copy ctor, which is used in > load_partial_dies, > - have a private ctor which is only used by dwarf2_cu::find_partial_die, > I don't want other code use it, so make it private, Hi Yao, From what I understand, the only reason to have that private constructor is to construct a temporary partial_die_info object used to search in the htab, is that right? If so, converting that htab_t to an std::unordered_map would remove the need for all this, since you don't need to construct an object to search it. See the diff below that applies on top of this patch. It's not thoroughly tested and I am not sure of the validity of the per_cu->cu->partial_dies.empty () call in find_partial_die, but I think it should work. Plus, it adds some type-safety, which I am a big fan of. But otherwise, the patch is fine with me. Simon From dacaf6028e921efeb8c7420db7f663e5b4d7e8f1 Mon Sep 17 00:00:00 2001 From: Simon Marchi <simon.marchi@polymtl.ca> Date: Sun, 28 Jan 2018 19:47:01 -0500 Subject: [PATCH] use std::unordered_map --- gdb/dwarf2read.c | 115 +++++++++++++------------------------------------------ 1 file changed, 26 insertions(+), 89 deletions(-) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 8996f49bae..330ca87a1d 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -652,6 +652,8 @@ struct delayed_method_info struct die_info *die; }; +struct partial_die_info; + /* Internal state when decoding a particular compilation unit. */ struct dwarf2_cu { @@ -686,9 +688,9 @@ struct dwarf2_cu distinguish these in buildsym.c. */ struct pending **list_in_scope = nullptr; - /* Hash table holding all the loaded partial DIEs - with partial_die->offset.SECT_OFF as hash. */ - htab_t partial_dies = nullptr; + /* Hash map holding all the loaded partial DIEs + with their section offset as the key. */ + std::unordered_map<sect_offset, partial_die_info *> partial_dies; /* Storage for things with the same lifetime as this read-in compilation unit, including partial DIEs. */ @@ -1493,36 +1495,6 @@ struct partial_die_info struct partial_die_info *die_parent = nullptr; struct partial_die_info *die_child = nullptr; struct partial_die_info *die_sibling = nullptr; - - friend struct partial_die_info * - dwarf2_cu::find_partial_die (sect_offset sect_off); - - private: - /* Only need to do look up in dwarf2_cu::find_partial_die. */ - partial_die_info (sect_offset sect_off) - : partial_die_info (sect_off, DW_TAG_padding, 0) - { - } - - partial_die_info (sect_offset sect_off_, enum dwarf_tag tag_, - int has_children_) - : sect_off (sect_off_), tag (tag_), has_children (has_children_) - { - is_external = 0; - is_declaration = 0; - has_type = 0; - has_specification = 0; - has_pc_info = 0; - may_be_inlined = 0; - main_subprogram = 0; - scope_set = 0; - has_byte_size = 0; - has_const_value = 0; - has_template_arguments = 0; - fixup_called = 0; - is_dwz = 0; - spec_is_dwz = 0; - } }; /* This data structure holds the information of an abbrev. */ @@ -2180,10 +2152,6 @@ static const gdb_byte *skip_one_die (const struct die_reader_specs *reader, const gdb_byte *info_ptr, struct abbrev_info *abbrev); -static hashval_t partial_die_hash (const void *item); - -static int partial_die_eq (const void *item_lhs, const void *item_rhs); - static struct dwarf2_per_cu_data *dwarf2_find_containing_comp_unit (sect_offset sect_off, unsigned int offset_in_dwz, struct dwarf2_per_objfile *dwarf2_per_objfile); @@ -18259,14 +18227,7 @@ load_partial_dies (const struct die_reader_specs *reader, if (cu->per_cu->load_all_dies) load_all = 1; - cu->partial_dies - = htab_create_alloc_ex (cu->header.length / 12, - partial_die_hash, - partial_die_eq, - NULL, - &cu->comp_unit_obstack, - hashtab_obstack_allocate, - dummy_obstack_deallocate); + cu->partial_dies.clear (); while (1) { @@ -18459,14 +18420,7 @@ load_partial_dies (const struct die_reader_specs *reader, || abbrev->tag == DW_TAG_variable || abbrev->tag == DW_TAG_namespace || part_die->is_declaration) - { - void **slot; - - slot = htab_find_slot_with_hash (cu->partial_dies, part_die, - to_underlying (part_die->sect_off), - INSERT); - *slot = part_die; - } + cu->partial_dies[part_die->sect_off] = part_die; /* For some DIEs we want to follow their children (if any). For C we have no reason to follow the children of structures; for other @@ -18512,8 +18466,22 @@ load_partial_dies (const struct die_reader_specs *reader, partial_die_info::partial_die_info (sect_offset sect_off_, struct abbrev_info *abbrev) - : partial_die_info (sect_off_, abbrev->tag, abbrev->has_children) -{ + : sect_off (sect_off_), tag (abbrev->tag), has_children (abbrev->has_children) +{ + is_external = 0; + is_declaration = 0; + has_type = 0; + has_specification = 0; + has_pc_info = 0; + may_be_inlined = 0; + main_subprogram = 0; + scope_set = 0; + has_byte_size = 0; + has_const_value = 0; + has_template_arguments = 0; + fixup_called = 0; + is_dwz = 0; + spec_is_dwz = 0; } /* Read a minimal amount of information into the minimal die structure. */ @@ -18735,14 +18703,9 @@ read_partial_die (const struct die_reader_specs *reader, struct partial_die_info * dwarf2_cu::find_partial_die (sect_offset sect_off) { - struct partial_die_info *lookup_die = NULL; - struct partial_die_info part_die (sect_off); - - lookup_die = ((struct partial_die_info *) - htab_find_with_hash (partial_dies, &part_die, - to_underlying (sect_off))); + auto it = partial_dies.find (sect_off); - return lookup_die; + return it != partial_dies.end () ? it->second : nullptr; } /* Find a partial DIE at OFFSET, which may or may not be in CU, @@ -18782,7 +18745,7 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu) per_cu = dwarf2_find_containing_comp_unit (sect_off, offset_in_dwz, dwarf2_per_objfile); - if (per_cu->cu == NULL || per_cu->cu->partial_dies == NULL) + if (per_cu->cu == NULL || per_cu->cu->partial_dies.empty ()) load_partial_comp_unit (per_cu); per_cu->cu->last_used = 0; @@ -25483,32 +25446,6 @@ dwarf2_clear_marks (struct dwarf2_per_cu_data *per_cu) } } -/* Trivial hash function for partial_die_info: the hash value of a DIE - is its offset in .debug_info for this objfile. */ - -static hashval_t -partial_die_hash (const void *item) -{ - const struct partial_die_info *part_die - = (const struct partial_die_info *) item; - - return to_underlying (part_die->sect_off); -} - -/* Trivial comparison function for partial_die_info structures: two DIEs - are equal if they have the same offset. */ - -static int -partial_die_eq (const void *item_lhs, const void *item_rhs) -{ - const struct partial_die_info *part_die_lhs - = (const struct partial_die_info *) item_lhs; - const struct partial_die_info *part_die_rhs - = (const struct partial_die_info *) item_rhs; - - return part_die_lhs->sect_off == part_die_rhs->sect_off; -} - static struct cmd_list_element *set_dwarf_cmdlist; static struct cmd_list_element *show_dwarf_cmdlist; -- 2.16.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] Class-fy partial_die_info 2018-01-29 1:15 ` Simon Marchi @ 2018-01-30 10:49 ` Yao Qi 2018-01-30 15:11 ` Pedro Alves 2018-01-30 11:39 ` Pedro Alves 1 sibling, 1 reply; 22+ messages in thread From: Yao Qi @ 2018-01-30 10:49 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches Simon Marchi <simark@simark.ca> writes: > From what I understand, the only reason to have that private constructor is > to construct a temporary partial_die_info object used to search in the htab, > is that right? If so, converting that htab_t to an std::unordered_map would Right. > > - /* Hash table holding all the loaded partial DIEs > - with partial_die->offset.SECT_OFF as hash. */ > - htab_t partial_dies = nullptr; > + /* Hash map holding all the loaded partial DIEs > + with their section offset as the key. */ > + std::unordered_map<sect_offset, partial_die_info *> partial_dies; > This doesn't compile with my g++ 4.9, as library doesn't provide std::hash<T> specialization for enumeration types. It is available since C++ 14. http://en.cppreference.com/w/cpp/utility/hash I can change it to std::unordered_map<std::underlying_type<sect_offset>::type, partial_die_info *> partial_dies; to fix the compiler errors. > @@ -18259,14 +18227,7 @@ load_partial_dies (const struct die_reader_specs *reader, > if (cu->per_cu->load_all_dies) > load_all = 1; > > - cu->partial_dies > - = htab_create_alloc_ex (cu->header.length / 12, > - partial_die_hash, > - partial_die_eq, > - NULL, > - &cu->comp_unit_obstack, > - hashtab_obstack_allocate, > - dummy_obstack_deallocate); > + cu->partial_dies.clear (); This changes the behavior, without this patch, cu->partial_dies's elements are allocated on cu->comp_unit_obstack. After this patch, elements are allocated on heap. I don't know how does it affect performance. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] Class-fy partial_die_info 2018-01-30 10:49 ` Yao Qi @ 2018-01-30 15:11 ` Pedro Alves 0 siblings, 0 replies; 22+ messages in thread From: Pedro Alves @ 2018-01-30 15:11 UTC (permalink / raw) To: Yao Qi, Simon Marchi; +Cc: gdb-patches On 01/30/2018 10:49 AM, Yao Qi wrote: > Simon Marchi <simark@simark.ca> writes: >> - /* Hash table holding all the loaded partial DIEs >> - with partial_die->offset.SECT_OFF as hash. */ >> - htab_t partial_dies = nullptr; >> + /* Hash map holding all the loaded partial DIEs >> + with their section offset as the key. */ >> + std::unordered_map<sect_offset, partial_die_info *> partial_dies; >> > This doesn't compile with my g++ 4.9, as library doesn't provide > std::hash<T> specialization for enumeration types. It is available > since C++ 14. http://en.cppreference.com/w/cpp/utility/hash > > I can change it to > > std::unordered_map<std::underlying_type<sect_offset>::type, > partial_die_info *> partial_dies; > > to fix the compiler errors. Note: in cases like these, we don't need to forgo using the (strong) enum as key type. unordered_map's third (defaulted) template parameter type is the hasher to use. And we have a convenience hasher for this: [pushed] Add gdb::hash_enum https://sourceware.org/ml/gdb-patches/2017-12/msg00210.html Currently used in dwarf2read.c: std::unordered_map<sect_offset, dwarf2_per_cu_data *, gdb::hash_enum<sect_offset>> Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] Class-fy partial_die_info 2018-01-29 1:15 ` Simon Marchi 2018-01-30 10:49 ` Yao Qi @ 2018-01-30 11:39 ` Pedro Alves 2018-01-31 3:46 ` Simon Marchi 1 sibling, 1 reply; 22+ messages in thread From: Pedro Alves @ 2018-01-30 11:39 UTC (permalink / raw) To: Simon Marchi, Yao Qi, gdb-patches On 01/29/2018 01:15 AM, Simon Marchi wrote: > From what I understand, the only reason to have that private constructor is > to construct a temporary partial_die_info object used to search in the htab, > is that right? If so, converting that htab_t to an std::unordered_map would > remove the need for all this, since you don't need to construct an object > to search it. See the diff below that applies on top of this patch. > > It's not thoroughly tested and I am not sure of the validity of the > per_cu->cu->partial_dies.empty () call in find_partial_die, but I think it > should work. Plus, it adds some type-safety, which I am a big fan of. > > But otherwise, the patch is fine with me. Careful here. This could do with some benchmarking. The DWARF reading code is performance (both timing and memory) sensitive. This is trading an open addressing hash table (htab_t), for a node-based closed addressing hash table. The keys/elements in the map are small so I'd expect this to make a difference. Also, this is trading a in-principle cache-friendly obstack allocation scheme for the standard new allocator. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] Class-fy partial_die_info 2018-01-30 11:39 ` Pedro Alves @ 2018-01-31 3:46 ` Simon Marchi 2018-01-31 11:55 ` Yao Qi 2018-01-31 15:33 ` Pedro Alves 0 siblings, 2 replies; 22+ messages in thread From: Simon Marchi @ 2018-01-31 3:46 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, gdb-patches On 2018-01-30 06:39, Pedro Alves wrote: > On 01/29/2018 01:15 AM, Simon Marchi wrote: > >> From what I understand, the only reason to have that private >> constructor is >> to construct a temporary partial_die_info object used to search in the >> htab, >> is that right? If so, converting that htab_t to an std::unordered_map >> would >> remove the need for all this, since you don't need to construct an >> object >> to search it. See the diff below that applies on top of this patch. >> >> It's not thoroughly tested and I am not sure of the validity of the >> per_cu->cu->partial_dies.empty () call in find_partial_die, but I >> think it >> should work. Plus, it adds some type-safety, which I am a big fan of. >> >> But otherwise, the patch is fine with me. > > Careful here. This could do with some benchmarking. The DWARF reading > code > is performance (both timing and memory) sensitive. This is trading an > open > addressing hash table (htab_t), for a node-based closed addressing hash > table. > The keys/elements in the map are small so I'd expect this to make > a difference. Also, this is trading a in-principle cache-friendly > obstack allocation scheme for the standard new allocator. Ah, indeed. I thought that unordered_map would be implemented the same way as htab_t, but I see it's not the case. Doing some quick tests on a big binary, it increases the time reading the symbols from an average of 37 seconds to an average of 42 seconds. I understand the different hash table implementation having an impact, but I don't really understand how the allocation scheme can have a meaningful impact. The partial_die_info objects are still allocated on the obstack, aren't they? So it's just the space for the table itself that isn't on the objstack, but I don't see why that would make a difference. If we want to have a data structure with the same kind of performance as htab_t but with type-safety in the future, is your vision that we'll have to implement it ourselves? Should we make a wrapper around htab_t? Simon ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] Class-fy partial_die_info 2018-01-31 3:46 ` Simon Marchi @ 2018-01-31 11:55 ` Yao Qi 2018-01-31 15:33 ` Pedro Alves 1 sibling, 0 replies; 22+ messages in thread From: Yao Qi @ 2018-01-31 11:55 UTC (permalink / raw) To: Simon Marchi; +Cc: Pedro Alves, gdb-patches Simon Marchi <simark@simark.ca> writes: > Ah, indeed. I thought that unordered_map would be implemented the > same way as htab_t, but I see it's not the case. Doing some quick > tests on a big binary, it increases the time reading the symbols from > an average of 37 seconds to an average of 42 seconds. > > I understand the different hash table implementation having an impact, > but I don't really understand how the allocation scheme can have a > meaningful impact. The partial_die_info objects are still allocated > on the obstack, aren't they? So it's just the space for the table > itself that isn't on the objstack, but I don't see why that would make > a difference. Hi Simon, We have some perf test cases, in gdb.perf, but they may not cover the path we are discussing here. If you want to run them, do these things, $ cd gdb/testsuite $ make -j10 build-perf RUNTESTFLAGS="MONSTER=y gmonster1.exp" // this takes a while to generate many executable files, $ make check-perf RUNTESTFLAGS="MONSTER=y gmonster1-null-lookup.exp gmonster1-print-cerr.exp gmonster1-runto-main.exp gmonster1-pervasive-typedef.exp gmonster1-ptype-string.exp gmonster1-select-file.exp" GDB_PERFTEST_MODE=run // it takes one hour on my aarch64-linux box You can get the performance number in perftest.sum. I run these perf tests, with and without Simon's patch (htab_t -> std::unordered_map), on aarch64-linux, I don't see speed and space change. Again, these existing test cases may not cover the path. gmonster1:gmonster-null-lookup cpu_time 10-cus 0.0008508 0.0013708 gmonster1:gmonster-null-lookup cpu_time 100-cus 0.0047922 0.0030584 gmonster1:gmonster-null-lookup cpu_time 1000-cus 0.0400274 0.0397188 gmonster1:gmonster-null-lookup cpu_time 10000-cus 0.3885292 0.3862456 gmonster1:gmonster-null-lookup wall_time 10-cus 0.000862598419189 0.00137987136841 gmonster1:gmonster-null-lookup wall_time 100-cus 0.00480117797852 0.00306539535522 gmonster1:gmonster-null-lookup wall_time 1000-cus 0.0400432109833 0.0397333621979 gmonster1:gmonster-null-lookup wall_time 10000-cus 0.388552331924 0.386282110214 gmonster1:gmonster-null-lookup vmsize 10-cus 35900 35896 gmonster1:gmonster-null-lookup vmsize 100-cus 64992 64972 gmonster1:gmonster-null-lookup vmsize 1000-cus 364748 364760 gmonster1:gmonster-null-lookup vmsize 10000-cus 3313284 3313360 gmonster1:gmonster-pervasive-typedef cpu_time 10-cus 0.0682848 0.0737696 gmonster1:gmonster-pervasive-typedef cpu_time 100-cus 0.5843324 0.6391266 gmonster1:gmonster-pervasive-typedef cpu_time 1000-cus 6.061932 6.621443 gmonster1:gmonster-pervasive-typedef cpu_time 10000-cus 62.6619226 67.1514486 gmonster1:gmonster-pervasive-typedef wall_time 10-cus 0.0683585643768 0.0737894058228 gmonster1:gmonster-pervasive-typedef wall_time 100-cus 0.585014867783 0.641395568848 gmonster1:gmonster-pervasive-typedef wall_time 1000-cus 6.07634234428 6.62292981148 gmonster1:gmonster-pervasive-typedef wall_time 10000-cus 62.6738821507 67.2341232777 gmonster1:gmonster-pervasive-typedef vmsize 10-cus 32381 32368 gmonster1:gmonster-pervasive-typedef vmsize 100-cus 41131 41084 gmonster1:gmonster-pervasive-typedef vmsize 1000-cus 129726 129553 gmonster1:gmonster-pervasive-typedef vmsize 10000-cus 1007898 1007878 gmonster1:gmonster-print-cerr cpu_time 10-cus 0.0011148 0.0011168 gmonster1:gmonster-print-cerr cpu_time 100-cus 0.0056498 0.0056002 gmonster1:gmonster-print-cerr cpu_time 1000-cus 0.0502508 0.0948982 gmonster1:gmonster-print-cerr cpu_time 10000-cus 0.4922956 0.4948586 gmonster1:gmonster-print-cerr wall_time 10-cus 0.00112357139587 0.00112380981445 gmonster1:gmonster-print-cerr wall_time 100-cus 0.00565934181213 0.00561075210571 gmonster1:gmonster-print-cerr wall_time 1000-cus 0.0502710342407 0.0949506282806 gmonster1:gmonster-print-cerr wall_time 10000-cus 0.492320823669 0.494928407669 gmonster1:gmonster-print-cerr vmsize 10-cus 32508 32500 gmonster1:gmonster-print-cerr vmsize 100-cus 41308 41300 gmonster1:gmonster-print-cerr vmsize 1000-cus 128944 128948 gmonster1:gmonster-print-cerr vmsize 10000-cus 993324 993316 gmonster1:gmonster-runto-main cpu_time 10-cus 0.0360472 0.0231514 gmonster1:gmonster-runto-main cpu_time 100-cus 0.5829484 0.5236148 gmonster1:gmonster-runto-main cpu_time 1000-cus 9.146062 7.9914552 gmonster1:gmonster-runto-main cpu_time 10000-cus 134.266728 134.1361918 gmonster1:gmonster-runto-main wall_time 10-cus 0.042032957077 0.0313341617584 gmonster1:gmonster-runto-main wall_time 100-cus 0.588344860077 0.530299949646 gmonster1:gmonster-runto-main wall_time 1000-cus 9.15567464828 7.99887242317 gmonster1:gmonster-runto-main wall_time 10000-cus 134.285585785 134.15385747 gmonster1:gmonster-runto-main vmsize 10-cus 32503 32496 gmonster1:gmonster-runto-main vmsize 100-cus 41308 41296 gmonster1:gmonster-runto-main vmsize 1000-cus 128960 128952 gmonster1:gmonster-runto-main vmsize 10000-cus 993336 993296 gmonster1:gmonster-select-file cpu_time 10-cus 0.0972798 0.1028992 gmonster1:gmonster-select-file cpu_time 100-cus 0.886951 0.9285964 gmonster1:gmonster-select-file cpu_time 1000-cus 9.166904 9.7175802 gmonster1:gmonster-select-file cpu_time 10000-cus 92.5872488 95.2634352 gmonster1:gmonster-select-file wall_time 10-cus 0.097380399704 0.103002214432 gmonster1:gmonster-select-file wall_time 100-cus 0.887106800079 0.929784965515 gmonster1:gmonster-select-file wall_time 1000-cus 9.17858901024 9.72046675682 gmonster1:gmonster-select-file wall_time 10000-cus 92.6326120853 95.3189713955 gmonster1:gmonster-select-file vmsize 10-cus 32367 32384 gmonster1:gmonster-select-file vmsize 100-cus 41064 41056 gmonster1:gmonster-select-file vmsize 1000-cus 128694 128703 gmonster1:gmonster-select-file vmsize 10000-cus 993052 993048 -- Yao (齐尧) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] Class-fy partial_die_info 2018-01-31 3:46 ` Simon Marchi 2018-01-31 11:55 ` Yao Qi @ 2018-01-31 15:33 ` Pedro Alves 1 sibling, 0 replies; 22+ messages in thread From: Pedro Alves @ 2018-01-31 15:33 UTC (permalink / raw) To: Simon Marchi; +Cc: Yao Qi, gdb-patches On 01/31/2018 03:46 AM, Simon Marchi wrote: > On 2018-01-30 06:39, Pedro Alves wrote: >> On 01/29/2018 01:15 AM, Simon Marchi wrote: >> >>> From what I understand, the only reason to have that private constructor is >>> to construct a temporary partial_die_info object used to search in the htab, >>> is that right? If so, converting that htab_t to an std::unordered_map would >>> remove the need for all this, since you don't need to construct an object >>> to search it. See the diff below that applies on top of this patch. >>> >>> It's not thoroughly tested and I am not sure of the validity of the >>> per_cu->cu->partial_dies.empty () call in find_partial_die, but I think it >>> should work. Plus, it adds some type-safety, which I am a big fan of. >>> >>> But otherwise, the patch is fine with me. >> >> Careful here. This could do with some benchmarking. The DWARF reading code >> is performance (both timing and memory) sensitive. This is trading an open >> addressing hash table (htab_t), for a node-based closed addressing hash table. >> The keys/elements in the map are small so I'd expect this to make >> a difference. Also, this is trading a in-principle cache-friendly >> obstack allocation scheme for the standard new allocator. > > Ah, indeed. I thought that unordered_map would be implemented the same way as htab_t, but I see it's not the case. Doing some quick tests on a big binary, it increases the time reading the symbols from an average of 37 seconds to an average of 42 seconds. > > I understand the different hash table implementation having an impact, but I don't really understand how the allocation scheme can have a meaningful impact. The partial_die_info objects are still allocated on the obstack, aren't they? So it's just the space for the table itself that isn't on the objstack, but I don't see why that would make a difference. Well, that's the thing. unordered_map is going to reach to the heap to allocate the buckets and nodes. And the heap is slow. And there's a memory size cost for each element too, along with that reducing cache locality. unordered_map is really inefficient for this use case. We have a hash map of small objects that is filled in once, and after initial fill, we don't do random insert/remove of elements from this map. I think. > > If we want to have a data structure with the same kind of performance as htab_t but with type-safety in the future, is your vision that we'll have to implement it ourselves? Should we make a wrapper around htab_t? Well, my vision is to investigate alternatives. :-) There are many options out there. If you google around for hash table benchmarks you'll find several. E.g.: <https://tessil.github.io/2016/08/29/benchmark-hopscotch-map.html>. So there are good alternatives out there that we could import and use. AFAIK, libiberty's htab_t is actually quite good, and GCC has a C++-ified version of that. See gcc/hash-table.h, gcc/hash-map.h, gcc/hash-set.h: /* This file implements a typed hash table. The implementation borrows from libiberty's htab_t in hashtab.h. So sharing that code with GCC may make sense, as it's already in "the family". One thing I dislike about GCC's hash containers above is that their API isn't modeled on the standard's, which can lead to confusion if you're used to the standard containers, or if you're experimenting/benchmarking different containers for some use case. You have things like: /* This function clears all entries in this hash table. */ void empty () { if (elements ()) empty_slow (); } while in the std containers, empty() is a predicate... It may be that GCC's hash/map/set could do with C++11-fication too. Really I haven't investigated much. I'm just aware of their existence. (A while ago I found reading this discussion quite educational: <https://bugs.ruby-lang.org/issues/12142>) Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-02-26 15:39 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-22 15:36 [PATCH 0/7 v2] Class-fy partial_die_info Yao Qi 2018-02-22 15:36 ` [PATCH 3/7] Change find_partial_die_in_comp_unit to dwarf2_cu::find_partial_die Yao Qi 2018-02-22 15:36 ` [PATCH 1/7] Re-write partial_die_info allocation in load_partial_dies Yao Qi 2018-02-22 15:36 ` [PATCH 5/7] Remove one argument abbrev_len in read_partial_die Yao Qi 2018-02-22 15:36 ` [PATCH 4/7] Class-fy partial_die_info Yao Qi 2018-02-22 15:36 ` [PATCH 7/7] Move read_partial_die to partial_die_info::read Yao Qi 2018-02-22 15:36 ` [PATCH 2/7] Don't check abbrev is NULL in read_partial_die Yao Qi 2018-02-25 23:33 ` Simon Marchi 2018-02-22 15:36 ` [PATCH 6/7] Move fixup_partial_die to partial_die_info::fixup Yao Qi 2018-02-25 23:52 ` [PATCH 0/7 v2] Class-fy partial_die_info Simon Marchi 2018-02-26 15:39 ` Yao Qi -- strict thread matches above, loose matches on Subject: below -- 2018-01-25 9:38 [PATCH 0/7] " Yao Qi 2018-01-25 9:38 ` [PATCH 4/7] " Yao Qi 2018-01-25 16:19 ` Tom Tromey 2018-01-26 17:25 ` Yao Qi 2018-01-26 20:55 ` Tom Tromey 2018-01-29 1:15 ` Simon Marchi 2018-01-30 10:49 ` Yao Qi 2018-01-30 15:11 ` Pedro Alves 2018-01-30 11:39 ` Pedro Alves 2018-01-31 3:46 ` Simon Marchi 2018-01-31 11:55 ` Yao Qi 2018-01-31 15:33 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).