Re: [PATCH] gdb: CTF support

[Wei-min Pan <weimin.pan@oracle.com>]

On 7/24/2019 12:56 PM, Tom Tromey wrote:
>
> >>>>> ">" == Weimin Pan <weimin.pan@oracle.com> writes:
>
> >> This patch adds the CTF (Compact Ansi-C Type Format) support in gdb.
>
> Thank you for the patch.
>
> >> Two submissions on which this gdb work depends were posted earlier
> >> in May:
>
> >>  * On the binutils mailing list - adding libctf which creates, updates,
> >>    reads, and manipulates the CTF data.
>
> I suspect the top-level Makefile.def / Makefile.in will need to be
> modified to ensure that libctf is built before gdb.  The only dependency
> I see on libctf right now is for binutils:
>
>     dependencies = { module=all-binutils; on=all-libctf; };

There are more libctf changes in both Makefile.def and Makefile.in.
Please see commit 0e65dfbaf3a0299e4837216a103c28625d4b4f1d which
should address your concern?

> >> The two-stage symbolic reading and setting strategy, partial and
> >> full, was used.
>
> Is there a benefit to making partial symbol tables for CTF?  It's fine
> to do it the way you've done it, but I'm interested in the reasoning.

Currently ctf toolchain supports the ctf info for single CU only but
is expected to be expanded to support multiple CU's. At which point,
handling partial symbol table will make more sense.

> >> +#include "buildsym-legacy.h"
>
> It's better to use the new buildsym.h if you possibly can.
> And, if you can't, it would be better to improve it.

Good catch, this item has been on my to-do list but I didn't get a
chance to really look into buildsym.h or fully understand it.

> >> +#include <include/ctf.h>
>
> Probably just "ctf.h" here?  The top-level include directory is already
> on the include path.

There is already a gdb/ctf.h for tracing?

> >> +#include <ctf-api.h>
>
> Probably "ctf-api.h" here.

Done.

> >> +static const struct objfile_data *ctf_tid_key;
> >> +static const struct objfile_data *ctf_file_key;
>
> There's a type-safe registry API now.  I would prefer that for new code.

Where can I get more info on this API?

> >> +/* The routines that read and process fields/members of a C struct, union,
> >> +   or enumeration, pass lists of data member fields in an instance of a
> >> +   field_info structure. It is dervied from dwarf2read.c.  */
>
> Typo, "derived".

Corrected.

> >> +struct nextfield
> >> +{
> >> +  struct field field {};
> >> +};
>
> IMO you might as well just remove this wrapper struct.

Since it's likely that CTF will be expanded to support C++, new members
such as "virtuality" and "accessibility" might be added to this struct.

> >> +/* Hash function for a ctf_tid_and_type.  */
> >> +
> >> +struct ctf_tid_and_type
> >> +{
>
> That comment is slightly misplaced.

It's been moved down to function tid_and_type_hash.

> >> +static struct type *
> >> +set_tid_type (struct objfile *of, ctf_id_t tid, struct type *typ)
> >> +{
> >> +  htab_t htab;
> >> +  htab = (htab_t) objfile_data (of, ctf_tid_key);
> >> +  if (htab == NULL)
> >> +    {
> >> +      htab = htab_create_alloc_ex (1, tid_and_type_hash,
> >> +                                   tid_and_type_eq,
> >> +                                   NULL, &of->objfile_obstack,
> >> +                                   hashtab_obstack_allocate,
> >> +                                   dummy_obstack_deallocate);
> >> +      set_objfile_data (of, ctf_tid_key, htab);
>
> A few things here.
>
> First, I think it's best not to allocate hash tables on the objfile
> obstack.  This approach means a memory leak (not detectable though) when
> the hash table is resized.  It's just as convenient to use
> xcalloc/xfree.  With the type-safe registry you can use htab_deleter
> for the keys's deleter; see elfread.c:elf_objfile_gnu_ifunc_cache_data.

Will take a look at elfread.c. Thanks.

> Second, is this hash table needed only when expanding symbols and/or
> creating psymtabs, or is it needed longer term?  I am not familiar with
> CTF and I didn't read all parts of the patch in detail.  Anyway I ask
> because if it is temporary, it's even better not to stash it in the
> objfile but rather just create it while reading and destroy it when
> done.

It's needed only when creating psymtabs and expanding symbols. BTW I
borrowed this idea from dwarf2read.c. So you recommend that I use
xcalloc/xfree instead?

> Unfortunately it isn't possible, yet, to allocate a type on the BFD
> rather than the objfile, or else I would suggest that here.
>
> >> +  *slot = XOBNEW (&of->objfile_obstack, struct ctf_tid_and_type);
>
> An addendum to the above: if it's not possible to remove items from the
> hash, then it is fine to store the node itself on the obstack.
>
> >> +static int
> >> +get_bitsize (ctf_file_t *fp, ctf_id_t tid, uint32_t kind)
> >> +{
> >> +  ctf_encoding_t cet;
> >> +
> >> +  if (((kind == CTF_K_INTEGER) || (kind == CTF_K_ENUM)
> >> +      || (kind == CTF_K_FLOAT))
>
> The gdb style uses fewer parens here.

Corrected.

>
> >> +      && ctf_type_reference (fp, tid) != CTF_ERR
> >> +      && ctf_type_encoding (fp, tid, &cet) != CTF_ERR)
> >> +    {
> >> +      return (cet.cte_bits);
>
> gdb also doesn't use "()" for a return, unless it spans multiple lines.
> There were a few instances of this.

OK, fixed this in several places. So we don't want "()" in stmt like:

   return (ids_lhs->tid == ids_rhs->tid);

>
> >> +      add_symbol_to_list (sym, get_file_symbols ());
>
> get_file_symbols means the top-level "static" scope.  Is this what you
> intended?  Or was it supposed to be the global (external) scope?

I'm pretty certain that is the intention. But will take another look to
see if I need get_global_symbols.

>
> thanks,
> Tom

Thank you very much for your comments.

Weimin