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