Re: [PATCH v2] gdb: Cache line headers in DWARF per CU

[Simon Marchi <simark@simark.ca>]

On 4/29/24 9:59 AM, Will Hawkins wrote:
> When the line headers of a DWARF unit are read, make sure that they are
> available for reuse by storing them in the per CU. A view of that data
> structure will be given to each of the dwarf2_cus as they are
> instantiated for reading debugging information. As a result, we can
> simplify the scoped RAII object (by removing code for deallocating a
> line header upon the completion of reading DWARF debugging information).

My understanding is that prior to this change, the lifetime of the
line_header objects that were not for partial units was approximately
equal to the lifetime of the dwarf2_cu object.  After this change, the
lifetime of those line_header objects becomes tied to the
dwarf2_per_cu_data object, which lives essentially as long as the
dwarf2_per_objfile object.

If so, could we simplify things and store all line_header objects in
dwarf2_per_objfile::line_header_hash?

Also, could you provide an explanation of why this is useful, when does
the re-use happens?  Naively, I would think that the line header is used
once when expanding the CU, creating the symtabs, and not used later.
What am I missing?

I noted a few random comments below.

> @@ -7312,29 +7302,38 @@ find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu)
>    return *cu->per_cu->fnd;
>  }
>  
> -/* Handle DW_AT_stmt_list for a compilation unit.
> -   DIE is the DW_TAG_compile_unit die for CU.
> -   COMP_DIR is the compilation directory.  LOWPC is passed to
> -   dwarf_decode_lines.  See dwarf_decode_lines comments about it.  */
> -
>  static void
> -handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
> -			const file_and_directory &fnd, unrelocated_addr lowpc,
> -			bool have_code) /* ARI: editCase function */
> +ensure_line_header_read (struct dwarf2_cu *cu, struct die_info *die,

I would suggest dropping the `struct ` keyword where possible.  DIE
could be const I think.

> +			 const file_and_directory &fnd,
> +			 sect_offset line_offset)
>  {
>    dwarf2_per_objfile *per_objfile = cu->per_objfile;
> -  struct attribute *attr;
> +  dwarf2_per_cu_data *per_cu = cu->per_cu;
>    hashval_t line_header_local_hash;
>    void **slot;
> -  int decode_mapping;
> -
> -  gdb_assert (! cu->per_cu->is_debug_types);
> -
> -  attr = dwarf2_attr (die, DW_AT_stmt_list, cu);
> -  if (attr == NULL || !attr->form_is_unsigned ())
> -    return;
>  
> -  sect_offset line_offset = (sect_offset) attr->as_unsigned ();
> +  /* There are two places for storing/finding line headers:
> +     1. Per CU: When DIE is not a partial unit, the decoded
> +	line header is owned by the per CU. In this case, the

We use two spaces after periods in text.

> +	job of this function is to simply give out a copy of
> +	that pointer to CU -- the per cu outlives CU so this
> +	lending is safe.
> +     2. Line Header Hash: When the DIE is a partial unit, the
> +	decoded line header is owned by the line header hash.
> +	In this case, the job of this function is to simply
> +	give out a copy of the appropriate entry in the hash.
> +	Caveat: If a decoded line header of a partial unit
> +	does not fit in the line header hash, it will be
> +	stored in/owned by the per cu.
> +    Of course, the first time that the line header is decoded
> +    it must be put in the proper place according to the rules
> +    above. That is the other job of this function.  */
> +
> +  if (per_cu->line_headers != NULL)

NULL -> nullptr

> +    {
> +      cu->line_header = per_cu->line_headers.get ();
> +      return;
> +    }
>  
>    /* The line header hash table is only created if needed (it exists to
>       prevent redundant reading of the line table for partial_units).
> @@ -7378,9 +7377,6 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
>    if (lh == NULL)
>      return;
>  
> -  cu->line_header = lh.release ();
> -  cu->line_header_die_owner = die;
> -
>    if (per_objfile->line_header_hash == NULL)
>      slot = NULL;
>    else
> @@ -7394,18 +7390,43 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
>      {
>        /* This newly decoded line number information unit will be owned
>  	 by line_header_hash hash table.  */
> -      *slot = cu->line_header;
> -      cu->line_header_die_owner = NULL;
> +      *slot = cu->line_header = lh.release ();
>      }
>    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.  */
> +	 may be already used by multiple CUs.  Therefore, this newly read line
> +	 header will be owned by the per_cu.  */
>        gdb_assert (die->tag != DW_TAG_partial_unit);
> +
> +      per_cu->line_headers = std::move (lh);
> +      cu->line_header = per_cu->line_headers.get ();
>      }

If the design stayed as-is, I would try to put:

  cu->line_header = lh.get ();

... before the condition, since that part doesn't depend on the
condition.

> @@ -222,6 +223,14 @@ struct dwarf2_per_cu_data
>       have one.  */
>    std::unique_ptr<file_and_directory> fnd;
>  
> +  /* The decoded line headers for this CU.  This is cached so that
> +     there is no need to refetch it repeatedly. ensure_line_headers_read
> +     in read.c is responsible for transferring a view of this structure
> +     to dwarf2_cus as they are instantiated. This may be nullptr when
> +     the decoded line header is owned by the line header hash associated
> +     with the per objfile in the presence of a partial unit.  */
> +  std::unique_ptr<struct line_header> line_headers;

Use line_header_up as the type.

Simon