Re: [PATCH v5] gdb: Support embedded source in DWARF

[Will Hawkins <hawkinsw@obs.cr>]

On Fri, Apr 19, 2024 at 4:18 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Will" == Will Hawkins <hawkinsw@obs.cr> writes:
>
> Will> While DW_LNCT_source is not yet finalized in the DWARF standard
> Will> (https://dwarfstd.org/issues/180201.1.html), LLVM does emit it.
>
> Will> This patch adds support for it in gdb.
>
> Thanks.

Sorry for the delay responding. I went back and did a much deeper dive
into the source code so that I could get a better idea of how each of
the components interacted to make myself feel more confident that I am
doing "the right thing".

>
> Will> -      m_fullname = find_source_or_rewrite (get_name (), get_comp_dir ());
> Will> +      {
> Will> + if (m_is_embedded)
> Will> +   m_fullname = make_unique_xstrdup (embedded_fullname (
> Will> +                                                        get_name (),
> Will> +                                                        get_comp_dir ()));
>
> No line break before the get_name call.
>
> Will> +   else if (entry.source != nullptr)
> Will> +     {
> Will> +       /* We have an embedded source for the CU.  */
> Will> +       gdb_assert (offset == 1);
> Will> +       cu_file_embedded = true;
>
> This assert makes me a bit nervous.  Can this be fooled by bad input
> data?  Or is it really testing some internal invariant?  I couldn't
> convince myself it was the latter.
>
> Will> + fullname.reset (embedded_fullname (dirname, qfn->file_names[index]));
>
> fullname = ...
>
> Will> +  /* Because the line header may tell us information about the CU
> Will> +     filename (e.g., whether it is embedded) which will affect other
> Will> +     calculations, we have to read that information here.  */
> Will> +  line_header *lh = cu->line_header;
> Will> +  struct attribute *attr = dwarf2_attr (die, DW_AT_stmt_list, cu);
> Will> +  if (lh == nullptr && attr != nullptr && attr->form_is_unsigned ())
> Will> +    {
> Will> +      sect_offset line_offset = (sect_offset) attr->as_unsigned ();
> Will> +      line_header_up lhu = dwarf_decode_line_header (line_offset, cu,
> Will> +                              res.get_comp_dir ());
> Will> +      if (lhu != nullptr)
> Will> +   lh = lhu.release();
>
> This will leak memory.
>
> However if this is the first time reading the header, should it be
> stashed somewhere?  It seems to me it shouldn't have to be read more
> than once.
>
> IOW, how does this case get reached?

As I was doing my additional research and attempting to figure out a
good response to this question, I came to the conclusion that there
are several places where dwarf_decode_line_header is invoked and the
results of those invocations are inconsistently stored for later
reuse. Like there is the line_header_hash_table for deduplicating line
header decoding in a partial unit, I am working on a change that will
give dwarf2_per_cu the power to own a line_header that can be shared
among each of dwarf2_cus instantiated for the various cutu_reader
instances. As it stands (still being debugged), the ability to own a
decoded line header in a per CU is a supplement to the line header
hash table in the per objfile -- if there is a line header hash table
and the line header belongs to a partial unit, then that owns the
decoded line header; if not (or there is a conflict in the hash
table), the per CU owns the decoded line header. With that change,
there is no need for instances of dwarf2_cu to own a decoded line
header which means that it is possible to remove the process_die_scope
class.

You all are the experts here and I am a new person looking at this
code. I would love to hear what you think about that possible
approach. It seems to increase the reuse of a data structure that is
expensive to build and reduce some code complexity by removing the
RAII process_die_scope.

Again, though, I am not nearly as smart as you all. That said, I have
had a tremendously enjoyable time studying the code and would be more
than willing to either

1. Incorporate this change into the changes needed to support embedded
source code in DWARF, or
2. Make it a separate patch entirely.

Sorry again for the delay responding!
Will


Will

>
> Will> +/* See source.h.  */
> Will> +
> Will> +char *
> Will> +embedded_fullname (const char *dirname, const char *filename)
>
> Should return a unique_xmalloc_ptr<char>
>
> Will> +{
> Will> +  if (dirname != nullptr)
> Will> +    {
> Will> +      return concat (dirname, SLASH_STRING, filename, (char *) nullptr);
> Will> +    }
>
> No braces needed.
>
> Will> +
> Will> +  return xstrdup (filename);
>
> make_unique_xstrdup
>
> thanks,
> Tom