Re: [PATCH v2] gdb: CTF support

[Weimin Pan <weimin dot pan at oracle dot com>]

On 9/29/2019 8:06 PM, Simon Marchi wrote:
> Hi Weimin,
>
> I am done reading ctfread.c, I noted a few comments below.
>
> There are a few issues with styling as well, most importantly:
>
> - The indentation
> - Use "= NULL" / "!= NULL" when testing a pointer
>
> Instead of pointing them out individually, I've fixed what I saw as I 
read
> the patch, see the attached patch.  If you use it, please read through it
> carefully and don't assume I haven't made mistakes :).

Hi Simon,

Thank you very much for the thorough review. I have read through the
modified patch which looks good and will be adopted. In addition, I
have made more changes to address the issues you raised. Please see
below and the updated patch (attached).

>> +struct field_info
>> +  {
>> +    /* List of data member fields.  */
>> +    std::vector<struct nextfield> fields;
>> +
>> +    /* Number of fields.  */
>> +    int nfields = 0;
>
> This field seems unnecessary, as it represents the size of the vector 
above (which you can get with fields.size()).

You're right, the field is not needed and is removed.

>
>> +/* Callback to add member NAME to a struct/union type. TID is the type
>> +   of struct/union member, OFFSET is the offset of member in bits
>> +   (gdbarch_bits_big_endian(), and ARG contains the field_info.  */
>
> This comment (the gdbarch_bits_big_endian part) seems incomplete).

Updated.

>
>> +/* Callback to add member NAME of EVAL to an enumeration type.
>> +   ARG contains the field_info.  */
>> +
>> +static int
>> +ctf_add_enum_member_cb (const char *name, int eval, void *arg)
>
> I had to search why that variable was called EVAL.  It means "enum 
value", but I
> automatically associate "eval" to "evaluate".  I'd suggest renaming 
it to "enum_value"
> or "value" for clarity.

OK, using "enum_value".

>
>> +/* Add a new symbol entry, with its name from TP, its access index and
>> +   domain from TP's kind, and its type from TPYE.  */
>
> This comment refers to TP, which doesn't exist, so it seems 
outdated.  Also,
> watch the typo "TPYE".

Fixed.

>
>> +
>> +static struct symbol *
>> +new_symbol (ctf_context_t *ccp, struct type *type, ctf_id_t tid)
>> +{
>> +  struct objfile *objfile = ccp->of;
>> +  ctf_file_t *fp = ccp->fp;
>> +  struct symbol *sym = NULL;
>> +
>> +  const char *name = ctf_type_aname_raw (fp, tid);
>> +  if (name)
>> +    {
>> +      sym = allocate_symbol (objfile);
>> +      OBJSTAT (objfile, n_syms++);
>> +
>> +      SYMBOL_SET_LANGUAGE (sym, language_c, &objfile->objfile_obstack);
>> +      SYMBOL_SET_NAMES (sym, xstrdup (name), strlen (name), 0, 
objfile);
>
> Looking at the code of ctf_type_aname_raw, it seems to return an 
allocated copy,
> which needs to be freed eventually.

Yes, I've made the change in several places where the allocated copy is 
freed after
it's being dup'ed via obstack_strdup.

> Also, instead of calling xstrdup, you can just pass true to the 
COPY_NAME parameter.

Good point, done.

>> +/* Given a TID of kind CTF_K_INTEGER or CTF_K_FLOAT, find a 
representation
>> +   and create the symbol for it.  */
>> +
>> +static struct type *
>> +read_base_type (ctf_context_t *ccp, ctf_id_t tid)
>> +{
>> +  struct objfile *of = ccp->of;
>> +  ctf_file_t *fp = ccp->fp;
>> +  ctf_encoding_t cet;
>> +  struct type *type = NULL;
>> +  const char *name;
>> +  uint32_t kind;
>> +
>> +  if (ctf_type_encoding (fp, tid, &cet))
>> +    {
>> +      complaint (_("ctf_type_encoding read_base_type failed - %s"),
>> +                 ctf_errmsg (ctf_errno (fp)));
>> +      return NULL;
>> +    }
>> +
>> +  name = ctf_type_aname_raw (fp, tid);
>> +  if (!name || (name && !strlen (name)))
>
> I think that can be written more simply (and in GNU style) as
>
>   if (name == NULL || strlen (name) == 0)

OK, updated in several places.

>
>> +/* Start a structure or union scope (definition) with TID and TP to 
create
>> +   a type for the structure or union.
>
> TP seems to refer to something that doesn't exist (anymore?).  This 
occurs a few times
> in the file.

All the references to TP have been cleaned up.

>
>> +
>> +   Fill in the type's name and general properties. The members will 
not be
>> +   processed, nor a symbol table entry be done until 
process_structure_type
>> +   (assuming the type has a name).  */
>> +
>> +static struct type *
>> +read_structure_type (ctf_context_t *ccp, ctf_id_t tid)
>> +{
>> +  struct objfile *of = ccp->of;
>> +  ctf_file_t *fp = ccp->fp;
>> +  struct type *type;
>> +  const char *name;
>> +  uint32_t kind;
>
> I'd suggest adding a gdb_assert to make sure that tid represents a 
struct or union
> (and actually I'd suggest doing the same for all read_foo functions, 
make sure the
> kind of the TID is what we expect).

Actually callers of these read_foo functions will call the appropriate 
function, based
on the tid's kind. For example, either ctf_add_type_cb or 
process_structure_type
calls read_structure_type only if tid's kind is CTF_K_STRUCT or 
CTF_K_STRUCT.

>
>> +  type = alloc_type (of);
>> +  name = ctf_type_aname_raw (fp, tid);
>
> ctf_type_aname_raw returns an allocated string.  Is it ever free'd?
>
> This applies to other uses of ctf_type_aname_raw.

Done, please see above.

>
>> +....
>> +/* Given a tid of CTF_K_STRUCT or CTF_K_UNION, process all its members
>> +   and create the symbol for it.  */
>> +
>> +static void
>> +process_struct_members (ctf_context_t *ccp,
>> +                        ctf_id_t tid,
>> +                        struct type *type)
>> +{
>> +  ctf_file_t *fp = ccp->fp;
>> +  struct field_info fi;> +
>> +  fi.cur_context.fp = fp;
>> +  fi.cur_context.of = ccp->of;
>> +  fi.cur_context.builder = ccp->builder;
>
> I might be missing something, but I find it odd to copy the whole 
ctf_context_t structure,
> couldn't you just make field_info::cur_context a pointer, and just 
assign CCP to it?

Yes, it is better and is done.

>
>> +/* Read TID of kind CTF_K_VOLATILE with base type BTID. */
>> +
>> +static struct type *
>> +read_volatile_type (ctf_context_t *ccp, ctf_id_t tid, ctf_id_t btid)
>> +{
>> +  struct objfile *objfile = ccp->of;
>> +  ctf_file_t *fp = ccp->fp;
>> +  struct type *base_type, *cv_type;
>> +
>> +  base_type = get_tid_type (objfile, btid);
>> +  if (!base_type)
>> +    {
>
> In read_const_type and read_restrict_type, you call read_type_record 
to read
> in the type if it hasn't been read yet.  Is there a reason you don't 
do it here?

No, I need to do the same, i.e. calling read_type_record, in 
read_volatile_type.

>
>> +/* Get text segment base for OBJFILE, TSIZE contains the segment 
size.  */
>> +
>> +static CORE_ADDR
>> +get_of_text_range (struct objfile *of, int *tsize)
>
> I'd suggest renaming this to get_objfile_text_range.  A few 
characters more,
> but a lot clearer IMO.

OK.

>
>> +/* Read in full symbols for PST, and anything it depends on.  */
>> +
>> +static void
>> +psymtab_to_symtab (struct partial_symtab *pst)
>> +{
>> +  struct symbol *sym;
>> +  ctf_context_t *ccp;
>> +
>> +  if (pst->readin == 1)
>> +    return;
>
> This should never happen (it's checked in ctf_read_symtab), so I 
would use:
>
>   gdb_assert (!pst->readin);

It makes sense, done.

>
>> +static struct partial_symtab *
>> +create_partial_symtab (const char *name,
>> +                       ctf_file_t *cfp,
>> +                       struct objfile *objfile)
>> +{
>> +  struct partial_symtab *pst;
>> +  static ctf_context_t ccx;
>> +
>> +  pst = start_psymtab_common (objfile, name, 0);
>> +
>> +  ccx.fp = cfp;
>> +  ccx.of = objfile;
>> +  pst->read_symtab_private = (void *)&ccx;
>
> Hmm that looks fishy.  It looks like this is taking the address of a
> local variable for something that will be used after the current function
> will have returned.

Sorry, but it's a "static" local variable.

>
>> +...
>> +  /* Scan CTF object and function sections which correspond to each
>> +     STT_FUNC or STT_OBJECT entry in the symbol table,
>> +     pick up what init_symtab has done.  */
>> +  for (unsigned long idx = 0; ; idx++)
>> +    {
>> +      ctf_id_t tid;
>> +      if ((tid = ctf_lookup_by_symbol (cfp, idx)) == CTF_ERR)
>> +        {
>> +    if (ctf_errno (cfp) == EINVAL
>> +           || ctf_errno (cfp) == ECTF_NOSYMTAB)
>> +        // case ECTF_SYMRANGE:
>> +      break;
>
> Is this comment (the "case ECTF_SYMRANGE:" comment) a leftover?

Yes, it is and is taken out.

>
> Also, if these cases happen, it means the debug information is not 
valid/corrupted?
> Should there be an error printed to the user, should we maybe call 
error()?

We use either EINVAL or ECTF_NOSYMTAB to signal end of the section has
been reached. I have added such a comment in the code.

Weimin

Attachment: 0001-updated.patch
Description: Text document