This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] gdb: CTF support


>>>>> ">" == 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; };

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

>> +#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.

>> +#include <include/ctf.h>

Probably just "ctf.h" here?  The top-level include directory is already
on the include path.

>> +#include <ctf-api.h>

Probably "ctf-api.h" here.

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

>> +/* 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".

>> +struct nextfield
>> +{
>> +  struct field field {};
>> +};

IMO you might as well just remove this wrapper struct.

>> +/* Hash function for a ctf_tid_and_type.  */
>> +
>> +struct ctf_tid_and_type
>> +{

That comment is slightly misplaced.

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

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.

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.

>> +      && 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.

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

thanks,
Tom


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]