[patch] Fix 100x slowdown regression on DWZ files

[patch] Fix 100x slowdown regression on DWZ files

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 7874 bytes --]

Hi,

Bug 16405 - backtrace takes GBs and minutes with dwz -m (edit) 
https://sourceware.org/bugzilla/show_bug.cgi?id=16405

Since Fedora started to use DWZ DWARF compressor:
	http://fedoraproject.org/wiki/Features/DwarfCompressor
GDB has slowed down a lot.  To make it clear - DWZ is DWARF structure
rearrangement, "compressor" does not mean any zlib style data compression.

This patch reduces LibreOffice backtrace from 5 minutes to 3 seconds (100x)
and it also reduces memory consumption 20x.
[ benchmark is at the bottom of this mail ]

Example of DWZ output:
------------------------------------------------------------------------------
  Compilation Unit @ offset 0xc4:
 <0><cf>: Abbrev Number: 17 (DW_TAG_partial_unit)
    <d0>   DW_AT_stmt_list   : 0x0
    <d4>   DW_AT_comp_dir    : (indirect string, offset: 0x6f): /usr/src/debug/gdb-7.7.1/build-x86_64-redhat-linux-gnu/gdb
 <1><d8>: Abbrev Number: 9 (DW_TAG_typedef)
    <d9>   DW_AT_name        : (indirect string, offset: 0x827dc): size_t
    <dd>   DW_AT_decl_file   : 4
    <de>   DW_AT_decl_line   : 212
    <df>   DW_AT_type        : <0xae>

  Compilation Unit @ offset 0xe4:
 <0><ef>: Abbrev Number: 13 (DW_TAG_partial_unit)
    <f0>   DW_AT_stmt_list   : 0x0
    <f4>   DW_AT_comp_dir    : (indirect string, offset: 0x6f): /usr/src/debug/gdb-7.7.1/build-x86_64-redhat-linux-gnu/gdb
 <1><f8>: Abbrev Number: 45 (DW_TAG_typedef)
    <f9>   DW_AT_name        : (indirect string, offset: 0x251): __off_t
    <fd>   DW_AT_decl_file   : 3
    <fe>   DW_AT_decl_line   : 131
    <ff>   DW_AT_type        : <0x68>

  Compilation Unit @ offset 0x62d9f9:
 <0><62da04>: Abbrev Number: 20 (DW_TAG_compile_unit)
[...]
    <62da12>   DW_AT_low_pc      : 0x807e10
    <62da1a>   DW_AT_high_pc     : 134
    <62da1c>   DW_AT_stmt_list   : 0xf557e
 <1><62da20>: Abbrev Number: 7 (DW_TAG_imported_unit)
    <62da21>   DW_AT_import      : <0xcf>       [Abbrev Number: 17]
------------------------------------------------------------------------------

One can see all DW_TAG_partial_unit have DW_AT_stmt_list 0x0 which causes
repeated decoding of that .debug_line unit on each DW_TAG_imported_unit.

This was OK before as each DW_TAG_compile_unit has its own .debug_line unit.
But since the introduction of DW_TAG_partial_unit by DWZ one should cache
read-in DW_AT_stmt_list .debug_line units.

Fortunately one does not need to cache whole
	struct linetable *symtab->linetable
and other data from .debug_line mapping PC<->lines
------------------------------------------------------------------------------
 Line Number Statements:
  Extended opcode 2: set Address to 0x45c880
  Advance Line by 25 to 26
  Copy
------------------------------------------------------------------------------
as the only part of .debug_line which GDB needs for DW_TAG_partial_unit is:
------------------------------------------------------------------------------
 The Directory Table:
  ../../gdb
  /usr/include/bits
[...]
 The File Name Table:
  Entry Dir     Time    Size    Name
  1     1       0       0       gdb.c
  2     2       0       0       string3.h
[...]
------------------------------------------------------------------------------
specifically referenced in GDB for DW_AT_decl_file at a single place:
------------------------------------------------------------------------------
              fe = &cu->line_header->file_names[file_index - 1];
              SYMBOL_SYMTAB (sym) = fe->symtab;
------------------------------------------------------------------------------

This is because for some reason DW_TAG_partial_unit never contains PC-related
DWARF information.  I do not know exactly why, the compression ratio is a bit
lower due to it but thanksfully currently it is that way:
dwz.c:
------------------------------------------------------------------------------
        /* These attributes reference code, prevent moving
           DIEs with them.  */
        case DW_AT_low_pc:
        case DW_AT_high_pc:
        case DW_AT_entry_pc:
        case DW_AT_ranges:
          die->die_ck_state = CK_BAD;
+
  /* State of checksum computation.  Not computed yet, computed and
     suitable for moving into partial units, currently being computed
     and finally determined unsuitable for moving into partial units.  */
  enum { CK_UNKNOWN, CK_KNOWN, CK_BEING_COMPUTED, CK_BAD } die_ck_state : 2;
------------------------------------------------------------------------------
I have also verified also real-world Fedora debuginfo files really comply with
that assumption with dwgrep
	https://github.com/pmachata/dwgrep
using:
------------------------------------------------------------------------------
dwgrep -e 'entry ?DW_TAG_partial_unit child* ( ?DW_AT_low_pc , ?DW_AT_high_pc , ?DW_AT_ranges )' /usr/lib/debug/**
------------------------------------------------------------------------------

BTW I think GDB already does not support the whole DW_TAG_imported_unit and
DW_TAG_partial_unit usage possibilities as specified by the DWARF standard.
I think GDB would not work if DW_TAG_imported_unit was used in some inner
level and not at the CU level (readelf -wi level <1>) - this is how DWZ is
using DW_TAG_imported_unit.  Therefore I do not think further assumptions
about DW_TAG_imported_unit and DW_TAG_partial_unit usage by DWZ are a problem
for GDB.

One could save the whole .debug_line decoded PC<->lines mapping (and not just
the DW_AT_decl_file table) but:
 * there are some problematic corner cases so one could do it incorrectly
 * there are no real world data to really test such patch extension
 * such extension could be done perfectly incrementally on top of this patch

One could save existing 'struct line_header' in the patch instead of
introducing new 'struct dwarf2_line_info'.  The problem is
dwarf_decode_line_header() currently uses xmalloc while we need the allocation
to be bound to objfile->objfile_obstack.  With 'struct line_header' one would
have to make special objfile-time destruction of those 'struct line_header'.

No regressions on {x86_64,x86_64-m32,i686}-fedora21pre-linux-gnu in DWZ mode
(contrib/cc-with-tweaks.sh -m, that is dwz -m).  I have seen there are several
GDB regressions in non-DWZ -> DWZ mode but that is off-topic for this patch.


Thanks,
Jan

------------------------------------------------------------------------------

benchmark - on Fedora 20 x86_64 and FSF GDB HEAD:
echo -e 'thread apply all bt\nset confirm no\nq'|./gdb -p `pidof soffice.bin` -ex 'set pagination off' -ex 'maintenance set per-command space' -ex 'maintenance set per-command symtab' -ex 'maintenance set per-command time'

FSF GDB HEAD ("thread apply all bt"):
Command execution time: 333.693000 (cpu), 335.587539 (wall)
                                          ---sec
Space used: 1736404992 (+1477189632 for this command)
                         ----MB
#symtabs: 6838113 (+6809597), #primary symtabs: 3666 (+3655), #blocks: 22968 (+22943)
vs.
THIS PATCH ("thread apply all bt"):
Command execution time: 2.595000 (cpu), 2.607573 (wall)
                                        -sec
Space used: 340058112 (+85917696 for this command)
                        --MB
#symtabs: 42221 (+39041), #primary symtabs: 3666 (+3655), #blocks: 22968 (+22943)

FSF GDB HEAD ("thread apply all bt full"):
Command execution time: 466.751000 (cpu), 468.345837 (wall)
                                          ---sec
Space used: 2330132480 (+2070974464 for this command)
                         ----MB
#symtabs: 9586991 (+9558475), #primary symtabs: 5367 (+5356), #blocks: 31638 (+31613)
vs.
THIS PATCH ("thread apply all bt full"):
Command execution time: 18.907000 (cpu), 18.964125 (wall)
                                         --sec
Space used: 364462080 (+110325760 for this command)
                        ---MB
#symtabs: 58780 (+55600), #primary symtabs: 5367 (+5356), #blocks: 31638 (+31613)

[-- Attachment #2: 1 --]
[-- Type: text/plain, Size: 5655 bytes --]

gdb/
2014-10-01  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix 100x slowdown regression on DWZ files.
	* dwarf2read.c (struct dwarf2_per_objfile): Add lineinfo_hash.
	(struct dwarf2_lineinfo, dwarf2_lineinfo_hash, dwarf2_lineinfo_eq): New.
	(struct dwarf2_cu): Add lineinfo.
	(handle_DW_AT_stmt_list): Use dwarf2_per_objfile->lineinfo_hash, set
	cu->lineinfo.
	(new_symbol_full): Use cu->lineinfo.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 9d0ee13..b24c133 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -309,8 +309,51 @@ struct dwarf2_per_objfile
 
   /* The CUs we recently read.  */
   VEC (dwarf2_per_cu_ptr) *just_read_cus;
+
+  /* Table containing struct dwarf2_lineinfo.  */
+  htab_t lineinfo_hash;
+};
+
+/* Table mapping .debug_line offsets to any symtab where they are
+   instantiated.  */
+
+struct dwarf2_lineinfo
+{
+  /* Offset of line number information in .debug_line section.  */
+  sect_offset offset;
+  unsigned offset_in_dwz : 1;
+
+  /* Number of entries in file_to_symtab array.  */
+  unsigned file_to_symtab_count;
+
+  /* struct is sized to contain FILE_TO_SYMTAB_COUNT elements of this array.
+     Map each DW_AT_decl_file entry to any instantiation of matching symtab.
+     This array is numbered from zero, DW_AT_decl_file is numbered from one.  */
+  struct symtab *file_to_symtab[1];
 };
 
+/* Hash function for a dwarf2_lineinfo.  */
+
+static hashval_t
+dwarf2_lineinfo_hash (const void *item)
+{
+  const struct dwarf2_lineinfo *ofs = item;
+
+  return ofs->offset.sect_off ^ ofs->offset_in_dwz;
+}
+
+/* Equality function for a dwarf2_lineinfo.  */
+
+static int
+dwarf2_lineinfo_eq (const void *item_lhs, const void *item_rhs)
+{
+  const struct dwarf2_lineinfo *ofs_lhs = item_lhs;
+  const struct dwarf2_lineinfo *ofs_rhs = item_rhs;
+
+  return (ofs_lhs->offset.sect_off == ofs_rhs->offset.sect_off
+	  && ofs_lhs->offset_in_dwz == ofs_rhs->offset_in_dwz);
+}
+
 static struct dwarf2_per_objfile *dwarf2_per_objfile;
 
 /* Default names of the debugging sections.  */
@@ -489,6 +532,10 @@ struct dwarf2_cu
   /* Header data from the line table, during full symbol processing.  */
   struct line_header *line_header;
 
+  /* Table mapping .debug_line offsets to any symtab where they are
+     instantiated.  It contains subset of LINE_HEADER information.  */
+  struct dwarf2_lineinfo *lineinfo;
+
   /* A list of methods which need to have physnames computed
      after all type information has been read.  */
   VEC (delayed_method_info) *method_list;
@@ -8975,24 +9022,61 @@ static void
 handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 			const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
 {
+  struct objfile *objfile = dwarf2_per_objfile->objfile;
   struct attribute *attr;
+  unsigned int line_offset;
+  struct dwarf2_lineinfo lineinfo;
+  struct line_header *line_header;
+  unsigned u;
+  void **slot;
 
   gdb_assert (! cu->per_cu->is_debug_types);
 
   attr = dwarf2_attr (die, DW_AT_stmt_list, cu);
-  if (attr)
+  if (attr == NULL)
+    return;
+
+  line_offset = DW_UNSND (attr);
+
+  if (dwarf2_per_objfile->lineinfo_hash == NULL)
     {
-      unsigned int line_offset = DW_UNSND (attr);
-      struct line_header *line_header
-	= dwarf_decode_line_header (line_offset, cu);
+      dwarf2_per_objfile->lineinfo_hash =
+	htab_create_alloc_ex (127, dwarf2_lineinfo_hash, dwarf2_lineinfo_eq,
+			      NULL, &objfile->objfile_obstack,
+			      hashtab_obstack_allocate,
+			      dummy_obstack_deallocate);
+    }
 
-      if (line_header)
-	{
-	  cu->line_header = line_header;
-	  make_cleanup (free_cu_line_header, cu);
-	  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
-	}
+  lineinfo.offset.sect_off = line_offset;
+  lineinfo.offset_in_dwz = cu->per_cu->is_dwz;
+  slot = htab_find_slot (dwarf2_per_objfile->lineinfo_hash, &lineinfo, INSERT);
+
+  /* For DW_TAG_compile_unit we need info like symtab::linetable which
+     is not present in *SLOT.  */
+  if (die->tag == DW_TAG_partial_unit && *slot != NULL)
+    {
+      cu->lineinfo = *slot;
+      return;
     }
+
+  line_header = dwarf_decode_line_header (line_offset, cu);
+  if (line_header == NULL)
+    return;
+  cu->line_header = line_header;
+  make_cleanup (free_cu_line_header, cu);
+  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
+
+  cu->lineinfo = obstack_alloc (&objfile->objfile_obstack,
+				(sizeof (*cu->lineinfo)
+				 + (sizeof (*cu->lineinfo->file_to_symtab)
+				    * (line_header->num_file_names - 1))));
+  cu->lineinfo->offset.sect_off = line_offset;
+  cu->lineinfo->offset_in_dwz = cu->per_cu->is_dwz;
+  cu->lineinfo->file_to_symtab_count = line_header->num_file_names;
+  for (u = 0; u < cu->lineinfo->file_to_symtab_count; u++)
+    cu->lineinfo->file_to_symtab[u] = line_header->file_names[u].symtab;
+  if (*slot == NULL)
+    *slot = cu->lineinfo;
 }
 
 /* Process DW_TAG_compile_unit or DW_TAG_partial_unit.  */
@@ -17870,17 +17954,12 @@ new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 	{
 	  int file_index = DW_UNSND (attr);
 
-	  if (cu->line_header == NULL
-	      || file_index > cu->line_header->num_file_names)
+	  if (cu->lineinfo == NULL
+	      || file_index > cu->lineinfo->file_to_symtab_count)
 	    complaint (&symfile_complaints,
 		       _("file index out of range"));
 	  else if (file_index > 0)
-	    {
-	      struct file_entry *fe;
-
-	      fe = &cu->line_header->file_names[file_index - 1];
-	      SYMBOL_SYMTAB (sym) = fe->symtab;
-	    }
+	    SYMBOL_SYMTAB (sym) = cu->lineinfo->file_to_symtab[file_index - 1];
 	}
 
       switch (die->tag)

Re: [patch] Fix 100x slowdown regression on DWZ files

[Doug Evans]

Jan Kratochvil writes:
 > Hi,
 > 
 > Bug 16405 - backtrace takes GBs and minutes with dwz -m (edit) 
 > https://sourceware.org/bugzilla/show_bug.cgi?id=16405
 > 
 > Since Fedora started to use DWZ DWARF compressor:
 > 	http://fedoraproject.org/wiki/Features/DwarfCompressor
 > GDB has slowed down a lot.  To make it clear - DWZ is DWARF structure
 > rearrangement, "compressor" does not mean any zlib style data compression.
 > 
 > This patch reduces LibreOffice backtrace from 5 minutes to 3 seconds (100x)
 > and it also reduces memory consumption 20x.
 > [ benchmark is at the bottom of this mail ]
 > 
 > Example of DWZ output:
 > ------------------------------------------------------------------------------
 >   Compilation Unit @ offset 0xc4:
 >  <0><cf>: Abbrev Number: 17 (DW_TAG_partial_unit)
 >     <d0>   DW_AT_stmt_list   : 0x0
 >     <d4>   DW_AT_comp_dir    : (indirect string, offset: 0x6f): /usr/src/debug/gdb-7.7.1/build-x86_64-redhat-linux-gnu/gdb
 >  <1><d8>: Abbrev Number: 9 (DW_TAG_typedef)
 >     <d9>   DW_AT_name        : (indirect string, offset: 0x827dc): size_t
 >     <dd>   DW_AT_decl_file   : 4
 >     <de>   DW_AT_decl_line   : 212
 >     <df>   DW_AT_type        : <0xae>
 > 
 >   Compilation Unit @ offset 0xe4:
 >  <0><ef>: Abbrev Number: 13 (DW_TAG_partial_unit)
 >     <f0>   DW_AT_stmt_list   : 0x0
 >     <f4>   DW_AT_comp_dir    : (indirect string, offset: 0x6f): /usr/src/debug/gdb-7.7.1/build-x86_64-redhat-linux-gnu/gdb
 >  <1><f8>: Abbrev Number: 45 (DW_TAG_typedef)
 >     <f9>   DW_AT_name        : (indirect string, offset: 0x251): __off_t
 >     <fd>   DW_AT_decl_file   : 3
 >     <fe>   DW_AT_decl_line   : 131
 >     <ff>   DW_AT_type        : <0x68>
 > 
 >   Compilation Unit @ offset 0x62d9f9:
 >  <0><62da04>: Abbrev Number: 20 (DW_TAG_compile_unit)
 > [...]
 >     <62da12>   DW_AT_low_pc      : 0x807e10
 >     <62da1a>   DW_AT_high_pc     : 134
 >     <62da1c>   DW_AT_stmt_list   : 0xf557e
 >  <1><62da20>: Abbrev Number: 7 (DW_TAG_imported_unit)
 >     <62da21>   DW_AT_import      : <0xcf>       [Abbrev Number: 17]
 > ------------------------------------------------------------------------------
 > 
 > One can see all DW_TAG_partial_unit have DW_AT_stmt_list 0x0 which causes
 > repeated decoding of that .debug_line unit on each DW_TAG_imported_unit.
 > 
 > This was OK before as each DW_TAG_compile_unit has its own .debug_line unit.
 > But since the introduction of DW_TAG_partial_unit by DWZ one should cache
 > read-in DW_AT_stmt_list .debug_line units.
 > 
 > Fortunately one does not need to cache whole
 > 	struct linetable *symtab->linetable
 > and other data from .debug_line mapping PC<->lines
 > ------------------------------------------------------------------------------
 >  Line Number Statements:
 >   Extended opcode 2: set Address to 0x45c880
 >   Advance Line by 25 to 26
 >   Copy
 > ------------------------------------------------------------------------------
 > as the only part of .debug_line which GDB needs for DW_TAG_partial_unit is:
 > ------------------------------------------------------------------------------
 >  The Directory Table:
 >   ../../gdb
 >   /usr/include/bits
 > [...]
 >  The File Name Table:
 >   Entry Dir     Time    Size    Name
 >   1     1       0       0       gdb.c
 >   2     2       0       0       string3.h
 > [...]
 > ------------------------------------------------------------------------------
 > specifically referenced in GDB for DW_AT_decl_file at a single place:
 > ------------------------------------------------------------------------------
 >               fe = &cu->line_header->file_names[file_index - 1];
 >               SYMBOL_SYMTAB (sym) = fe->symtab;
 > ------------------------------------------------------------------------------
 > 
 > This is because for some reason DW_TAG_partial_unit never contains PC-related
 > DWARF information.  I do not know exactly why, the compression ratio is a bit
 > lower due to it but thanksfully currently it is that way:
 > dwz.c:
 > ------------------------------------------------------------------------------
 >         /* These attributes reference code, prevent moving
 >            DIEs with them.  */
 >         case DW_AT_low_pc:
 >         case DW_AT_high_pc:
 >         case DW_AT_entry_pc:
 >         case DW_AT_ranges:
 >           die->die_ck_state = CK_BAD;
 > +
 >   /* State of checksum computation.  Not computed yet, computed and
 >      suitable for moving into partial units, currently being computed
 >      and finally determined unsuitable for moving into partial units.  */
 >   enum { CK_UNKNOWN, CK_KNOWN, CK_BEING_COMPUTED, CK_BAD } die_ck_state : 2;
 > ------------------------------------------------------------------------------
 > I have also verified also real-world Fedora debuginfo files really comply with
 > that assumption with dwgrep
 > 	https://github.com/pmachata/dwgrep
 > using:
 > ------------------------------------------------------------------------------
 > dwgrep -e 'entry ?DW_TAG_partial_unit child* ( ?DW_AT_low_pc , ?DW_AT_high_pc , ?DW_AT_ranges )' /usr/lib/debug/**
 > ------------------------------------------------------------------------------
 > 
 > BTW I think GDB already does not support the whole DW_TAG_imported_unit and
 > DW_TAG_partial_unit usage possibilities as specified by the DWARF standard.
 > I think GDB would not work if DW_TAG_imported_unit was used in some inner
 > level and not at the CU level (readelf -wi level <1>) - this is how DWZ is
 > using DW_TAG_imported_unit.  Therefore I do not think further assumptions
 > about DW_TAG_imported_unit and DW_TAG_partial_unit usage by DWZ are a problem
 > for GDB.
 > 
 > One could save the whole .debug_line decoded PC<->lines mapping (and not just
 > the DW_AT_decl_file table) but:
 >  * there are some problematic corner cases so one could do it incorrectly
 >  * there are no real world data to really test such patch extension
 >  * such extension could be done perfectly incrementally on top of this patch
 > 
 > One could save existing 'struct line_header' in the patch instead of
 > introducing new 'struct dwarf2_line_info'.  The problem is
 > dwarf_decode_line_header() currently uses xmalloc while we need the allocation
 > to be bound to objfile->objfile_obstack.  With 'struct line_header' one would
 > have to make special objfile-time destruction of those 'struct line_header'.
 > 
 > No regressions on {x86_64,x86_64-m32,i686}-fedora21pre-linux-gnu in DWZ mode
 > (contrib/cc-with-tweaks.sh -m, that is dwz -m).  I have seen there are several
 > GDB regressions in non-DWZ -> DWZ mode but that is off-topic for this patch.
 > 
 > 
 > Thanks,
 > Jan
 > 
 > ------------------------------------------------------------------------------
 > 
 > benchmark - on Fedora 20 x86_64 and FSF GDB HEAD:
 > echo -e 'thread apply all bt\nset confirm no\nq'|./gdb -p `pidof soffice.bin` -ex 'set pagination off' -ex 'maintenance set per-command space' -ex 'maintenance set per-command symtab' -ex 'maintenance set per-command time'
 > 
 > FSF GDB HEAD ("thread apply all bt"):
 > Command execution time: 333.693000 (cpu), 335.587539 (wall)
 >                                           ---sec
 > Space used: 1736404992 (+1477189632 for this command)
 >                          ----MB
 > #symtabs: 6838113 (+6809597), #primary symtabs: 3666 (+3655), #blocks: 22968 (+22943)
 > vs.
 > THIS PATCH ("thread apply all bt"):
 > Command execution time: 2.595000 (cpu), 2.607573 (wall)
 >                                         -sec
 > Space used: 340058112 (+85917696 for this command)
 >                         --MB
 > #symtabs: 42221 (+39041), #primary symtabs: 3666 (+3655), #blocks: 22968 (+22943)
 > 
 > FSF GDB HEAD ("thread apply all bt full"):
 > Command execution time: 466.751000 (cpu), 468.345837 (wall)
 >                                           ---sec
 > Space used: 2330132480 (+2070974464 for this command)
 >                          ----MB
 > #symtabs: 9586991 (+9558475), #primary symtabs: 5367 (+5356), #blocks: 31638 (+31613)
 > vs.
 > THIS PATCH ("thread apply all bt full"):
 > Command execution time: 18.907000 (cpu), 18.964125 (wall)
 >                                          --sec
 > Space used: 364462080 (+110325760 for this command)
 >                         ---MB
 > #symtabs: 58780 (+55600), #primary symtabs: 5367 (+5356), #blocks: 31638 (+31613)
 > gdb/
 > 2014-10-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
 > 
 > 	Fix 100x slowdown regression on DWZ files.
 > 	* dwarf2read.c (struct dwarf2_per_objfile): Add lineinfo_hash.
 > 	(struct dwarf2_lineinfo, dwarf2_lineinfo_hash, dwarf2_lineinfo_eq): New.
 > 	(struct dwarf2_cu): Add lineinfo.
 > 	(handle_DW_AT_stmt_list): Use dwarf2_per_objfile->lineinfo_hash, set
 > 	cu->lineinfo.
 > 	(new_symbol_full): Use cu->lineinfo.

Looks like dwz support is going through some of the pains I went through
with type units. :-)

I tested this patch with --target_board=dwarf4-gdb-index
and got a failure in m-static.exp:

info variable everywhere^M
All variables matching regular expression "everywhere":^M
^M
File :^M
const int gnu_obj_4::everywhere;^M
(gdb) FAIL: gdb.cp/m-static.exp: info variable everywhere

Type units read the line table in a separate path,
and when we get here in new_symbol_full:

          if (cu->lineinfo == NULL
              || file_index > cu->lineinfo->file_to_symtab_count)
            complaint (&symfile_complaints,
                       _("file index out of range"));

cu->lineinfo is NULL.

I wouldn't suggest trying too hard for type units and dwz support
to share code, but I think there is room for at least some
exploration. There's also room for some improvement in the type unit
support so if that happens that'd be great (though such improvement
is obviously *not* a requirement for this patch).

OTOH, I do want to avoid any confusion that this patch may inadvertently
introduce. For example, IIUC with your patch as is,
if we read a partial_unit first, before a compile_unit
that has the same stmt_list value, we'll do more processing in
dwarf_decode_lines than we really need to since we only need a file
number to symtab mapping. And if we later read in a compile_unit
with the same stmt_value we'll call dwarf_decode_lines again,
and this time we need the pc/line mapping it computes.
Whereas if we process these in the opposite order we'll only call
dwarf_decode_lines once. I'm sure this will be confusing at first
to some later developer going through this code.
[I could be missing something of course, and I'm happy for any corrections.]

The code that processes stmt_list for type_units is in setup_type_unit_groups.
Note that this code goes to the trouble of re-initializing the buildsym
machinery (see the calls to restart_symtab in dwarf2read.c) when we process
the second and subsequent type units that share a stmt_list value.
This is something that used to be done before your patch and will no
longer be done with your patch (since if we get a cache hit we exit).
It may be that the type_unit support is doing this unnecessarily,
which would be great because we can then simplify it.
The details have dropped out of cache so I'd have to go back and check
why it was done this way.

Further minor comments inline.

 > 
 > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
 > index 9d0ee13..b24c133 100644
 > --- a/gdb/dwarf2read.c
 > +++ b/gdb/dwarf2read.c
 > @@ -309,8 +309,51 @@ struct dwarf2_per_objfile
 >  
 >    /* The CUs we recently read.  */
 >    VEC (dwarf2_per_cu_ptr) *just_read_cus;
 > +
 > +  /* Table containing struct dwarf2_lineinfo.  */
 > +  htab_t lineinfo_hash;
 > +};
 > +
 > +/* Table mapping .debug_line offsets to any symtab where they are
 > +   instantiated.  */
 > +
 > +struct dwarf2_lineinfo
 > +{
 > +  /* Offset of line number information in .debug_line section.  */
 > +  sect_offset offset;
 > +  unsigned offset_in_dwz : 1;

IWBN to document why offset_in_dwz is here.
It's not obvious why it's needed.

 > +
 > +  /* Number of entries in file_to_symtab array.  */
 > +  unsigned file_to_symtab_count;
 > +
 > +  /* struct is sized to contain FILE_TO_SYMTAB_COUNT elements of this array.
 > +     Map each DW_AT_decl_file entry to any instantiation of matching symtab.
 > +     This array is numbered from zero, DW_AT_decl_file is numbered from one.  */
 > +  struct symtab *file_to_symtab[1];
 >  };
 >  
 > +/* Hash function for a dwarf2_lineinfo.  */
 > +
 > +static hashval_t
 > +dwarf2_lineinfo_hash (const void *item)
 > +{
 > +  const struct dwarf2_lineinfo *ofs = item;
 > +
 > +  return ofs->offset.sect_off ^ ofs->offset_in_dwz;
 > +}
 > +
 > +/* Equality function for a dwarf2_lineinfo.  */
 > +
 > +static int
 > +dwarf2_lineinfo_eq (const void *item_lhs, const void *item_rhs)
 > +{
 > +  const struct dwarf2_lineinfo *ofs_lhs = item_lhs;
 > +  const struct dwarf2_lineinfo *ofs_rhs = item_rhs;
 > +
 > +  return (ofs_lhs->offset.sect_off == ofs_rhs->offset.sect_off
 > +	  && ofs_lhs->offset_in_dwz == ofs_rhs->offset_in_dwz);
 > +}
 > +
 >  static struct dwarf2_per_objfile *dwarf2_per_objfile;
 >  
 >  /* Default names of the debugging sections.  */
 > @@ -489,6 +532,10 @@ struct dwarf2_cu
 >    /* Header data from the line table, during full symbol processing.  */
 >    struct line_header *line_header;
 >  
 > +  /* Table mapping .debug_line offsets to any symtab where they are
 > +     instantiated.  It contains subset of LINE_HEADER information.  */
 > +  struct dwarf2_lineinfo *lineinfo;
 > +
 >    /* A list of methods which need to have physnames computed
 >       after all type information has been read.  */
 >    VEC (delayed_method_info) *method_list;
 > @@ -8975,24 +9022,61 @@ static void
 >  handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 >  			const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
 >  {
 > +  struct objfile *objfile = dwarf2_per_objfile->objfile;
 >    struct attribute *attr;
 > +  unsigned int line_offset;
 > +  struct dwarf2_lineinfo lineinfo;
 > +  struct line_header *line_header;
 > +  unsigned u;
 > +  void **slot;
 >  
 >    gdb_assert (! cu->per_cu->is_debug_types);
 >  
 >    attr = dwarf2_attr (die, DW_AT_stmt_list, cu);
 > -  if (attr)
 > +  if (attr == NULL)
 > +    return;
 > +
 > +  line_offset = DW_UNSND (attr);
 > +
 > +  if (dwarf2_per_objfile->lineinfo_hash == NULL)
 >      {
 > -      unsigned int line_offset = DW_UNSND (attr);
 > -      struct line_header *line_header
 > -	= dwarf_decode_line_header (line_offset, cu);
 > +      dwarf2_per_objfile->lineinfo_hash =

As much as I prefer "=" going here, convention says to put it on the
next line.

 > +	htab_create_alloc_ex (127, dwarf2_lineinfo_hash, dwarf2_lineinfo_eq,

I don't have any data, but 127 seems high.
I wouldn't change it, I just wanted to ask if you have any data
guiding this choice.
[I'm not worried about spending a bit of extra space on small programs, btw.]

 > +			      NULL, &objfile->objfile_obstack,
 > +			      hashtab_obstack_allocate,
 > +			      dummy_obstack_deallocate);
 > +    }
 >  
 > -      if (line_header)
 > -	{
 > -	  cu->line_header = line_header;
 > -	  make_cleanup (free_cu_line_header, cu);
 > -	  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
 > -	}
 > +  lineinfo.offset.sect_off = line_offset;
 > +  lineinfo.offset_in_dwz = cu->per_cu->is_dwz;
 > +  slot = htab_find_slot (dwarf2_per_objfile->lineinfo_hash, &lineinfo, INSERT);
 > +
 > +  /* For DW_TAG_compile_unit we need info like symtab::linetable which
 > +     is not present in *SLOT.  */
 > +  if (die->tag == DW_TAG_partial_unit && *slot != NULL)
 > +    {
 > +      cu->lineinfo = *slot;
 > +      return;
 >      }
 > +
 > +  line_header = dwarf_decode_line_header (line_offset, cu);
 > +  if (line_header == NULL)
 > +    return;
 > +  cu->line_header = line_header;
 > +  make_cleanup (free_cu_line_header, cu);
 > +  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
 > +
 > +  cu->lineinfo = obstack_alloc (&objfile->objfile_obstack,
 > +				(sizeof (*cu->lineinfo)
 > +				 + (sizeof (*cu->lineinfo->file_to_symtab)
 > +				    * (line_header->num_file_names - 1))));
 > +  cu->lineinfo->offset.sect_off = line_offset;
 > +  cu->lineinfo->offset_in_dwz = cu->per_cu->is_dwz;
 > +  cu->lineinfo->file_to_symtab_count = line_header->num_file_names;
 > +  for (u = 0; u < cu->lineinfo->file_to_symtab_count; u++)
 > +    cu->lineinfo->file_to_symtab[u] = line_header->file_names[u].symtab;
 > +  if (*slot == NULL)
 > +    *slot = cu->lineinfo;
 >  }
 >  
 >  /* Process DW_TAG_compile_unit or DW_TAG_partial_unit.  */
 > @@ -17870,17 +17954,12 @@ new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 >  	{
 >  	  int file_index = DW_UNSND (attr);
 >  
 > -	  if (cu->line_header == NULL
 > -	      || file_index > cu->line_header->num_file_names)
 > +	  if (cu->lineinfo == NULL
 > +	      || file_index > cu->lineinfo->file_to_symtab_count)
 >  	    complaint (&symfile_complaints,
 >  		       _("file index out of range"));
 >  	  else if (file_index > 0)
 > -	    {
 > -	      struct file_entry *fe;
 > -
 > -	      fe = &cu->line_header->file_names[file_index - 1];
 > -	      SYMBOL_SYMTAB (sym) = fe->symtab;
 > -	    }
 > +	    SYMBOL_SYMTAB (sym) = cu->lineinfo->file_to_symtab[file_index - 1];
 >  	}
 >  
 >        switch (die->tag)

Re: [patch] Fix 100x slowdown regression on DWZ files

[Doug Evans]

On Wed, Oct 1, 2014 at 4:51 PM, Doug Evans <dje@google.com> wrote:
> [...]
> OTOH, I do want to avoid any confusion that this patch may inadvertently
> introduce. For example, IIUC with your patch as is,
> if we read a partial_unit first, before a compile_unit
> that has the same stmt_list value, we'll do more processing in
> dwarf_decode_lines than we really need to since we only need a file
> number to symtab mapping. And if we later read in a compile_unit
> with the same stmt_value we'll call dwarf_decode_lines again,
> and this time we need the pc/line mapping it computes.
> Whereas if we process these in the opposite order we'll only call
> dwarf_decode_lines once. I'm sure this will be confusing at first
> to some later developer going through this code.
> [I could be missing something of course, and I'm happy for any corrections.]

Ah, I guess the offset_in_dwz flag will ensure dwarf_decode_lines gets called
twice regardless of order.  But is that the only reason for the flag?

[patchv2] Fix 100x slowdown regression on DWZ files

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 4294 bytes --]

On Thu, 02 Oct 2014 01:51:38 +0200, Doug Evans wrote:
> I tested this patch with --target_board=dwarf4-gdb-index
> and got a failure in m-static.exp:

That is particularly with -fdebug-types-section.


> Type units read the line table in a separate path,

OK, therefore I dropped that separate struct dwarf2_lineinfo
and reused struct line_header instead.


> OTOH, I do want to avoid any confusion that this patch may inadvertently
> introduce. For example, IIUC with your patch as is,
> if we read a partial_unit first, before a compile_unit
> that has the same stmt_list value, we'll do more processing in
> dwarf_decode_lines than we really need to since we only need a file
> number to symtab mapping. And if we later read in a compile_unit
> with the same stmt_value we'll call dwarf_decode_lines again,
> and this time we need the pc/line mapping it computes.
> Whereas if we process these in the opposite order we'll only call
> dwarf_decode_lines once. I'm sure this will be confusing at first
> to some later developer going through this code.
> [I could be missing something of course, and I'm happy for any corrections.]

Implemented (omitting some story why I did not include it before).


> The code that processes stmt_list for type_units is in setup_type_unit_groups.
> Note that this code goes to the trouble of re-initializing the buildsym
> machinery (see the calls to restart_symtab in dwarf2read.c) when we process
> the second and subsequent type units that share a stmt_list value.
> This is something that used to be done before your patch and will no
> longer be done with your patch (since if we get a cache hit we exit).
> It may be that the type_unit support is doing this unnecessarily,
> which would be great because we can then simplify it.

I hope this patch should no longer break -fdebug-types-section.
If it additionally enables some future optimization for -fdebug-types-section
the better.


>  > +  /* Offset of line number information in .debug_line section.  */
>  > +  sect_offset offset;
>  > +  unsigned offset_in_dwz : 1;
> 
> IWBN to document why offset_in_dwz is here.
> It's not obvious why it's needed.
+
On Thu, 02 Oct 2014 01:57:03 +0200, Doug Evans wrote:
> Ah, I guess the offset_in_dwz flag will ensure dwarf_decode_lines gets called
> twice regardless of order.  But is that the only reason for the flag?

I have added there now:
+  /* OFFSET is for struct dwz_file associated with dwarf2_per_objfile.  */

If one removes it regressions really happen.  What happens is that this
line_header_hash (former lineinfo_hash) is in struct dwarf2_per_objfile which
is common for both objfile and its objfile.dwz (that one is normally in
/usr/lib/debug/.dwz/ - common for multiple objfiles).  And there are two
different DIEs at offset 0xb - one in objfile and one in objfile.dwz - which
would match single line_header if offset_in_dwz was not there.

Also existing dwarf2read.c code usually transfers "dwz flag" together with DIE
offset, such as:
	dwarf2_find_containing_comp_unit (sect_offset offset,
					  unsigned int offset_in_dwz,
					  struct objfile *objfile)
This reminds me - why doesn't similar ambiguity happen also for dwp_file?
I am unfortunately not much aware of the dwp implementation details.


>  > -      struct line_header *line_header
>  > -	= dwarf_decode_line_header (line_offset, cu);
>  > +      dwarf2_per_objfile->lineinfo_hash =
> 
> As much as I prefer "=" going here, convention says to put it on the
> next line.

I have changed it but this was just blind copy from existing line 21818.


>  > +	htab_create_alloc_ex (127, dwarf2_lineinfo_hash, dwarf2_lineinfo_eq,
> 
> I don't have any data, but 127 seems high.

I have not changed it but this was just blind copy from existing line 21818.


> I wouldn't change it, I just wanted to ask if you have any data
> guiding this choice.

Tuning some constants really makes no sense when GDB has missing + insanely
complicated data structures and in consequence GDB is using inappropriate data
structures with bad algorithmic complexity.  One needs to switch GDB to C++
and its STL before one can start talking about data structures performance.


No regressions on {x86_64,x86_64-m32,i686}-fedora20-linux-gnu in DWZ mode and
in -fdebug-types-section mode.


Thanks,
Jan

[-- Attachment #2: partialunit5.patch --]
[-- Type: text/plain, Size: 7429 bytes --]

gdb/
2014-10-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix 100x slowdown regression on DWZ files.
	* dwarf2read.c (struct dwarf2_per_objfile): Add line_header_hash.
	(struct line_header): Add offset and offset_in_dwz.
	(dwarf_decode_lines): Add parameter decode_mapping to the declaration.
	(free_line_header_voidp): New declaration.
	(line_header_hash, line_header_eq): New functions.
	(dwarf2_build_include_psymtabs): Update dwarf_decode_lines caller.
	(handle_DW_AT_stmt_list): Use dwarf2_per_objfile->line_header_hash.
	(free_line_header_voidp): New function.
	(dwarf_decode_line_header): Initialize offset and offset_in_dwz.
	(dwarf_decode_lines): New parameter decode_mapping, use it.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 9d0ee13..206dc10 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -309,6 +309,9 @@ struct dwarf2_per_objfile
 
   /* The CUs we recently read.  */
   VEC (dwarf2_per_cu_ptr) *just_read_cus;
+
+  /* Table containing line_header indexed by offset and offset_in_dwz.  */
+  htab_t line_header_hash;
 };
 
 static struct dwarf2_per_objfile *dwarf2_per_objfile;
@@ -1025,6 +1028,12 @@ typedef void (die_reader_func_ftype) (const struct die_reader_specs *reader,
    which contains the following information.  */
 struct line_header
 {
+  /* Offset of line number information in .debug_line section.  */
+  sect_offset offset;
+
+  /* OFFSET is for struct dwz_file associated with dwarf2_per_objfile.  */
+  unsigned offset_in_dwz : 1;
+
   unsigned int total_length;
   unsigned short version;
   unsigned int header_length;
@@ -1513,7 +1522,7 @@ static struct line_header *dwarf_decode_line_header (unsigned int offset,
 
 static void dwarf_decode_lines (struct line_header *, const char *,
 				struct dwarf2_cu *, struct partial_symtab *,
-				CORE_ADDR);
+				CORE_ADDR, int decode_mapping);
 
 static void dwarf2_start_subfile (const char *, const char *, const char *);
 
@@ -1845,6 +1854,8 @@ static void free_dwo_file_cleanup (void *);
 static void process_cu_includes (void);
 
 static void check_producer (struct dwarf2_cu *cu);
+
+static void free_line_header_voidp (void *arg);
 \f
 /* Various complaints about symbol reading that don't abort the process.  */
 
@@ -1911,6 +1922,29 @@ dwarf2_invalid_attrib_class_complaint (const char *arg1, const char *arg2)
 	     _("invalid attribute class or form for '%s' in '%s'"),
 	     arg1, arg2);
 }
+
+/* Hash function for line_header_hash.  */
+
+static hashval_t
+line_header_hash (const void *item)
+{
+  const struct line_header *ofs = item;
+
+  return ofs->offset.sect_off ^ ofs->offset_in_dwz;
+}
+
+/* Equality function for line_header_hash.  */
+
+static int
+line_header_eq (const void *item_lhs, const void *item_rhs)
+{
+  const struct line_header *ofs_lhs = item_lhs;
+  const struct line_header *ofs_rhs = item_rhs;
+
+  return (ofs_lhs->offset.sect_off == ofs_rhs->offset.sect_off
+	  && ofs_lhs->offset_in_dwz == ofs_rhs->offset_in_dwz);
+}
+
 \f
 #if WORDS_BIGENDIAN
 
@@ -4449,7 +4483,7 @@ dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
     return;  /* No linetable, so no includes.  */
 
   /* NOTE: pst->dirname is DW_AT_comp_dir (if present).  */
-  dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow);
+  dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow, 1);
 
   free_line_header (lh);
 }
@@ -8975,24 +9009,64 @@ static void
 handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 			const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
 {
+  struct objfile *objfile = dwarf2_per_objfile->objfile;
   struct attribute *attr;
+  unsigned int line_offset;
+  struct line_header *line_header, line_header_local;
+  unsigned u;
+  void **slot;
+  int decode_mapping;
 
   gdb_assert (! cu->per_cu->is_debug_types);
 
   attr = dwarf2_attr (die, DW_AT_stmt_list, cu);
-  if (attr)
+  if (attr == NULL)
+    return;
+
+  line_offset = DW_UNSND (attr);
+
+  if (dwarf2_per_objfile->line_header_hash == NULL)
     {
-      unsigned int line_offset = DW_UNSND (attr);
-      struct line_header *line_header
-	= dwarf_decode_line_header (line_offset, cu);
+      dwarf2_per_objfile->line_header_hash
+	= htab_create_alloc_ex (127, line_header_hash, line_header_eq,
+				free_line_header_voidp,
+				&objfile->objfile_obstack,
+				hashtab_obstack_allocate,
+				dummy_obstack_deallocate);
+    }
 
-      if (line_header)
-	{
-	  cu->line_header = line_header;
-	  make_cleanup (free_cu_line_header, cu);
-	  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
-	}
+  line_header_local.offset.sect_off = line_offset;
+  line_header_local.offset_in_dwz = cu->per_cu->is_dwz;
+  slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
+			 &line_header_local, NO_INSERT);
+
+  /* For DW_TAG_compile_unit we need info like symtab::linetable which
+     is not present in *SLOT.  */
+  if (die->tag == DW_TAG_partial_unit && slot != NULL)
+    {
+      gdb_assert (*slot != NULL);
+      cu->line_header = *slot;
+      return;
+    }
+
+  line_header = dwarf_decode_line_header (line_offset, cu);
+  if (line_header == NULL)
+    return;
+  cu->line_header = line_header;
+
+  slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
+			 &line_header_local, INSERT);
+  gdb_assert (slot != NULL);
+  if (*slot == NULL)
+    *slot = line_header;
+  else
+    {
+      gdb_assert (die->tag != DW_TAG_partial_unit);
+      make_cleanup (free_cu_line_header, cu);
     }
+  decode_mapping = (die->tag != DW_TAG_partial_unit);
+  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc,
+		      decode_mapping);
 }
 
 /* Process DW_TAG_compile_unit or DW_TAG_partial_unit.  */
@@ -16863,6 +16937,16 @@ free_line_header (struct line_header *lh)
   xfree (lh);
 }
 
+/* Stub for free_line_header to match void * callback types.  */
+
+static void
+free_line_header_voidp (void *arg)
+{
+  struct line_header *lh = arg;
+
+  free_line_header (lh);
+}
+
 /* Add an entry to LH's include directory table.  */
 
 static void
@@ -16993,6 +17077,9 @@ dwarf_decode_line_header (unsigned int offset, struct dwarf2_cu *cu)
   back_to = make_cleanup ((make_cleanup_ftype *) free_line_header,
                           (void *) lh);
 
+  lh->offset.sect_off = offset;
+  lh->offset_in_dwz = cu->per_cu->is_dwz;
+
   line_ptr = section->buffer + offset;
 
   /* Read in the header.  */
@@ -17605,18 +17692,23 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
    E.g. expand_line_sal requires this when finding psymtabs to expand.
    A good testcase for this is mb-inline.exp.
 
-   LOWPC is the lowest address in CU (or 0 if not known).  */
+   LOWPC is the lowest address in CU (or 0 if not known).
+
+   Boolean DECODE_MAPPING specifies we need to fully decode .debug_line
+   for its PC<->lines mapping information.  Otherwise only filenames
+   tables is read in.  */
 
 static void
 dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
 		    struct dwarf2_cu *cu, struct partial_symtab *pst,
-		    CORE_ADDR lowpc)
+		    CORE_ADDR lowpc, int decode_mapping)
 {
   struct objfile *objfile = cu->objfile;
   const int decode_for_pst_p = (pst != NULL);
   struct subfile *first_subfile = current_subfile;
 
-  dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p, lowpc);
+  if (decode_mapping)
+    dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p, lowpc);
 
   if (decode_for_pst_p)
     {

ping: [patchv2] Fix 100x slowdown regression on DWZ files

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 4432 bytes --]

On Thu, 02 Oct 2014 17:56:53 +0200, Jan Kratochvil wrote:
------------------------------------------------------------------------------

On Thu, 02 Oct 2014 01:51:38 +0200, Doug Evans wrote:
> I tested this patch with --target_board=dwarf4-gdb-index
> and got a failure in m-static.exp:

That is particularly with -fdebug-types-section.


> Type units read the line table in a separate path,

OK, therefore I dropped that separate struct dwarf2_lineinfo
and reused struct line_header instead.


> OTOH, I do want to avoid any confusion that this patch may inadvertently
> introduce. For example, IIUC with your patch as is,
> if we read a partial_unit first, before a compile_unit
> that has the same stmt_list value, we'll do more processing in
> dwarf_decode_lines than we really need to since we only need a file
> number to symtab mapping. And if we later read in a compile_unit
> with the same stmt_value we'll call dwarf_decode_lines again,
> and this time we need the pc/line mapping it computes.
> Whereas if we process these in the opposite order we'll only call
> dwarf_decode_lines once. I'm sure this will be confusing at first
> to some later developer going through this code.
> [I could be missing something of course, and I'm happy for any corrections.]

Implemented (omitting some story why I did not include it before).


> The code that processes stmt_list for type_units is in setup_type_unit_groups.
> Note that this code goes to the trouble of re-initializing the buildsym
> machinery (see the calls to restart_symtab in dwarf2read.c) when we process
> the second and subsequent type units that share a stmt_list value.
> This is something that used to be done before your patch and will no
> longer be done with your patch (since if we get a cache hit we exit).
> It may be that the type_unit support is doing this unnecessarily,
> which would be great because we can then simplify it.

I hope this patch should no longer break -fdebug-types-section.
If it additionally enables some future optimization for -fdebug-types-section
the better.


>  > +  /* Offset of line number information in .debug_line section.  */
>  > +  sect_offset offset;
>  > +  unsigned offset_in_dwz : 1;
> 
> IWBN to document why offset_in_dwz is here.
> It's not obvious why it's needed.
+
On Thu, 02 Oct 2014 01:57:03 +0200, Doug Evans wrote:
> Ah, I guess the offset_in_dwz flag will ensure dwarf_decode_lines gets called
> twice regardless of order.  But is that the only reason for the flag?

I have added there now:
+  /* OFFSET is for struct dwz_file associated with dwarf2_per_objfile.  */

If one removes it regressions really happen.  What happens is that this
line_header_hash (former lineinfo_hash) is in struct dwarf2_per_objfile which
is common for both objfile and its objfile.dwz (that one is normally in
/usr/lib/debug/.dwz/ - common for multiple objfiles).  And there are two
different DIEs at offset 0xb - one in objfile and one in objfile.dwz - which
would match single line_header if offset_in_dwz was not there.

Also existing dwarf2read.c code usually transfers "dwz flag" together with DIE
offset, such as:
	dwarf2_find_containing_comp_unit (sect_offset offset,
					  unsigned int offset_in_dwz,
					  struct objfile *objfile)
This reminds me - why doesn't similar ambiguity happen also for dwp_file?
I am unfortunately not much aware of the dwp implementation details.


>  > -      struct line_header *line_header
>  > -	= dwarf_decode_line_header (line_offset, cu);
>  > +      dwarf2_per_objfile->lineinfo_hash =
> 
> As much as I prefer "=" going here, convention says to put it on the
> next line.

I have changed it but this was just blind copy from existing line 21818.


>  > +	htab_create_alloc_ex (127, dwarf2_lineinfo_hash, dwarf2_lineinfo_eq,
> 
> I don't have any data, but 127 seems high.

I have not changed it but this was just blind copy from existing line 21818.


> I wouldn't change it, I just wanted to ask if you have any data
> guiding this choice.

Tuning some constants really makes no sense when GDB has missing + insanely
complicated data structures and in consequence GDB is using inappropriate data
structures with bad algorithmic complexity.  One needs to switch GDB to C++
and its STL before one can start talking about data structures performance.


No regressions on {x86_64,x86_64-m32,i686}-fedora20-linux-gnu in DWZ mode and
in -fdebug-types-section mode.


Thanks,
Jan

[-- Attachment #2: partialunit5.patch --]
[-- Type: text/plain, Size: 7429 bytes --]

gdb/
2014-10-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix 100x slowdown regression on DWZ files.
	* dwarf2read.c (struct dwarf2_per_objfile): Add line_header_hash.
	(struct line_header): Add offset and offset_in_dwz.
	(dwarf_decode_lines): Add parameter decode_mapping to the declaration.
	(free_line_header_voidp): New declaration.
	(line_header_hash, line_header_eq): New functions.
	(dwarf2_build_include_psymtabs): Update dwarf_decode_lines caller.
	(handle_DW_AT_stmt_list): Use dwarf2_per_objfile->line_header_hash.
	(free_line_header_voidp): New function.
	(dwarf_decode_line_header): Initialize offset and offset_in_dwz.
	(dwarf_decode_lines): New parameter decode_mapping, use it.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 9d0ee13..206dc10 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -309,6 +309,9 @@ struct dwarf2_per_objfile
 
   /* The CUs we recently read.  */
   VEC (dwarf2_per_cu_ptr) *just_read_cus;
+
+  /* Table containing line_header indexed by offset and offset_in_dwz.  */
+  htab_t line_header_hash;
 };
 
 static struct dwarf2_per_objfile *dwarf2_per_objfile;
@@ -1025,6 +1028,12 @@ typedef void (die_reader_func_ftype) (const struct die_reader_specs *reader,
    which contains the following information.  */
 struct line_header
 {
+  /* Offset of line number information in .debug_line section.  */
+  sect_offset offset;
+
+  /* OFFSET is for struct dwz_file associated with dwarf2_per_objfile.  */
+  unsigned offset_in_dwz : 1;
+
   unsigned int total_length;
   unsigned short version;
   unsigned int header_length;
@@ -1513,7 +1522,7 @@ static struct line_header *dwarf_decode_line_header (unsigned int offset,
 
 static void dwarf_decode_lines (struct line_header *, const char *,
 				struct dwarf2_cu *, struct partial_symtab *,
-				CORE_ADDR);
+				CORE_ADDR, int decode_mapping);
 
 static void dwarf2_start_subfile (const char *, const char *, const char *);
 
@@ -1845,6 +1854,8 @@ static void free_dwo_file_cleanup (void *);
 static void process_cu_includes (void);
 
 static void check_producer (struct dwarf2_cu *cu);
+
+static void free_line_header_voidp (void *arg);
 \f
 /* Various complaints about symbol reading that don't abort the process.  */
 
@@ -1911,6 +1922,29 @@ dwarf2_invalid_attrib_class_complaint (const char *arg1, const char *arg2)
 	     _("invalid attribute class or form for '%s' in '%s'"),
 	     arg1, arg2);
 }
+
+/* Hash function for line_header_hash.  */
+
+static hashval_t
+line_header_hash (const void *item)
+{
+  const struct line_header *ofs = item;
+
+  return ofs->offset.sect_off ^ ofs->offset_in_dwz;
+}
+
+/* Equality function for line_header_hash.  */
+
+static int
+line_header_eq (const void *item_lhs, const void *item_rhs)
+{
+  const struct line_header *ofs_lhs = item_lhs;
+  const struct line_header *ofs_rhs = item_rhs;
+
+  return (ofs_lhs->offset.sect_off == ofs_rhs->offset.sect_off
+	  && ofs_lhs->offset_in_dwz == ofs_rhs->offset_in_dwz);
+}
+
 \f
 #if WORDS_BIGENDIAN
 
@@ -4449,7 +4483,7 @@ dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
     return;  /* No linetable, so no includes.  */
 
   /* NOTE: pst->dirname is DW_AT_comp_dir (if present).  */
-  dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow);
+  dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow, 1);
 
   free_line_header (lh);
 }
@@ -8975,24 +9009,64 @@ static void
 handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 			const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
 {
+  struct objfile *objfile = dwarf2_per_objfile->objfile;
   struct attribute *attr;
+  unsigned int line_offset;
+  struct line_header *line_header, line_header_local;
+  unsigned u;
+  void **slot;
+  int decode_mapping;
 
   gdb_assert (! cu->per_cu->is_debug_types);
 
   attr = dwarf2_attr (die, DW_AT_stmt_list, cu);
-  if (attr)
+  if (attr == NULL)
+    return;
+
+  line_offset = DW_UNSND (attr);
+
+  if (dwarf2_per_objfile->line_header_hash == NULL)
     {
-      unsigned int line_offset = DW_UNSND (attr);
-      struct line_header *line_header
-	= dwarf_decode_line_header (line_offset, cu);
+      dwarf2_per_objfile->line_header_hash
+	= htab_create_alloc_ex (127, line_header_hash, line_header_eq,
+				free_line_header_voidp,
+				&objfile->objfile_obstack,
+				hashtab_obstack_allocate,
+				dummy_obstack_deallocate);
+    }
 
-      if (line_header)
-	{
-	  cu->line_header = line_header;
-	  make_cleanup (free_cu_line_header, cu);
-	  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
-	}
+  line_header_local.offset.sect_off = line_offset;
+  line_header_local.offset_in_dwz = cu->per_cu->is_dwz;
+  slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
+			 &line_header_local, NO_INSERT);
+
+  /* For DW_TAG_compile_unit we need info like symtab::linetable which
+     is not present in *SLOT.  */
+  if (die->tag == DW_TAG_partial_unit && slot != NULL)
+    {
+      gdb_assert (*slot != NULL);
+      cu->line_header = *slot;
+      return;
+    }
+
+  line_header = dwarf_decode_line_header (line_offset, cu);
+  if (line_header == NULL)
+    return;
+  cu->line_header = line_header;
+
+  slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
+			 &line_header_local, INSERT);
+  gdb_assert (slot != NULL);
+  if (*slot == NULL)
+    *slot = line_header;
+  else
+    {
+      gdb_assert (die->tag != DW_TAG_partial_unit);
+      make_cleanup (free_cu_line_header, cu);
     }
+  decode_mapping = (die->tag != DW_TAG_partial_unit);
+  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc,
+		      decode_mapping);
 }
 
 /* Process DW_TAG_compile_unit or DW_TAG_partial_unit.  */
@@ -16863,6 +16937,16 @@ free_line_header (struct line_header *lh)
   xfree (lh);
 }
 
+/* Stub for free_line_header to match void * callback types.  */
+
+static void
+free_line_header_voidp (void *arg)
+{
+  struct line_header *lh = arg;
+
+  free_line_header (lh);
+}
+
 /* Add an entry to LH's include directory table.  */
 
 static void
@@ -16993,6 +17077,9 @@ dwarf_decode_line_header (unsigned int offset, struct dwarf2_cu *cu)
   back_to = make_cleanup ((make_cleanup_ftype *) free_line_header,
                           (void *) lh);
 
+  lh->offset.sect_off = offset;
+  lh->offset_in_dwz = cu->per_cu->is_dwz;
+
   line_ptr = section->buffer + offset;
 
   /* Read in the header.  */
@@ -17605,18 +17692,23 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
    E.g. expand_line_sal requires this when finding psymtabs to expand.
    A good testcase for this is mb-inline.exp.
 
-   LOWPC is the lowest address in CU (or 0 if not known).  */
+   LOWPC is the lowest address in CU (or 0 if not known).
+
+   Boolean DECODE_MAPPING specifies we need to fully decode .debug_line
+   for its PC<->lines mapping information.  Otherwise only filenames
+   tables is read in.  */
 
 static void
 dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
 		    struct dwarf2_cu *cu, struct partial_symtab *pst,
-		    CORE_ADDR lowpc)
+		    CORE_ADDR lowpc, int decode_mapping)
 {
   struct objfile *objfile = cu->objfile;
   const int decode_for_pst_p = (pst != NULL);
   struct subfile *first_subfile = current_subfile;
 
-  dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p, lowpc);
+  if (decode_mapping)
+    dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p, lowpc);
 
   if (decode_for_pst_p)
     {

ping^2: [patchv2] Fix 100x slowdown regression on DWZ files

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 4353 bytes --]

On Thu, 02 Oct 2014 17:56:53 +0200, Jan Kratochvil wrote:

On Thu, 02 Oct 2014 01:51:38 +0200, Doug Evans wrote:
> I tested this patch with --target_board=dwarf4-gdb-index
> and got a failure in m-static.exp:

That is particularly with -fdebug-types-section.


> Type units read the line table in a separate path,

OK, therefore I dropped that separate struct dwarf2_lineinfo
and reused struct line_header instead.


> OTOH, I do want to avoid any confusion that this patch may inadvertently
> introduce. For example, IIUC with your patch as is,
> if we read a partial_unit first, before a compile_unit
> that has the same stmt_list value, we'll do more processing in
> dwarf_decode_lines than we really need to since we only need a file
> number to symtab mapping. And if we later read in a compile_unit
> with the same stmt_value we'll call dwarf_decode_lines again,
> and this time we need the pc/line mapping it computes.
> Whereas if we process these in the opposite order we'll only call
> dwarf_decode_lines once. I'm sure this will be confusing at first
> to some later developer going through this code.
> [I could be missing something of course, and I'm happy for any corrections.]

Implemented (omitting some story why I did not include it before).


> The code that processes stmt_list for type_units is in setup_type_unit_groups.
> Note that this code goes to the trouble of re-initializing the buildsym
> machinery (see the calls to restart_symtab in dwarf2read.c) when we process
> the second and subsequent type units that share a stmt_list value.
> This is something that used to be done before your patch and will no
> longer be done with your patch (since if we get a cache hit we exit).
> It may be that the type_unit support is doing this unnecessarily,
> which would be great because we can then simplify it.

I hope this patch should no longer break -fdebug-types-section.
If it additionally enables some future optimization for -fdebug-types-section
the better.


>  > +  /* Offset of line number information in .debug_line section.  */
>  > +  sect_offset offset;
>  > +  unsigned offset_in_dwz : 1;
> 
> IWBN to document why offset_in_dwz is here.
> It's not obvious why it's needed.
+
On Thu, 02 Oct 2014 01:57:03 +0200, Doug Evans wrote:
> Ah, I guess the offset_in_dwz flag will ensure dwarf_decode_lines gets called
> twice regardless of order.  But is that the only reason for the flag?

I have added there now:
+  /* OFFSET is for struct dwz_file associated with dwarf2_per_objfile.  */

If one removes it regressions really happen.  What happens is that this
line_header_hash (former lineinfo_hash) is in struct dwarf2_per_objfile which
is common for both objfile and its objfile.dwz (that one is normally in
/usr/lib/debug/.dwz/ - common for multiple objfiles).  And there are two
different DIEs at offset 0xb - one in objfile and one in objfile.dwz - which
would match single line_header if offset_in_dwz was not there.

Also existing dwarf2read.c code usually transfers "dwz flag" together with DIE
offset, such as:
	dwarf2_find_containing_comp_unit (sect_offset offset,
					  unsigned int offset_in_dwz,
					  struct objfile *objfile)
This reminds me - why doesn't similar ambiguity happen also for dwp_file?
I am unfortunately not much aware of the dwp implementation details.


>  > -      struct line_header *line_header
>  > -	= dwarf_decode_line_header (line_offset, cu);
>  > +      dwarf2_per_objfile->lineinfo_hash =
> 
> As much as I prefer "=" going here, convention says to put it on the
> next line.

I have changed it but this was just blind copy from existing line 21818.


>  > +	htab_create_alloc_ex (127, dwarf2_lineinfo_hash, dwarf2_lineinfo_eq,
> 
> I don't have any data, but 127 seems high.

I have not changed it but this was just blind copy from existing line 21818.


> I wouldn't change it, I just wanted to ask if you have any data
> guiding this choice.

Tuning some constants really makes no sense when GDB has missing + insanely
complicated data structures and in consequence GDB is using inappropriate data
structures with bad algorithmic complexity.  One needs to switch GDB to C++
and its STL before one can start talking about data structures performance.


No regressions on {x86_64,x86_64-m32,i686}-fedora20-linux-gnu in DWZ mode and
in -fdebug-types-section mode.


Thanks,
Jan

[-- Attachment #2: partialunit5.patch --]
[-- Type: text/plain, Size: 7429 bytes --]

gdb/
2014-10-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix 100x slowdown regression on DWZ files.
	* dwarf2read.c (struct dwarf2_per_objfile): Add line_header_hash.
	(struct line_header): Add offset and offset_in_dwz.
	(dwarf_decode_lines): Add parameter decode_mapping to the declaration.
	(free_line_header_voidp): New declaration.
	(line_header_hash, line_header_eq): New functions.
	(dwarf2_build_include_psymtabs): Update dwarf_decode_lines caller.
	(handle_DW_AT_stmt_list): Use dwarf2_per_objfile->line_header_hash.
	(free_line_header_voidp): New function.
	(dwarf_decode_line_header): Initialize offset and offset_in_dwz.
	(dwarf_decode_lines): New parameter decode_mapping, use it.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 9d0ee13..206dc10 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -309,6 +309,9 @@ struct dwarf2_per_objfile
 
   /* The CUs we recently read.  */
   VEC (dwarf2_per_cu_ptr) *just_read_cus;
+
+  /* Table containing line_header indexed by offset and offset_in_dwz.  */
+  htab_t line_header_hash;
 };
 
 static struct dwarf2_per_objfile *dwarf2_per_objfile;
@@ -1025,6 +1028,12 @@ typedef void (die_reader_func_ftype) (const struct die_reader_specs *reader,
    which contains the following information.  */
 struct line_header
 {
+  /* Offset of line number information in .debug_line section.  */
+  sect_offset offset;
+
+  /* OFFSET is for struct dwz_file associated with dwarf2_per_objfile.  */
+  unsigned offset_in_dwz : 1;
+
   unsigned int total_length;
   unsigned short version;
   unsigned int header_length;
@@ -1513,7 +1522,7 @@ static struct line_header *dwarf_decode_line_header (unsigned int offset,
 
 static void dwarf_decode_lines (struct line_header *, const char *,
 				struct dwarf2_cu *, struct partial_symtab *,
-				CORE_ADDR);
+				CORE_ADDR, int decode_mapping);
 
 static void dwarf2_start_subfile (const char *, const char *, const char *);
 
@@ -1845,6 +1854,8 @@ static void free_dwo_file_cleanup (void *);
 static void process_cu_includes (void);
 
 static void check_producer (struct dwarf2_cu *cu);
+
+static void free_line_header_voidp (void *arg);
 \f
 /* Various complaints about symbol reading that don't abort the process.  */
 
@@ -1911,6 +1922,29 @@ dwarf2_invalid_attrib_class_complaint (const char *arg1, const char *arg2)
 	     _("invalid attribute class or form for '%s' in '%s'"),
 	     arg1, arg2);
 }
+
+/* Hash function for line_header_hash.  */
+
+static hashval_t
+line_header_hash (const void *item)
+{
+  const struct line_header *ofs = item;
+
+  return ofs->offset.sect_off ^ ofs->offset_in_dwz;
+}
+
+/* Equality function for line_header_hash.  */
+
+static int
+line_header_eq (const void *item_lhs, const void *item_rhs)
+{
+  const struct line_header *ofs_lhs = item_lhs;
+  const struct line_header *ofs_rhs = item_rhs;
+
+  return (ofs_lhs->offset.sect_off == ofs_rhs->offset.sect_off
+	  && ofs_lhs->offset_in_dwz == ofs_rhs->offset_in_dwz);
+}
+
 \f
 #if WORDS_BIGENDIAN
 
@@ -4449,7 +4483,7 @@ dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
     return;  /* No linetable, so no includes.  */
 
   /* NOTE: pst->dirname is DW_AT_comp_dir (if present).  */
-  dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow);
+  dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow, 1);
 
   free_line_header (lh);
 }
@@ -8975,24 +9009,64 @@ static void
 handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 			const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
 {
+  struct objfile *objfile = dwarf2_per_objfile->objfile;
   struct attribute *attr;
+  unsigned int line_offset;
+  struct line_header *line_header, line_header_local;
+  unsigned u;
+  void **slot;
+  int decode_mapping;
 
   gdb_assert (! cu->per_cu->is_debug_types);
 
   attr = dwarf2_attr (die, DW_AT_stmt_list, cu);
-  if (attr)
+  if (attr == NULL)
+    return;
+
+  line_offset = DW_UNSND (attr);
+
+  if (dwarf2_per_objfile->line_header_hash == NULL)
     {
-      unsigned int line_offset = DW_UNSND (attr);
-      struct line_header *line_header
-	= dwarf_decode_line_header (line_offset, cu);
+      dwarf2_per_objfile->line_header_hash
+	= htab_create_alloc_ex (127, line_header_hash, line_header_eq,
+				free_line_header_voidp,
+				&objfile->objfile_obstack,
+				hashtab_obstack_allocate,
+				dummy_obstack_deallocate);
+    }
 
-      if (line_header)
-	{
-	  cu->line_header = line_header;
-	  make_cleanup (free_cu_line_header, cu);
-	  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
-	}
+  line_header_local.offset.sect_off = line_offset;
+  line_header_local.offset_in_dwz = cu->per_cu->is_dwz;
+  slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
+			 &line_header_local, NO_INSERT);
+
+  /* For DW_TAG_compile_unit we need info like symtab::linetable which
+     is not present in *SLOT.  */
+  if (die->tag == DW_TAG_partial_unit && slot != NULL)
+    {
+      gdb_assert (*slot != NULL);
+      cu->line_header = *slot;
+      return;
+    }
+
+  line_header = dwarf_decode_line_header (line_offset, cu);
+  if (line_header == NULL)
+    return;
+  cu->line_header = line_header;
+
+  slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
+			 &line_header_local, INSERT);
+  gdb_assert (slot != NULL);
+  if (*slot == NULL)
+    *slot = line_header;
+  else
+    {
+      gdb_assert (die->tag != DW_TAG_partial_unit);
+      make_cleanup (free_cu_line_header, cu);
     }
+  decode_mapping = (die->tag != DW_TAG_partial_unit);
+  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc,
+		      decode_mapping);
 }
 
 /* Process DW_TAG_compile_unit or DW_TAG_partial_unit.  */
@@ -16863,6 +16937,16 @@ free_line_header (struct line_header *lh)
   xfree (lh);
 }
 
+/* Stub for free_line_header to match void * callback types.  */
+
+static void
+free_line_header_voidp (void *arg)
+{
+  struct line_header *lh = arg;
+
+  free_line_header (lh);
+}
+
 /* Add an entry to LH's include directory table.  */
 
 static void
@@ -16993,6 +17077,9 @@ dwarf_decode_line_header (unsigned int offset, struct dwarf2_cu *cu)
   back_to = make_cleanup ((make_cleanup_ftype *) free_line_header,
                           (void *) lh);
 
+  lh->offset.sect_off = offset;
+  lh->offset_in_dwz = cu->per_cu->is_dwz;
+
   line_ptr = section->buffer + offset;
 
   /* Read in the header.  */
@@ -17605,18 +17692,23 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
    E.g. expand_line_sal requires this when finding psymtabs to expand.
    A good testcase for this is mb-inline.exp.
 
-   LOWPC is the lowest address in CU (or 0 if not known).  */
+   LOWPC is the lowest address in CU (or 0 if not known).
+
+   Boolean DECODE_MAPPING specifies we need to fully decode .debug_line
+   for its PC<->lines mapping information.  Otherwise only filenames
+   tables is read in.  */
 
 static void
 dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
 		    struct dwarf2_cu *cu, struct partial_symtab *pst,
-		    CORE_ADDR lowpc)
+		    CORE_ADDR lowpc, int decode_mapping)
 {
   struct objfile *objfile = cu->objfile;
   const int decode_for_pst_p = (pst != NULL);
   struct subfile *first_subfile = current_subfile;
 
-  dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p, lowpc);
+  if (decode_mapping)
+    dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p, lowpc);
 
   if (decode_for_pst_p)
     {

Re: ping^2: [patchv2] Fix 100x slowdown regression on DWZ files

[Doug Evans]

Jan Kratochvil writes:
 > On Thu, 02 Oct 2014 17:56:53 +0200, Jan Kratochvil wrote:
 > 
 > On Thu, 02 Oct 2014 01:51:38 +0200, Doug Evans wrote:
 > > I tested this patch with --target_board=dwarf4-gdb-index
 > > and got a failure in m-static.exp:
 > 
 > That is particularly with -fdebug-types-section.
 > 
 > 
 > > Type units read the line table in a separate path,
 > 
 > OK, therefore I dropped that separate struct dwarf2_lineinfo
 > and reused struct line_header instead.
 > 
 > 
 > > OTOH, I do want to avoid any confusion that this patch may inadvertently
 > > introduce. For example, IIUC with your patch as is,
 > > if we read a partial_unit first, before a compile_unit
 > > that has the same stmt_list value, we'll do more processing in
 > > dwarf_decode_lines than we really need to since we only need a file
 > > number to symtab mapping. And if we later read in a compile_unit
 > > with the same stmt_value we'll call dwarf_decode_lines again,
 > > and this time we need the pc/line mapping it computes.
 > > Whereas if we process these in the opposite order we'll only call
 > > dwarf_decode_lines once. I'm sure this will be confusing at first
 > > to some later developer going through this code.
 > > [I could be missing something of course, and I'm happy for any corrections.]
 > 
 > Implemented (omitting some story why I did not include it before).
 > 
 > 
 > > The code that processes stmt_list for type_units is in setup_type_unit_groups.
 > > Note that this code goes to the trouble of re-initializing the buildsym
 > > machinery (see the calls to restart_symtab in dwarf2read.c) when we process
 > > the second and subsequent type units that share a stmt_list value.
 > > This is something that used to be done before your patch and will no
 > > longer be done with your patch (since if we get a cache hit we exit).
 > > It may be that the type_unit support is doing this unnecessarily,
 > > which would be great because we can then simplify it.
 > 
 > I hope this patch should no longer break -fdebug-types-section.
 > If it additionally enables some future optimization for -fdebug-types-section
 > the better.
 > 
 > 
 > >  > +  /* Offset of line number information in .debug_line section.  */
 > >  > +  sect_offset offset;
 > >  > +  unsigned offset_in_dwz : 1;
 > > 
 > > IWBN to document why offset_in_dwz is here.
 > > It's not obvious why it's needed.
 > +
 > On Thu, 02 Oct 2014 01:57:03 +0200, Doug Evans wrote:
 > > Ah, I guess the offset_in_dwz flag will ensure dwarf_decode_lines gets called
 > > twice regardless of order.  But is that the only reason for the flag?
 > 
 > I have added there now:
 > +  /* OFFSET is for struct dwz_file associated with dwarf2_per_objfile.  */
 > 
 > If one removes it regressions really happen.  What happens is that this
 > line_header_hash (former lineinfo_hash) is in struct dwarf2_per_objfile which
 > is common for both objfile and its objfile.dwz (that one is normally in
 > /usr/lib/debug/.dwz/ - common for multiple objfiles).  And there are two
 > different DIEs at offset 0xb - one in objfile and one in objfile.dwz - which
 > would match single line_header if offset_in_dwz was not there.
 > 
 > Also existing dwarf2read.c code usually transfers "dwz flag" together with DIE
 > offset, such as:
 > 	dwarf2_find_containing_comp_unit (sect_offset offset,
 > 					  unsigned int offset_in_dwz,
 > 					  struct objfile *objfile)
 > This reminds me - why doesn't similar ambiguity happen also for dwp_file?
 > I am unfortunately not much aware of the dwp implementation details.
 > 
 > 
 > >  > -      struct line_header *line_header
 > >  > -	= dwarf_decode_line_header (line_offset, cu);
 > >  > +      dwarf2_per_objfile->lineinfo_hash =
 > > 
 > > As much as I prefer "=" going here, convention says to put it on the
 > > next line.
 > 
 > I have changed it but this was just blind copy from existing line 21818.
 > 
 > 
 > >  > +	htab_create_alloc_ex (127, dwarf2_lineinfo_hash, dwarf2_lineinfo_eq,
 > > 
 > > I don't have any data, but 127 seems high.
 > 
 > I have not changed it but this was just blind copy from existing line 21818.
 > 
 > 
 > > I wouldn't change it, I just wanted to ask if you have any data
 > > guiding this choice.
 > 
 > Tuning some constants really makes no sense when GDB has missing + insanely
 > complicated data structures and in consequence GDB is using inappropriate data
 > structures with bad algorithmic complexity.  One needs to switch GDB to C++
 > and its STL before one can start talking about data structures performance.
 > 
 > 
 > No regressions on {x86_64,x86_64-m32,i686}-fedora20-linux-gnu in DWZ mode and
 > in -fdebug-types-section mode.
 > 
 > 
 > Thanks,
 > Jan
 > gdb/
 > 2014-10-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
 > 
 > 	Fix 100x slowdown regression on DWZ files.
 > 	* dwarf2read.c (struct dwarf2_per_objfile): Add line_header_hash.
 > 	(struct line_header): Add offset and offset_in_dwz.
 > 	(dwarf_decode_lines): Add parameter decode_mapping to the declaration.
 > 	(free_line_header_voidp): New declaration.
 > 	(line_header_hash, line_header_eq): New functions.
 > 	(dwarf2_build_include_psymtabs): Update dwarf_decode_lines caller.
 > 	(handle_DW_AT_stmt_list): Use dwarf2_per_objfile->line_header_hash.
 > 	(free_line_header_voidp): New function.
 > 	(dwarf_decode_line_header): Initialize offset and offset_in_dwz.
 > 	(dwarf_decode_lines): New parameter decode_mapping, use it.

Hi.

Sorry for the delay.

A few comments inline.

@@ -8994,20 +9028,33 @@ static void
 handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 			const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
 {
+  struct objfile *objfile = dwarf2_per_objfile->objfile;
   struct attribute *attr;
+  unsigned int line_offset;
+  struct line_header *line_header, line_header_local;
+  unsigned u;
+  void **slot;
+  int decode_mapping;
 
   gdb_assert (! cu->per_cu->is_debug_types);
 
   attr = dwarf2_attr (die, DW_AT_stmt_list, cu);
-  if (attr)
+  if (attr == NULL)
+    return;
+
+  line_offset = DW_UNSND (attr);
+
+  if (dwarf2_per_objfile->line_header_hash == NULL)
     {
-      unsigned int line_offset = DW_UNSND (attr);
-      struct line_header *line_header
-	= dwarf_decode_line_header (line_offset, cu);
+      dwarf2_per_objfile->line_header_hash
+	= htab_create_alloc_ex (127, line_header_hash, line_header_eq,
+				free_line_header_voidp,
+				&objfile->objfile_obstack,
+				hashtab_obstack_allocate,
+				dummy_obstack_deallocate);
+    }
 
-      if (line_header)
-	{
-	  cu->line_header = line_header;
-	  make_cleanup (free_cu_line_header, cu);
-	  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
-	}
+  line_header_local.offset.sect_off = line_offset;
+  line_header_local.offset_in_dwz = cu->per_cu->is_dwz;
+  slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
+			 &line_header_local, NO_INSERT);

Do hashtables support calling htab_find_slot with INSERT but then
not assigning the slot a value if it was empty?
I could be wrong but I think it does.
If so, we can remove one call to htab_find_slot here.

+
+  /* For DW_TAG_compile_unit we need info like symtab::linetable which
+     is not present in *SLOT.  */
+  if (die->tag == DW_TAG_partial_unit && slot != NULL)
+    {
+      gdb_assert (*slot != NULL);
+      cu->line_header = *slot;
+      return;
+    }
+
+  line_header = dwarf_decode_line_header (line_offset, cu);
+  if (line_header == NULL)
+    return;
+  cu->line_header = line_header;
+
+  slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
+			 &line_header_local, INSERT);
+  gdb_assert (slot != NULL);
+  if (*slot == NULL)
+    *slot = line_header;
+  else
+    {
+      gdb_assert (die->tag != DW_TAG_partial_unit);
+      make_cleanup (free_cu_line_header, cu);
     }
+  decode_mapping = (die->tag != DW_TAG_partial_unit);
+  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc,
+		      decode_mapping);

This is a bit confusing to follow.
If the slot was empty we save line_header in it (and don't record a cleanup),
but if wasn't empty we don't update *slot but do record a cleanup.
Presumably there's a reason, but it's hard to follow.
It would be simpler to just free any previous entry and conditionally
update *slot.  Is there a reason to not do that?
Can you add a clarifying comment?

Plus I'm worried about increased memory usage in the non-dwz case.
IIUC, the non-dwz case will always have *slot == NULL, and thus we'll
always be saving line header entries we'll never need again.

Also, it looks like line_header_hash (and its entries) aren't
deleted when the objfile goes away.  Missing call to htab_delete.

 }
 
 /* Process DW_TAG_compile_unit or DW_TAG_partial_unit.  */

....

@@ -17665,17 +17752,22 @@ dwarf_decode_lines_1 (struct line_header *lh, struct dwarf2_cu *cu,
    E.g. expand_line_sal requires this when finding psymtabs to expand.
    A good testcase for this is mb-inline.exp.
 
-   LOWPC is the lowest address in CU (or 0 if not known).  */
+   LOWPC is the lowest address in CU (or 0 if not known).
+
+   Boolean DECODE_MAPPING specifies we need to fully decode .debug_line
+   for its PC<->lines mapping information.  Otherwise only filenames

*the filenames

+   tables is read in.  */

s/tables/table/
 
 static void
 dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
 		    struct dwarf2_cu *cu, struct partial_symtab *pst,
-		    CORE_ADDR lowpc)
+		    CORE_ADDR lowpc, int decode_mapping)
 {
   struct objfile *objfile = cu->objfile;
   const int decode_for_pst_p = (pst != NULL);
 
-  dwarf_decode_lines_1 (lh, cu, decode_for_pst_p, lowpc);
+  if (decode_mapping)
+    dwarf_decode_lines_1 (lh, cu, decode_for_pst_p, lowpc);
 
   if (decode_for_pst_p)
     {

Re: ping^2: [patchv2] Fix 100x slowdown regression on DWZ files

[Doug Evans]

Doug Evans writes:
 > Do hashtables support calling htab_find_slot with INSERT but then
 > not assigning the slot a value if it was empty?
 > I could be wrong but I think it does.
 > If so, we can remove one call to htab_find_slot here.

I was wrong.
I thought I was, but I couldn't remember, and couldn't remember where
I had seen this.  But htab_find_slot_with_hash does assume
that if you call it with INSERT, and the slot isn't already used,
then you're going to fill in the slot.
No matter, it was just a thought.

Re: ping^2: [patchv2] Fix 100x slowdown regression on DWZ files

[Jan Kratochvil]

On Fri, 09 Jan 2015 01:32:27 +0100, Doug Evans wrote:
> Doug Evans writes:
>  > Do hashtables support calling htab_find_slot with INSERT but then
>  > not assigning the slot a value if it was empty?
>  > I could be wrong but I think it does.
>  > If so, we can remove one call to htab_find_slot here.
> 
> I was wrong.
> I thought I was, but I couldn't remember, and couldn't remember where
> I had seen this.  But htab_find_slot_with_hash does assume
> that if you call it with INSERT, and the slot isn't already used,
> then you're going to fill in the slot.
> No matter, it was just a thought.

Maybe one could use HTAB_DELETED_ENTRY in such case?

But I find that needless microoptimization.  This is solved by compilers this
century.  And much faster avoiding all the pointer dereferences which is all
that matters.

But I can use at least htab_find_slot_with_hash instead of htab_find_slot.


Jan

Re: ping^2: [patchv2] Fix 100x slowdown regression on DWZ files

[Doug Evans]

On Mon, Jan 12, 2015 at 9:13 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 09 Jan 2015 01:32:27 +0100, Doug Evans wrote:
>> Doug Evans writes:
>>  > Do hashtables support calling htab_find_slot with INSERT but then
>>  > not assigning the slot a value if it was empty?
>>  > I could be wrong but I think it does.
>>  > If so, we can remove one call to htab_find_slot here.
>>
>> I was wrong.
>> I thought I was, but I couldn't remember, and couldn't remember where
>> I had seen this.  But htab_find_slot_with_hash does assume
>> that if you call it with INSERT, and the slot isn't already used,
>> then you're going to fill in the slot.
>> No matter, it was just a thought.
>
> Maybe one could use HTAB_DELETED_ENTRY in such case?
>
> But I find that needless microoptimization.  This is solved by compilers this
> century.  And much faster avoiding all the pointer dereferences which is all
> that matters.
>
> But I can use at least htab_find_slot_with_hash instead of htab_find_slot.

For reference sake,
This was never about "microoptimization".
It was about code readability.

[patchv3] Fix 100x slowdown regression on DWZ files

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 2975 bytes --]

Hi Doug,

On Thu, 08 Jan 2015 02:45:18 +0100, Doug Evans wrote:
> +  line_header_local.offset.sect_off = line_offset;
> +  line_header_local.offset_in_dwz = cu->per_cu->is_dwz;
> +  slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
> +			 &line_header_local, NO_INSERT);
> 
> Do hashtables support calling htab_find_slot with INSERT but then
> not assigning the slot a value if it was empty?
> I could be wrong but I think it does.
> If so, we can remove one call to htab_find_slot here.

If dwarf_decode_line_header() fails we have nothing to put there.  If we had
done INSERT it is a problem as discussed in previous mail.


> +  /* For DW_TAG_compile_unit we need info like symtab::linetable which
> +     is not present in *SLOT.  */
> +  if (die->tag == DW_TAG_partial_unit && slot != NULL)
> +    {
> +      gdb_assert (*slot != NULL);
> +      cu->line_header = *slot;
> +      return;
> +    }
> +
> +  line_header = dwarf_decode_line_header (line_offset, cu);
> +  if (line_header == NULL)
> +    return;
> +  cu->line_header = line_header;
> +
> +  slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
> +			 &line_header_local, INSERT);
> +  gdb_assert (slot != NULL);
> +  if (*slot == NULL)
> +    *slot = line_header;
> +  else
> +    {
> +      gdb_assert (die->tag != DW_TAG_partial_unit);
> +      make_cleanup (free_cu_line_header, cu);
>      }
> +  decode_mapping = (die->tag != DW_TAG_partial_unit);
> +  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc,
> +		      decode_mapping);
> 
> This is a bit confusing to follow.
> If the slot was empty we save line_header in it (and don't record a cleanup),
> but if wasn't empty we don't update *slot but do record a cleanup.
> Presumably there's a reason, but it's hard to follow.

I do not see how to make it differently.  I have put there more comments.


> It would be simpler to just free any previous entry and conditionally
> update *slot.  Is there a reason to not do that?
> Can you add a clarifying comment?

We cannot - comment with the reason is now in the code.


> Plus I'm worried about increased memory usage in the non-dwz case.
> IIUC, the non-dwz case will always have *slot == NULL, and thus we'll
> always be saving line header entries we'll never need again.

<rant>Those are few bytes for each expanded CU.  The problem of GDB is that it
needlessly expands thousands of CUs.  Saving each byte of a decoded CU is not
the right way how to fix the excessive memory consumption.</rant>

But added there for it 'dwarf2_per_objfile->seen_partial_unit'.


> Also, it looks like line_header_hash (and its entries) aren't
> deleted when the objfile goes away.  Missing call to htab_delete.

Fixed.


> A few comments inline.

BTW I would prefer s/^/> / for the patch, the comments are difficult to find
this way.



No regressions on {x86_64,x86_64-m32,i686}-fedora22pre-linux-gnu in default
mode, other modes still run but hopefully they will be OK.


Thanks,
Jan

[-- Attachment #2: doug3.patch --]
[-- Type: text/plain, Size: 9353 bytes --]

2015-01-14  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix 100x slowdown regression on DWZ files.
	* dwarf2read.c (struct dwarf2_per_objfile): Add seen_partial_unit and
	line_header_hash.
	(struct line_header): Add offset and offset_in_dwz.
	(dwarf_decode_lines): Add parameter decode_mapping to the declaration.
	(free_line_header_voidp): New declaration.
	(line_header_hash, line_header_hash_voidp, line_header_eq_voidp): New
	functions.
	(dwarf2_build_include_psymtabs): Update dwarf_decode_lines caller.
	(handle_DW_AT_stmt_list): Use seen_partial_unit and line_header_hash.
	(free_line_header_voidp): New function.
	(dwarf_decode_line_header): Initialize offset and offset_in_dwz.
	(dwarf_decode_lines): New parameter decode_mapping, use it.
	(dwarf2_free_objfile): Free line_header_hash.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 96b2537..fadc258 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -285,6 +285,9 @@ struct dwarf2_per_objfile
      or we are faking it for OBJF_READNOW's sake.  */
   unsigned char using_index;
 
+  /* True if we have already seen a DW_TAG_partial_unit.  */
+  unsigned char seen_partial_unit;
+
   /* The mapped index, or NULL if .gdb_index is missing or not being used.  */
   struct mapped_index *index_table;
 
@@ -308,6 +311,9 @@ struct dwarf2_per_objfile
 
   /* The CUs we recently read.  */
   VEC (dwarf2_per_cu_ptr) *just_read_cus;
+
+  /* Table containing line_header indexed by offset and offset_in_dwz.  */
+  htab_t line_header_hash;
 };
 
 static struct dwarf2_per_objfile *dwarf2_per_objfile;
@@ -1024,6 +1030,12 @@ typedef void (die_reader_func_ftype) (const struct die_reader_specs *reader,
    which contains the following information.  */
 struct line_header
 {
+  /* Offset of line number information in .debug_line section.  */
+  sect_offset offset;
+
+  /* OFFSET is for struct dwz_file associated with dwarf2_per_objfile.  */
+  unsigned offset_in_dwz : 1;
+
   unsigned int total_length;
   unsigned short version;
   unsigned int header_length;
@@ -1512,7 +1524,7 @@ static struct line_header *dwarf_decode_line_header (unsigned int offset,
 
 static void dwarf_decode_lines (struct line_header *, const char *,
 				struct dwarf2_cu *, struct partial_symtab *,
-				CORE_ADDR);
+				CORE_ADDR, int decode_mapping);
 
 static void dwarf2_start_subfile (const char *, const char *);
 
@@ -1844,6 +1856,8 @@ static void free_dwo_file_cleanup (void *);
 static void process_cu_includes (void);
 
 static void check_producer (struct dwarf2_cu *cu);
+
+static void free_line_header_voidp (void *arg);
 \f
 /* Various complaints about symbol reading that don't abort the process.  */
 
@@ -1910,6 +1924,37 @@ dwarf2_invalid_attrib_class_complaint (const char *arg1, const char *arg2)
 	     _("invalid attribute class or form for '%s' in '%s'"),
 	     arg1, arg2);
 }
+
+/* Hash function for line_header_hash.  */
+
+static hashval_t
+line_header_hash (const struct line_header *ofs)
+{
+  return ofs->offset.sect_off ^ ofs->offset_in_dwz;
+}
+
+/* Hash function for htab_create_alloc_ex for line_header_hash.  */
+
+static hashval_t
+line_header_hash_voidp (const void *item)
+{
+  const struct line_header *ofs = item;
+
+  return line_header_hash (ofs);
+}
+
+/* Equality function for line_header_hash.  */
+
+static int
+line_header_eq_voidp (const void *item_lhs, const void *item_rhs)
+{
+  const struct line_header *ofs_lhs = item_lhs;
+  const struct line_header *ofs_rhs = item_rhs;
+
+  return (ofs_lhs->offset.sect_off == ofs_rhs->offset.sect_off
+	  && ofs_lhs->offset_in_dwz == ofs_rhs->offset_in_dwz);
+}
+
 \f
 #if WORDS_BIGENDIAN
 
@@ -4452,7 +4497,7 @@ dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
     return;  /* No linetable, so no includes.  */
 
   /* NOTE: pst->dirname is DW_AT_comp_dir (if present).  */
-  dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow);
+  dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow, 1);
 
   free_line_header (lh);
 }
@@ -8995,24 +9040,90 @@ static void
 handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 			const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
 {
+  struct objfile *objfile = dwarf2_per_objfile->objfile;
   struct attribute *attr;
+  unsigned int line_offset;
+  struct line_header line_header_local;
+  hashval_t line_header_local_hash;
+  unsigned u;
+  void **slot;
+  int decode_mapping;
 
   gdb_assert (! cu->per_cu->is_debug_types);
 
   attr = dwarf2_attr (die, DW_AT_stmt_list, cu);
-  if (attr)
+  if (attr == NULL)
+    return;
+
+  line_offset = DW_UNSND (attr);
+
+  if (die->tag == DW_TAG_partial_unit)
+    dwarf2_per_objfile->seen_partial_unit = 1;
+
+  if (dwarf2_per_objfile->line_header_hash == NULL
+      && dwarf2_per_objfile->seen_partial_unit)
+    {
+      dwarf2_per_objfile->line_header_hash
+	= htab_create_alloc_ex (127, line_header_hash_voidp,
+				line_header_eq_voidp,
+				free_line_header_voidp,
+				&objfile->objfile_obstack,
+				hashtab_obstack_allocate,
+				dummy_obstack_deallocate);
+    }
+
+  line_header_local.offset.sect_off = line_offset;
+  line_header_local.offset_in_dwz = cu->per_cu->is_dwz;
+  line_header_local_hash = line_header_hash (&line_header_local);
+  if (dwarf2_per_objfile->seen_partial_unit)
     {
-      unsigned int line_offset = DW_UNSND (attr);
-      struct line_header *line_header
-	= dwarf_decode_line_header (line_offset, cu);
+      slot = htab_find_slot_with_hash (dwarf2_per_objfile->line_header_hash,
+				       &line_header_local,
+				       line_header_local_hash, NO_INSERT);
 
-      if (line_header)
+      /* For DW_TAG_compile_unit we need info like symtab::linetable which
+	 is not present in *SLOT.  */
+      if (die->tag == DW_TAG_partial_unit && slot != NULL)
 	{
-	  cu->line_header = line_header;
-	  make_cleanup (free_cu_line_header, cu);
-	  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
+	  gdb_assert (*slot != NULL);
+	  cu->line_header = *slot;
+	  return;
 	}
     }
+
+  /* dwarf_decode_line_header does not yet provide sufficient information.
+     We always have to call also dwarf_decode_lines for it.  */
+  cu->line_header = dwarf_decode_line_header (line_offset, cu);
+  if (cu->line_header == NULL)
+    return;
+
+  if (!dwarf2_per_objfile->seen_partial_unit)
+    slot = NULL;
+  else
+    {
+      slot = htab_find_slot_with_hash (dwarf2_per_objfile->line_header_hash,
+				       &line_header_local,
+				       line_header_local_hash, INSERT);
+      gdb_assert (slot != NULL);
+    }
+  if (slot != NULL && *slot == NULL)
+    {
+      /* This newly decoded line number information unit will be owned
+	 by line_header_hash hash table.  */
+      *slot = cu->line_header;
+    }
+  else
+    {
+      /* We cannot free_line_header (*slot) as that struct line_header
+         may be already used by multiple CUs.  Create only temporary
+	 decoded line_header for this CU - it may happen at most once
+	 for each line number information unit.  */
+      gdb_assert (die->tag != DW_TAG_partial_unit);
+      make_cleanup (free_cu_line_header, cu);
+    }
+  decode_mapping = (die->tag != DW_TAG_partial_unit);
+  dwarf_decode_lines (cu->line_header, comp_dir, cu, NULL, lowpc,
+		      decode_mapping);
 }
 
 /* Process DW_TAG_compile_unit or DW_TAG_partial_unit.  */
@@ -16909,6 +17020,16 @@ free_line_header (struct line_header *lh)
   xfree (lh);
 }
 
+/* Stub for free_line_header to match void * callback types.  */
+
+static void
+free_line_header_voidp (void *arg)
+{
+  struct line_header *lh = arg;
+
+  free_line_header (lh);
+}
+
 /* Add an entry to LH's include directory table.  */
 
 static void
@@ -17039,6 +17160,9 @@ dwarf_decode_line_header (unsigned int offset, struct dwarf2_cu *cu)
   back_to = make_cleanup ((make_cleanup_ftype *) free_line_header,
                           (void *) lh);
 
+  lh->offset.sect_off = offset;
+  lh->offset_in_dwz = cu->per_cu->is_dwz;
+
   line_ptr = section->buffer + offset;
 
   /* Read in the header.  */
@@ -17666,17 +17790,22 @@ dwarf_decode_lines_1 (struct line_header *lh, struct dwarf2_cu *cu,
    E.g. expand_line_sal requires this when finding psymtabs to expand.
    A good testcase for this is mb-inline.exp.
 
-   LOWPC is the lowest address in CU (or 0 if not known).  */
+   LOWPC is the lowest address in CU (or 0 if not known).
+
+   Boolean DECODE_MAPPING specifies we need to fully decode .debug_line
+   for its PC<->lines mapping information.  Otherwise only the filename
+   table is read in.  */
 
 static void
 dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
 		    struct dwarf2_cu *cu, struct partial_symtab *pst,
-		    CORE_ADDR lowpc)
+		    CORE_ADDR lowpc, int decode_mapping)
 {
   struct objfile *objfile = cu->objfile;
   const int decode_for_pst_p = (pst != NULL);
 
-  dwarf_decode_lines_1 (lh, cu, decode_for_pst_p, lowpc);
+  if (decode_mapping)
+    dwarf_decode_lines_1 (lh, cu, decode_for_pst_p, lowpc);
 
   if (decode_for_pst_p)
     {
@@ -21773,6 +21902,9 @@ dwarf2_free_objfile (struct objfile *objfile)
   if (dwarf2_per_objfile->quick_file_names_table)
     htab_delete (dwarf2_per_objfile->quick_file_names_table);
 
+  if (dwarf2_per_objfile->line_header_hash)
+    htab_delete (dwarf2_per_objfile->line_header_hash);
+
   /* Everything else should be on the objfile obstack.  */
 }
 

Re: [patchv3] Fix 100x slowdown regression on DWZ files

[Doug Evans]

Jan Kratochvil writes:
 > Hi Doug,
 > 
 > On Thu, 08 Jan 2015 02:45:18 +0100, Doug Evans wrote:
 > > +  line_header_local.offset.sect_off = line_offset;
 > > +  line_header_local.offset_in_dwz = cu->per_cu->is_dwz;
 > > +  slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
 > > +			 &line_header_local, NO_INSERT);
 > > 
 > > Do hashtables support calling htab_find_slot with INSERT but then
 > > not assigning the slot a value if it was empty?
 > > I could be wrong but I think it does.
 > > If so, we can remove one call to htab_find_slot here.
 > 
 > If dwarf_decode_line_header() fails we have nothing to put there.  If we had
 > done INSERT it is a problem as discussed in previous mail.
 > 
 > 
 > > +  /* For DW_TAG_compile_unit we need info like symtab::linetable which
 > > +     is not present in *SLOT.  */
 > > +  if (die->tag == DW_TAG_partial_unit && slot != NULL)
 > > +    {
 > > +      gdb_assert (*slot != NULL);
 > > +      cu->line_header = *slot;
 > > +      return;
 > > +    }
 > > +
 > > +  line_header = dwarf_decode_line_header (line_offset, cu);
 > > +  if (line_header == NULL)
 > > +    return;
 > > +  cu->line_header = line_header;
 > > +
 > > +  slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
 > > +			 &line_header_local, INSERT);
 > > +  gdb_assert (slot != NULL);
 > > +  if (*slot == NULL)
 > > +    *slot = line_header;
 > > +  else
 > > +    {
 > > +      gdb_assert (die->tag != DW_TAG_partial_unit);
 > > +      make_cleanup (free_cu_line_header, cu);
 > >      }
 > > +  decode_mapping = (die->tag != DW_TAG_partial_unit);
 > > +  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc,
 > > +		      decode_mapping);
 > > 
 > > This is a bit confusing to follow.
 > > If the slot was empty we save line_header in it (and don't record a cleanup),
 > > but if wasn't empty we don't update *slot but do record a cleanup.
 > > Presumably there's a reason, but it's hard to follow.
 > 
 > I do not see how to make it differently.  I have put there more comments.
 > 
 > 
 > > It would be simpler to just free any previous entry and conditionally
 > > update *slot.  Is there a reason to not do that?
 > > Can you add a clarifying comment?
 > 
 > We cannot - comment with the reason is now in the code.
 > 
 > 
 > > Plus I'm worried about increased memory usage in the non-dwz case.
 > > IIUC, the non-dwz case will always have *slot == NULL, and thus we'll
 > > always be saving line header entries we'll never need again.
 > 
 > <rant>Those are few bytes for each expanded CU.  The problem of GDB is that it
 > needlessly expands thousands of CUs.  Saving each byte of a decoded CU is not
 > the right way how to fix the excessive memory consumption.</rant>
 > 
 > But added there for it 'dwarf2_per_objfile->seen_partial_unit'.
 > 
 > 
 > > Also, it looks like line_header_hash (and its entries) aren't
 > > deleted when the objfile goes away.  Missing call to htab_delete.
 > 
 > Fixed.
 > 
 > 
 > > A few comments inline.
 > 
 > BTW I would prefer s/^/> / for the patch, the comments are difficult to find
 > this way.
 > 
 > 
 > 
 > No regressions on {x86_64,x86_64-m32,i686}-fedora22pre-linux-gnu in default
 > mode, other modes still run but hopefully they will be OK.
 > 
 > 
 > Thanks,
 > Jan
 > 2015-01-14  Jan Kratochvil  <jan.kratochvil@redhat.com>
 > 
 > 	Fix 100x slowdown regression on DWZ files.
 > 	* dwarf2read.c (struct dwarf2_per_objfile): Add seen_partial_unit and
 > 	line_header_hash.
 > 	(struct line_header): Add offset and offset_in_dwz.
 > 	(dwarf_decode_lines): Add parameter decode_mapping to the declaration.
 > 	(free_line_header_voidp): New declaration.
 > 	(line_header_hash, line_header_hash_voidp, line_header_eq_voidp): New
 > 	functions.
 > 	(dwarf2_build_include_psymtabs): Update dwarf_decode_lines caller.
 > 	(handle_DW_AT_stmt_list): Use seen_partial_unit and line_header_hash.
 > 	(free_line_header_voidp): New function.
 > 	(dwarf_decode_line_header): Initialize offset and offset_in_dwz.
 > 	(dwarf_decode_lines): New parameter decode_mapping, use it.
 > 	(dwarf2_free_objfile): Free line_header_hash.

Hi.

Thanks, I understand things now.

I tweaked the patch a bit so that a year from now I'll still understand it. :-)

Appended is a diff to your patch, and then the full modified patch.

In my mind it's easier to just treat a non-NULL value for line_header_hash
as the flag to decide whether we're using the hash (instead of
seen_partial_unit).

Sound ok?

-- patch of the patch ---

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 3d27d70..e7246f3 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -285,9 +285,6 @@ struct dwarf2_per_objfile
      or we are faking it for OBJF_READNOW's sake.  */
   unsigned char using_index;
 
-  /* True if we have already seen a DW_TAG_partial_unit.  */
-  unsigned char seen_partial_unit;
-
   /* The mapped index, or NULL if .gdb_index is missing or not being used.  */
   struct mapped_index *index_table;
 
@@ -9056,11 +9053,14 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 
   line_offset = DW_UNSND (attr);
 
-  if (die->tag == DW_TAG_partial_unit)
-    dwarf2_per_objfile->seen_partial_unit = 1;
+  /* The line header hash table is only created if needed (it exists to
+     prevent redundant reading of the line table for partial_units).
+     If we're given a partial_unit, we'll need it.  If we're given a
+     compile_unit, then use the line header hash table if it's already
+     created, but don't create one just yet.  */
 
   if (dwarf2_per_objfile->line_header_hash == NULL
-      && dwarf2_per_objfile->seen_partial_unit)
+      && die->tag == DW_TAG_partial_unit)
     {
       dwarf2_per_objfile->line_header_hash
 	= htab_create_alloc_ex (127, line_header_hash_voidp,
@@ -9074,14 +9074,15 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
   line_header_local.offset.sect_off = line_offset;
   line_header_local.offset_in_dwz = cu->per_cu->is_dwz;
   line_header_local_hash = line_header_hash (&line_header_local);
-  if (dwarf2_per_objfile->seen_partial_unit)
+  if (dwarf2_per_objfile->line_header_hash != NULL)
     {
       slot = htab_find_slot_with_hash (dwarf2_per_objfile->line_header_hash,
 				       &line_header_local,
 				       line_header_local_hash, NO_INSERT);
 
       /* For DW_TAG_compile_unit we need info like symtab::linetable which
-	 is not present in *SLOT.  */
+	 is not present in *SLOT (since if there is something in *SLOT then
+	 it will be for a partial_unit).  */
       if (die->tag == DW_TAG_partial_unit && slot != NULL)
 	{
 	  gdb_assert (*slot != NULL);
@@ -9096,7 +9097,7 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
   if (cu->line_header == NULL)
     return;
 
-  if (!dwarf2_per_objfile->seen_partial_unit)
+  if (dwarf2_per_objfile->line_header_hash == NULL)
     slot = NULL;
   else
     {
@@ -9113,10 +9114,11 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
     }
   else
     {
-      /* We cannot free_line_header (*slot) as that struct line_header
-         may be already used by multiple CUs.  Create only temporary
-	 decoded line_header for this CU - it may happen at most once
-	 for each line number information unit.  */
+      /* We cannot free any current entry in (*slot) as that struct line_header
+         may be already used by multiple CUs.  Create only temporary decoded
+	 line_header for this CU - it may happen at most once for each line
+	 number information unit.  And if we're not using line_header_hash
+	 then this is what we want as well.  */
       gdb_assert (die->tag != DW_TAG_partial_unit);
       make_cleanup (free_cu_line_header, cu);
     }



--- full patch ---

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 86c3a73..e7246f3 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -308,6 +308,9 @@ struct dwarf2_per_objfile
 
   /* The CUs we recently read.  */
   VEC (dwarf2_per_cu_ptr) *just_read_cus;
+
+  /* Table containing line_header indexed by offset and offset_in_dwz.  */
+  htab_t line_header_hash;
 };
 
 static struct dwarf2_per_objfile *dwarf2_per_objfile;
@@ -1024,6 +1027,12 @@ typedef void (die_reader_func_ftype) (const struct die_reader_specs *reader,
    which contains the following information.  */
 struct line_header
 {
+  /* Offset of line number information in .debug_line section.  */
+  sect_offset offset;
+
+  /* OFFSET is for struct dwz_file associated with dwarf2_per_objfile.  */
+  unsigned offset_in_dwz : 1;
+
   unsigned int total_length;
   unsigned short version;
   unsigned int header_length;
@@ -1512,7 +1521,7 @@ static struct line_header *dwarf_decode_line_header (unsigned int offset,
 
 static void dwarf_decode_lines (struct line_header *, const char *,
 				struct dwarf2_cu *, struct partial_symtab *,
-				CORE_ADDR);
+				CORE_ADDR, int decode_mapping);
 
 static void dwarf2_start_subfile (const char *, const char *);
 
@@ -1844,6 +1853,8 @@ static void free_dwo_file_cleanup (void *);
 static void process_cu_includes (void);
 
 static void check_producer (struct dwarf2_cu *cu);
+
+static void free_line_header_voidp (void *arg);
 \f
 /* Various complaints about symbol reading that don't abort the process.  */
 
@@ -1910,6 +1921,37 @@ dwarf2_invalid_attrib_class_complaint (const char *arg1, const char *arg2)
 	     _("invalid attribute class or form for '%s' in '%s'"),
 	     arg1, arg2);
 }
+
+/* Hash function for line_header_hash.  */
+
+static hashval_t
+line_header_hash (const struct line_header *ofs)
+{
+  return ofs->offset.sect_off ^ ofs->offset_in_dwz;
+}
+
+/* Hash function for htab_create_alloc_ex for line_header_hash.  */
+
+static hashval_t
+line_header_hash_voidp (const void *item)
+{
+  const struct line_header *ofs = item;
+
+  return line_header_hash (ofs);
+}
+
+/* Equality function for line_header_hash.  */
+
+static int
+line_header_eq_voidp (const void *item_lhs, const void *item_rhs)
+{
+  const struct line_header *ofs_lhs = item_lhs;
+  const struct line_header *ofs_rhs = item_rhs;
+
+  return (ofs_lhs->offset.sect_off == ofs_rhs->offset.sect_off
+	  && ofs_lhs->offset_in_dwz == ofs_rhs->offset_in_dwz);
+}
+
 \f
 #if WORDS_BIGENDIAN
 
@@ -4452,7 +4494,7 @@ dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
     return;  /* No linetable, so no includes.  */
 
   /* NOTE: pst->dirname is DW_AT_comp_dir (if present).  */
-  dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow);
+  dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow, 1);
 
   free_line_header (lh);
 }
@@ -8994,24 +9036,95 @@ static void
 handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 			const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
 {
+  struct objfile *objfile = dwarf2_per_objfile->objfile;
   struct attribute *attr;
+  unsigned int line_offset;
+  struct line_header line_header_local;
+  hashval_t line_header_local_hash;
+  unsigned u;
+  void **slot;
+  int decode_mapping;
 
   gdb_assert (! cu->per_cu->is_debug_types);
 
   attr = dwarf2_attr (die, DW_AT_stmt_list, cu);
-  if (attr)
+  if (attr == NULL)
+    return;
+
+  line_offset = DW_UNSND (attr);
+
+  /* The line header hash table is only created if needed (it exists to
+     prevent redundant reading of the line table for partial_units).
+     If we're given a partial_unit, we'll need it.  If we're given a
+     compile_unit, then use the line header hash table if it's already
+     created, but don't create one just yet.  */
+
+  if (dwarf2_per_objfile->line_header_hash == NULL
+      && die->tag == DW_TAG_partial_unit)
+    {
+      dwarf2_per_objfile->line_header_hash
+	= htab_create_alloc_ex (127, line_header_hash_voidp,
+				line_header_eq_voidp,
+				free_line_header_voidp,
+				&objfile->objfile_obstack,
+				hashtab_obstack_allocate,
+				dummy_obstack_deallocate);
+    }
+
+  line_header_local.offset.sect_off = line_offset;
+  line_header_local.offset_in_dwz = cu->per_cu->is_dwz;
+  line_header_local_hash = line_header_hash (&line_header_local);
+  if (dwarf2_per_objfile->line_header_hash != NULL)
     {
-      unsigned int line_offset = DW_UNSND (attr);
-      struct line_header *line_header
-	= dwarf_decode_line_header (line_offset, cu);
+      slot = htab_find_slot_with_hash (dwarf2_per_objfile->line_header_hash,
+				       &line_header_local,
+				       line_header_local_hash, NO_INSERT);
 
-      if (line_header)
+      /* For DW_TAG_compile_unit we need info like symtab::linetable which
+	 is not present in *SLOT (since if there is something in *SLOT then
+	 it will be for a partial_unit).  */
+      if (die->tag == DW_TAG_partial_unit && slot != NULL)
 	{
-	  cu->line_header = line_header;
-	  make_cleanup (free_cu_line_header, cu);
-	  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
+	  gdb_assert (*slot != NULL);
+	  cu->line_header = *slot;
+	  return;
 	}
     }
+
+  /* dwarf_decode_line_header does not yet provide sufficient information.
+     We always have to call also dwarf_decode_lines for it.  */
+  cu->line_header = dwarf_decode_line_header (line_offset, cu);
+  if (cu->line_header == NULL)
+    return;
+
+  if (dwarf2_per_objfile->line_header_hash == NULL)
+    slot = NULL;
+  else
+    {
+      slot = htab_find_slot_with_hash (dwarf2_per_objfile->line_header_hash,
+				       &line_header_local,
+				       line_header_local_hash, INSERT);
+      gdb_assert (slot != NULL);
+    }
+  if (slot != NULL && *slot == NULL)
+    {
+      /* This newly decoded line number information unit will be owned
+	 by line_header_hash hash table.  */
+      *slot = cu->line_header;
+    }
+  else
+    {
+      /* We cannot free any current entry in (*slot) as that struct line_header
+         may be already used by multiple CUs.  Create only temporary decoded
+	 line_header for this CU - it may happen at most once for each line
+	 number information unit.  And if we're not using line_header_hash
+	 then this is what we want as well.  */
+      gdb_assert (die->tag != DW_TAG_partial_unit);
+      make_cleanup (free_cu_line_header, cu);
+    }
+  decode_mapping = (die->tag != DW_TAG_partial_unit);
+  dwarf_decode_lines (cu->line_header, comp_dir, cu, NULL, lowpc,
+		      decode_mapping);
 }
 
 /* Process DW_TAG_compile_unit or DW_TAG_partial_unit.  */
@@ -16908,6 +17021,16 @@ free_line_header (struct line_header *lh)
   xfree (lh);
 }
 
+/* Stub for free_line_header to match void * callback types.  */
+
+static void
+free_line_header_voidp (void *arg)
+{
+  struct line_header *lh = arg;
+
+  free_line_header (lh);
+}
+
 /* Add an entry to LH's include directory table.  */
 
 static void
@@ -17038,6 +17161,9 @@ dwarf_decode_line_header (unsigned int offset, struct dwarf2_cu *cu)
   back_to = make_cleanup ((make_cleanup_ftype *) free_line_header,
                           (void *) lh);
 
+  lh->offset.sect_off = offset;
+  lh->offset_in_dwz = cu->per_cu->is_dwz;
+
   line_ptr = section->buffer + offset;
 
   /* Read in the header.  */
@@ -17665,17 +17791,22 @@ dwarf_decode_lines_1 (struct line_header *lh, struct dwarf2_cu *cu,
    E.g. expand_line_sal requires this when finding psymtabs to expand.
    A good testcase for this is mb-inline.exp.
 
-   LOWPC is the lowest address in CU (or 0 if not known).  */
+   LOWPC is the lowest address in CU (or 0 if not known).
+
+   Boolean DECODE_MAPPING specifies we need to fully decode .debug_line
+   for its PC<->lines mapping information.  Otherwise only the filename
+   table is read in.  */
 
 static void
 dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
 		    struct dwarf2_cu *cu, struct partial_symtab *pst,
-		    CORE_ADDR lowpc)
+		    CORE_ADDR lowpc, int decode_mapping)
 {
   struct objfile *objfile = cu->objfile;
   const int decode_for_pst_p = (pst != NULL);
 
-  dwarf_decode_lines_1 (lh, cu, decode_for_pst_p, lowpc);
+  if (decode_mapping)
+    dwarf_decode_lines_1 (lh, cu, decode_for_pst_p, lowpc);
 
   if (decode_for_pst_p)
     {
@@ -21772,6 +21903,9 @@ dwarf2_free_objfile (struct objfile *objfile)
   if (dwarf2_per_objfile->quick_file_names_table)
     htab_delete (dwarf2_per_objfile->quick_file_names_table);
 
+  if (dwarf2_per_objfile->line_header_hash)
+    htab_delete (dwarf2_per_objfile->line_header_hash);
+
   /* Everything else should be on the objfile obstack.  */
 }
 

Re: [patchv3] Fix 100x slowdown regression on DWZ files

[Jan Kratochvil]

On Thu, 22 Jan 2015 19:45:08 +0100, Doug Evans wrote:
> In my mind it's easier to just treat a non-NULL value for line_header_hash
> as the flag to decide whether we're using the hash (instead of
> seen_partial_unit).

Yes, sure I agree.  I just haven't realized this simplificaton while coding it.


> Sound ok?

Yes, I am fine with your change; so I expect you can check it in when you ask
this way.


Thanks,
Jan

Re: [patchv3] Fix 100x slowdown regression on DWZ files

[Doug Evans]

On Thu, Jan 22, 2015 at 10:55 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Thu, 22 Jan 2015 19:45:08 +0100, Doug Evans wrote:
>> In my mind it's easier to just treat a non-NULL value for line_header_hash
>> as the flag to decide whether we're using the hash (instead of
>> seen_partial_unit).
>
> Yes, sure I agree.  I just haven't realized this simplificaton while coding it.
>
>
>> Sound ok?
>
> Yes, I am fine with your change; so I expect you can check it in when you ask
> this way.

Can you check it in?  Thanks.

[commit+7.9] [patchv3] Fix 100x slowdown regression on DWZ files

[Jan Kratochvil]

On Sat, 24 Jan 2015 02:46:45 +0100, Doug Evans wrote:
> Can you check it in?  Thanks.

Checked in:
	527f3840e1af8bc2e3173922ddae15d0021ed9b1 master
	0325b9bae8037aaa69cdfc8aa43a46678474f261 7.9


Thanks,
Jan

Re: [commit+7.9] [patchv3] Fix 100x slowdown regression on DWZ files

[Jan Kratochvil]

On Sat, 24 Jan 2015 15:53:52 +0100, Jan Kratochvil wrote:
> On Sat, 24 Jan 2015 02:46:45 +0100, Doug Evans wrote:
> > Can you check it in?  Thanks.
> 
> Checked in:
> 	527f3840e1af8bc2e3173922ddae15d0021ed9b1 master
> 	0325b9bae8037aaa69cdfc8aa43a46678474f261 7.9

I forgot to push the 7.9 branch so the new push is:
	c8f66196de03935d0d0dca7a5e16054262a38561 7.9


Jan