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

  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).

Tested on x86_64-redhat-linux and aarch64-linux-gnu.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
---
    v1 -> v2:
      - Add back the in-process die RAII object to protect against
        infinite recursion.

 gdb/dwarf2/cu.h   |   6 ---
 gdb/dwarf2/read.c | 108 +++++++++++++++++++++++++++-------------------
 gdb/dwarf2/read.h |   9 ++++
 3 files changed, 72 insertions(+), 51 deletions(-)
  

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
  

On Mon, Apr 29, 2024 at 10:37 AM Simon Marchi <simark@simark.ca> wrote:
>
> 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?

I *sincerely* thought about this! If you think that it is a good idea,
then I can incorporate this into a v3. Based on my understanding of
the code, I think that this is reasonable. The only problem is that
(as I understand it), there could be a situation where the hash does
not have space and we could lose the reuse. (see
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/dwarf2/read.c;h=7eacafc25b5e5f5202769b18175ed510a6787489;hb=43ca4ec2996a6bfbb8726f1c753aeb85fe0f92a4#l7401)

>
> 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?

You are missing nothing ... at this point. However, the goal of this
is to make it easier to read the line header earlier in the decoding
process. We will need to read the line header earlier as part of the
work on supporting embedded source code in the dwarf file. There is
some discussion about that here:
https://sourceware.org/pipermail/gdb-patches/2024-April/208614.html

If we have this "ensure" function then we can simply call it at
find_file_and_directory and we will ensure that we do not have
multiple decodings.

Does that make sense? As I said, I am only trying to help and do not
want to upset anything!

>
> I noted a few random comments below.

Thank you!! I will absolutely make these changes in a v3.

I sincerely appreciate the feedback and hope that what I am offering
is helpful! I am really enjoying the process of contributing!

Will


>
> > @@ -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
  

On Mon, Apr 29, 2024 at 11:50 AM Will Hawkins <hawkinsw@obs.cr> wrote:
>
> On Mon, Apr 29, 2024 at 10:37 AM Simon Marchi <simark@simark.ca> wrote:
> >
> > 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?
>
> I *sincerely* thought about this! If you think that it is a good idea,
> then I can incorporate this into a v3. Based on my understanding of
> the code, I think that this is reasonable. The only problem is that
> (as I understand it), there could be a situation where the hash does
> not have space and we could lose the reuse. (see
> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/dwarf2/read.c;h=7eacafc25b5e5f5202769b18175ed510a6787489;hb=43ca4ec2996a6bfbb8726f1c753aeb85fe0f92a4#l7401)

I have a v3 that is almost ready that combines all the caching into
the line_header_hash. I hope that I will have it ready to send out
tomorrow! Thank you for your patience!

>
> >
> > 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?
>
> You are missing nothing ... at this point. However, the goal of this
> is to make it easier to read the line header earlier in the decoding
> process. We will need to read the line header earlier as part of the
> work on supporting embedded source code in the dwarf file. There is
> some discussion about that here:
> https://sourceware.org/pipermail/gdb-patches/2024-April/208614.html
>
> If we have this "ensure" function then we can simply call it at
> find_file_and_directory and we will ensure that we do not have
> multiple decodings.
>
> Does that make sense? As I said, I am only trying to help and do not
> want to upset anything!
>
> >
> > I noted a few random comments below.
>
> Thank you!! I will absolutely make these changes in a v3.
>
> I sincerely appreciate the feedback and hope that what I am offering
> is helpful! I am really enjoying the process of contributing!
>
> Will
>
>
> >
> > > @@ -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