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