[PATCH 1/3] gdb/ctf: don't create a buildsym_compunit when building partial symbols

[PATCH 1/3] gdb/ctf: don't create a buildsym_compunit when building partial symbols

[Simon Marchi]

I am trying to do some changes to buildsym_compunit, so I am auditing
the current uses.  Something seems odd with this use of
buildsym_compunit (that this patch removes).

A buildsym_compunit is normally used when building a compunit_symtab.
That is, when expanding a partial symtab into a full compunit symtab.
In ctfread.c, a buildsym_compunit is created in ctf_start_archive, which
is only used when creating partial symtabs.  At this moment, I don't
see how that's useful.  ctf_start_archive creates a new
buildsym_compunit and starts a subfile.  But that buildsym_compunit is
never used again.  It's just overriden in ctf_start_symtab, which means
we leak the old buildsym_compunit, I suppose.

Remove ctf_start_archive completely.  Add an assert in
ctf_start_symtab to verify that we are not overwriting an existing
buildsym_compunit (meaning we'd leak the existing one).  This assert
triggers without the other part of the fix.  When doing:

  $ ./gdb --data-directory=data-directory /tmp/babeltrace-ctf/src/lib/.libs/libbabeltrace2.so.0.0.0
  ...
  (gdb) maintenance expand-symtabs
  /home/simark/src/binutils-gdb/gdb/ctfread.c:1255: internal-error: ctf_start_symtab: Assertion `!ccp->builder' failed.

Change-Id: I666d146454a019f08e7305f3a1c4a974d27b4592
---
 gdb/ctfread.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/gdb/ctfread.c b/gdb/ctfread.c
index 8da38cc70af8..8636692e2e1b 100644
--- a/gdb/ctfread.c
+++ b/gdb/ctfread.c
@@ -1531,21 +1531,6 @@ ctf_psymtab_var_cb (const char *name, ctf_id_t id, void *arg)
   return 0;
 }
 
-/* Start a subfile for CTF. FNAME is the name of the archive.  */
-
-static void
-ctf_start_archive (struct ctf_context *ccx, struct objfile *of,
-		   const char *fname)
-{
-  if (ccx->builder == nullptr)
-    {
-      ccx->builder = new buildsym_compunit (of,
-		      of->original_name, nullptr, language_c, 0);
-      ccx->builder->record_debugformat ("ctf");
-    }
-  ccx->builder->start_subfile (fname);
-}
-
 /* Setup partial_symtab's describing each source file for which
    debugging information is available.  */
 
@@ -1567,10 +1552,7 @@ scan_partial_symbols (ctf_dict_t *cfp, psymtab_storage *partial_symtabs,
 
   struct ctf_context *ccx = &pst->context;
   if (isparent == false)
-    {
-      ctf_start_archive (ccx, of, fname);
-      ccx->pst = pst;
-    }
+    ccx->pst = pst;
 
   if (ctf_type_iter (cfp, ctf_psymtab_type_cb, ccx) == CTF_ERR)
     complaint (_("ctf_type_iter scan_partial_symbols failed - %s"),

base-commit: 59f837cb11e3b96c581fe23b79f6995b7c8177ee
-- 
2.35.1

[PATCH 2/3] gdb: print compunit_symtab name in "maint info symtabs"

[Simon Marchi]

I think it would make sense to print a compunit_symtab's name in "maint
info symtabs".  If you are looking for a given CU in the list, that's
probably the field you will be looking at.  As the doc of
compunit_symtab::name says, it is not meant to be a reliable file name,
it is for debugging purposes (and "maint info symtabs" exists for
debugging purposes).

Sample output with the new field:

    (gdb) maintenance info symtabs
    { objfile /home/simark/build/binutils-gdb-one-target/gdb/a.out ((struct objfile *) 0x613000005d00)
      { ((struct compunit_symtab *) 0x621000131630)
        debugformat DWARF 5
        producer GNU C17 11.2.0 -mtune=generic -march=x86-64 -g3 -O0
        name test.c
        dirname /home/simark/build/binutils-gdb-one-target/gdb
        blockvector ((struct blockvector *) 0x621000131d10)
        user ((struct compunit_symtab *) (null))
            { symtab test.c ((struct symtab *) 0x6210001316b0)
              fullname (null)
              linetable ((struct linetable *) 0x621000131d40)
            }
            { symtab /home/simark/build/binutils-gdb-one-target/gdb/test.h ((struct symtab *) 0x6210001316e0)
              fullname (null)
              linetable ((struct linetable *) 0x0)
            }
            { symtab /usr/include/stdc-predef.h ((struct symtab *) 0x621000131710)
              fullname (null)
              linetable ((struct linetable *) 0x0)
            }
      }
      { ((struct compunit_symtab *) 0x6210001170a0)
        debugformat DWARF 5
        producer GNU C17 11.2.0 -mtune=generic -march=x86-64 -g3 -O0
        name foo.c
        dirname /home/simark/build/binutils-gdb-one-target/gdb
        blockvector ((struct blockvector *) 0x621000131580)
        user ((struct compunit_symtab *) (null))
            { symtab foo.c ((struct symtab *) 0x621000117120)
              fullname (null)
              linetable ((struct linetable *) 0x6210001315b0)
            }
            { symtab /usr/include/stdc-predef.h ((struct symtab *) 0x621000117150)
              fullname (null)
              linetable ((struct linetable *) 0x0)
            }
      }
    }

Change-Id: I17b87adfac2f6551cb5bda30d59f6c6882789211
---
 gdb/symmisc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/symmisc.c b/gdb/symmisc.c
index 37ecc8064da5..d33ea0fed9a4 100644
--- a/gdb/symmisc.c
+++ b/gdb/symmisc.c
@@ -781,6 +781,7 @@ maintenance_info_symtabs (const char *regexp, int from_tty)
 			gdb_printf ("    producer %s\n",
 				    (cust->producer () != nullptr
 				     ? cust->producer () : "(null)"));
+			gdb_printf ("    name %s\n", cust->name);
 			gdb_printf ("    dirname %s\n",
 				    (cust->dirname () != NULL
 				     ? cust->dirname () : "(null)"));
-- 
2.35.1

[PATCH 3/3] gdb/ctf: pass partial symtab's filename to buildsym_compunit

[Simon Marchi]

I noticed that the CTF symbol reader passes the objfile's name to all
buildsym_compunit instances it creates.  The result is that all
compunit_symtabs created have the same name, that of the objfile:

    { objfile /tmp/babeltrace-ctf/src/lib/.libs/libbabeltrace2.so.0.0.0 ((struct objfile *) 0x613000005d00)
      { ((struct compunit_symtab *) 0x621000286760)
        debugformat ctf
        producer (null)
        name libbabeltrace2.so.0.0.0
        dirname (null)
        blockvector ((struct blockvector *) 0x6210003911d0)
        user ((struct compunit_symtab *) (null))
            { symtab /tmp/babeltrace-ctf/src/lib/.libs/libbabeltrace2.so.0.0.0 ((struct symtab *) 0x6210003911f0)
              fullname (null)
              linetable ((struct linetable *) 0x0)
            }
      }
      { ((struct compunit_symtab *) 0x621000275c10)
        debugformat ctf
        producer (null)
        name libbabeltrace2.so.0.0.0
        dirname (null)
        blockvector ((struct blockvector *) 0x621000286710)
        user ((struct compunit_symtab *) (null))
            { symtab /tmp/babeltrace-ctf/src/lib/.libs/libbabeltrace2.so.0.0.0 ((struct symtab *) 0x621000286730)
              fullname (null)
              linetable ((struct linetable *) 0x0)
            }
      }

Notice the two "name libbabeltrace2.so.0.0.0".

Change it to pass the partial_symtab's filename instead.  The output
becomes:

    { objfile /tmp/babeltrace-ctf/src/lib/.libs/libbabeltrace2.so.0.0.0 ((struct objfile *) 0x613000005d00)
      { ((struct compunit_symtab *) 0x621000295610)
        debugformat ctf
        producer (null)
        name libbabeltrace2.so.0.0.0
        dirname (null)
        blockvector ((struct blockvector *) 0x6210003a15d0)
        user ((struct compunit_symtab *) (null))
            { symtab /tmp/babeltrace-ctf/src/lib/.libs/libbabeltrace2.so.0.0.0 ((struct symtab *) 0x6210003a15f0)
              fullname (null)
              linetable ((struct linetable *) 0x0)
            }
      }
      { ((struct compunit_symtab *) 0x621000288700)
        debugformat ctf
        producer (null)
        name current-thread.c
        dirname (null)
        blockvector ((struct blockvector *) 0x6210002955c0)
        user ((struct compunit_symtab *) (null))
            { symtab /home/simark/src/babeltrace/src/lib/current-thread.c ((struct symtab *) 0x6210002955e0)
              fullname (null)
              linetable ((struct linetable *) 0x0)
            }
      }

Note that the first compunit_symtab still has libbabeltrace2.so.0.0.0 as
its name.  This is because the CTF symbol reader really creates a
partial symtab named like this.  It appears to be because the debug info
contains information that has been factored out of all CUs and is at the
"top-level" of the objfile, outside any real CU.  So it creates a
partial symtab and an artificial CU that's named after the objfile.

Change-Id: I576316bab2a3668adf87b4e6cebda900a8159b1b
---
 gdb/ctfread.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/ctfread.c b/gdb/ctfread.c
index 8636692e2e1b..7f7e09638a40 100644
--- a/gdb/ctfread.c
+++ b/gdb/ctfread.c
@@ -1253,7 +1253,7 @@ ctf_start_symtab (ctf_psymtab *pst,
 
   ccp = &pst->context;
   ccp->builder = new buildsym_compunit
-		       (of, of->original_name, nullptr,
+		       (of, pst->filename, nullptr,
 		       language_c, text_offset);
   ccp->builder->record_debugformat ("ctf");
 }
-- 
2.35.1

Re: [PATCH 3/3] gdb/ctf: pass partial symtab's filename to buildsym_compunit

[Wei-min Pan]

On 3/31/2022 2:20 PM, Simon Marchi wrote:
> I noticed that the CTF symbol reader passes the objfile's name to all
> buildsym_compunit instances it creates.  The result is that all
> compunit_symtabs created have the same name, that of the objfile:
>
>      { objfile /tmp/babeltrace-ctf/src/lib/.libs/libbabeltrace2.so.0.0.0 ((struct objfile *) 0x613000005d00)
>        { ((struct compunit_symtab *) 0x621000286760)
>          debugformat ctf
>          producer (null)
>          name libbabeltrace2.so.0.0.0
>          dirname (null)
>          blockvector ((struct blockvector *) 0x6210003911d0)
>          user ((struct compunit_symtab *) (null))
>              { symtab /tmp/babeltrace-ctf/src/lib/.libs/libbabeltrace2.so.0.0.0 ((struct symtab *) 0x6210003911f0)
>                fullname (null)
>                linetable ((struct linetable *) 0x0)
>              }
>        }
>        { ((struct compunit_symtab *) 0x621000275c10)
>          debugformat ctf
>          producer (null)
>          name libbabeltrace2.so.0.0.0
>          dirname (null)
>          blockvector ((struct blockvector *) 0x621000286710)
>          user ((struct compunit_symtab *) (null))
>              { symtab /tmp/babeltrace-ctf/src/lib/.libs/libbabeltrace2.so.0.0.0 ((struct symtab *) 0x621000286730)
>                fullname (null)
>                linetable ((struct linetable *) 0x0)
>              }
>        }
>
> Notice the two "name libbabeltrace2.so.0.0.0".
>
> Change it to pass the partial_symtab's filename instead.  The output
> becomes:
>
>      { objfile /tmp/babeltrace-ctf/src/lib/.libs/libbabeltrace2.so.0.0.0 ((struct objfile *) 0x613000005d00)
>        { ((struct compunit_symtab *) 0x621000295610)
>          debugformat ctf
>          producer (null)
>          name libbabeltrace2.so.0.0.0
>          dirname (null)
>          blockvector ((struct blockvector *) 0x6210003a15d0)
>          user ((struct compunit_symtab *) (null))
>              { symtab /tmp/babeltrace-ctf/src/lib/.libs/libbabeltrace2.so.0.0.0 ((struct symtab *) 0x6210003a15f0)
>                fullname (null)
>                linetable ((struct linetable *) 0x0)
>              }
>        }
>        { ((struct compunit_symtab *) 0x621000288700)
>          debugformat ctf
>          producer (null)
>          name current-thread.c
>          dirname (null)
>          blockvector ((struct blockvector *) 0x6210002955c0)
>          user ((struct compunit_symtab *) (null))
>              { symtab /home/simark/src/babeltrace/src/lib/current-thread.c ((struct symtab *) 0x6210002955e0)
>                fullname (null)
>                linetable ((struct linetable *) 0x0)
>              }
>        }
>
> Note that the first compunit_symtab still has libbabeltrace2.so.0.0.0 as
> its name.  This is because the CTF symbol reader really creates a
> partial symtab named like this.  It appears to be because the debug info
> contains information that has been factored out of all CUs and is at the
> "top-level" of the objfile, outside any real CU.  So it creates a
> partial symtab and an artificial CU that's named after the objfile.
>
> Change-Id: I576316bab2a3668adf87b4e6cebda900a8159b1b
> ---
>   gdb/ctfread.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/ctfread.c b/gdb/ctfread.c
> index 8636692e2e1b..7f7e09638a40 100644
> --- a/gdb/ctfread.c
> +++ b/gdb/ctfread.c
> @@ -1253,7 +1253,7 @@ ctf_start_symtab (ctf_psymtab *pst,
>   
>     ccp = &pst->context;
>     ccp->builder = new buildsym_compunit
> -		       (of, of->original_name, nullptr,
> +		       (of, pst->filename, nullptr,
>   		       language_c, text_offset);

Looks good. Thanks.

>     ccp->builder->record_debugformat ("ctf");
>   }

Re: [PATCH 1/3] gdb/ctf: don't create a buildsym_compunit when building partial symbols

[Wei-min Pan]

On 3/31/2022 2:20 PM, Simon Marchi wrote:
> I am trying to do some changes to buildsym_compunit, so I am auditing
> the current uses.  Something seems odd with this use of
> buildsym_compunit (that this patch removes).
>
> A buildsym_compunit is normally used when building a compunit_symtab.
> That is, when expanding a partial symtab into a full compunit symtab.
> In ctfread.c, a buildsym_compunit is created in ctf_start_archive, which
> is only used when creating partial symtabs.  At this moment, I don't
> see how that's useful.  ctf_start_archive creates a new
> buildsym_compunit and starts a subfile.  But that buildsym_compunit is
> never used again.  It's just overriden in ctf_start_symtab, which means
> we leak the old buildsym_compunit, I suppose.
>
> Remove ctf_start_archive completely.  Add an assert in
> ctf_start_symtab to verify that we are not overwriting an existing
> buildsym_compunit (meaning we'd leak the existing one).  This assert
> triggers without the other part of the fix.  When doing:
>
>    $ ./gdb --data-directory=data-directory /tmp/babeltrace-ctf/src/lib/.libs/libbabeltrace2.so.0.0.0
>    ...
>    (gdb) maintenance expand-symtabs
>    /home/simark/src/binutils-gdb/gdb/ctfread.c:1255: internal-error: ctf_start_symtab: Assertion `!ccp->builder' failed.
>
> Change-Id: I666d146454a019f08e7305f3a1c4a974d27b4592
> ---
>   gdb/ctfread.c | 20 +-------------------
>   1 file changed, 1 insertion(+), 19 deletions(-)
>
> diff --git a/gdb/ctfread.c b/gdb/ctfread.c
> index 8da38cc70af8..8636692e2e1b 100644
> --- a/gdb/ctfread.c
> +++ b/gdb/ctfread.c
> @@ -1531,21 +1531,6 @@ ctf_psymtab_var_cb (const char *name, ctf_id_t id, void *arg)
>     return 0;
>   }
>   
> -/* Start a subfile for CTF. FNAME is the name of the archive.  */
> -
> -static void
> -ctf_start_archive (struct ctf_context *ccx, struct objfile *of,
> -		   const char *fname)
> -{
> -  if (ccx->builder == nullptr)
> -    {
> -      ccx->builder = new buildsym_compunit (of,
> -		      of->original_name, nullptr, language_c, 0);
> -      ccx->builder->record_debugformat ("ctf");
> -    }
> -  ccx->builder->start_subfile (fname);
> -}
> -
>   /* Setup partial_symtab's describing each source file for which
>      debugging information is available.  */
>   
> @@ -1567,10 +1552,7 @@ scan_partial_symbols (ctf_dict_t *cfp, psymtab_storage *partial_symtabs,
>   
>     struct ctf_context *ccx = &pst->context;
>     if (isparent == false)
> -    {
> -      ctf_start_archive (ccx, of, fname);
> -      ccx->pst = pst;
> -    }
> +    ccx->pst = pst;
>   
>     if (ctf_type_iter (cfp, ctf_psymtab_type_cb, ccx) == CTF_ERR)
>       complaint (_("ctf_type_iter scan_partial_symbols failed - %s"),

A CTF archive contains conflicting type info and is treated as a CU.
Since one can't switch scope among CUs in CTF, not reading archives
might not be a problem. Please go ahead and make the change. It can
be revisited when we come up a test case that shows otherwise.

Thanks.

>
> base-commit: 59f837cb11e3b96c581fe23b79f6995b7c8177ee

Re: [PATCH 1/3] gdb/ctf: don't create a buildsym_compunit when building partial symbols

[Simon Marchi]

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

> A CTF archive contains conflicting type info and is treated as a CU.
> Since one can't switch scope among CUs in CTF, not reading archives
> might not be a problem. Please go ahead and make the change. It can
> be revisited when we come up a test case that shows otherwise.
> 
> Thanks.

Hi Wei-Min,

I'm not sure we are on the same page.  I don't know much about CTF, so
let's make sure I understand things right.  I am not sure how CTF debug
info is organized, and in particular what an "archive" is.  If you can
explain this or give a reference, that would be useful.

I'll attach the output of "readelf --ctf=.ctf" for the file I am working
with.

My understanding from the readelf output is that there is some debug
info (function, types, etc) described at the top-level.  It doesn't seem
associated to a source file, and it seems to contain things that are
global.

Then there is one "archive" for each compiled .c file, such as:

  CTF archive member: /home/simark/src/babeltrace/src/lib/current-thread.c:

Each archive contains very little data, my guess is that it only
contains what is not already described at the top-level?

Right now, in GDB, we create one CU (partial_symtab, and then
compunit_symtab after expansion) for the top-level, and the one CU for
each archive.  My patch does not change that.  It only removes a
"new buildsym_compunit" call that just seems useless / to do nothing.

In other words, no user-visible changes are expected from this patch.

Simon

[-- Attachment #2: readelf-output.txt.gz --]
[-- Type: application/gzip, Size: 73551 bytes --]

Re: [PATCH 1/3] gdb/ctf: don't create a buildsym_compunit when building partial symbols

[Wei-min Pan]

Hi Simon,

On 3/31/2022 5:14 PM, Simon Marchi wrote:
>> A CTF archive contains conflicting type info and is treated as a CU.
>> Since one can't switch scope among CUs in CTF, not reading archives
>> might not be a problem. Please go ahead and make the change. It can
>> be revisited when we come up a test case that shows otherwise.
>>
>> Thanks.
> Hi Wei-Min,
>
> I'm not sure we are on the same page.  I don't know much about CTF, so
> let's make sure I understand things right.  I am not sure how CTF debug
> info is organized, and in particular what an "archive" is.  If you can
> explain this or give a reference, that would be useful.

Here is the spec for CTF V3:

http://www.esperi.org.uk/~oranix/ctf/ctf-spec.pdf

which hopefully is helpful.

>
> I'll attach the output of "readelf --ctf=.ctf" for the file I am working
> with.
>
> My understanding from the readelf output is that there is some debug
> info (function, types, etc) described at the top-level.  It doesn't seem
> associated to a source file, and it seems to contain things that are
> global.
>
> Then there is one "archive" for each compiled .c file, such as:
>
>    CTF archive member: /home/simark/src/babeltrace/src/lib/current-thread.c:
>
> Each archive contains very little data, my guess is that it only
> contains what is not already described at the top-level?

Yes, that's my understanding too.

>
> Right now, in GDB, we create one CU (partial_symtab, and then
> compunit_symtab after expansion) for the top-level, and the one CU for
> each archive.  My patch does not change that.  It only removes a
> "new buildsym_compunit" call that just seems useless / to do nothing.
>
> In other words, no user-visible changes are expected from this patch.

OK. Thanks.

>
> Simon

Re: [PATCH 1/3] gdb/ctf: don't create a buildsym_compunit when building partial symbols

[Simon Marchi]

> Here is the spec for CTF V3:
> 
> http://www.esperi.org.uk/~oranix/ctf/ctf-spec.pdf
> 
> which hopefully is helpful.

Thanks, I read the first section (CTF archives), I think it somewhat
matches what I thought.  I'm not sure about how the details of how types
are deduplicated exactly, but that's not relevant for this patch.

>> I'll attach the output of "readelf --ctf=.ctf" for the file I am working
>> with.
>>
>> My understanding from the readelf output is that there is some debug
>> info (function, types, etc) described at the top-level.  It doesn't seem
>> associated to a source file, and it seems to contain things that are
>> global.
>>
>> Then there is one "archive" for each compiled .c file, such as:
>>
>>    CTF archive member: /home/simark/src/babeltrace/src/lib/current-thread.c:
>>
>> Each archive contains very little data, my guess is that it only
>> contains what is not already described at the top-level?
> 
> Yes, that's my understanding too.
> 
>>
>> Right now, in GDB, we create one CU (partial_symtab, and then
>> compunit_symtab after expansion) for the top-level, and the one CU for
>> each archive.  My patch does not change that.  It only removes a
>> "new buildsym_compunit" call that just seems useless / to do nothing.
>>
>> In other words, no user-visible changes are expected from this patch.
> 
> OK. Thanks.

Thanks, I will push the series, let me know if you see something
breaking.

Simon

Re: [PATCH 1/3] gdb/ctf: don't create a buildsym_compunit when building partial symbols

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> In ctfread.c, a buildsym_compunit is created in ctf_start_archive, which
Simon> is only used when creating partial symtabs.  At this moment, I don't
Simon> see how that's useful.

It probably can't be, as those are different phases.

TBH, ctfread would probably be improved by not using psymtabs at all.
That is, if libctf provides by-name lookups, then there's really no need
to even have a psymtab.

Tom