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

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

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

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

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

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

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

> +/* 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)

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

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

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

> +  if (name && strlen (name))
> +    TYPE_NAME (type) = name;
> +  kind = ctf_type_kind (fp, tid);
> +  if (kind == CTF_K_UNION)
> +    {
> +      TYPE_CODE (type) = TYPE_CODE_UNION;
> +    }
> +  else
> +    {
> +      TYPE_CODE (type) = TYPE_CODE_STRUCT;
> +    }
> +  TYPE_LENGTH (type) = ctf_type_size (fp, tid);
> +  set_type_align (type, ctf_type_align (fp, tid));
> +
> +  return set_tid_type (ccp->of, tid, type);
> +}
> +
> +/* 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?

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

> +/* Add an ELF STT_FUNC symbol with index IDX to the symbol table.  */
> +
> +struct symbol *
> +add_stt_func (ctf_context_t *ccp, unsigned long idx)

static

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

> +/* 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);

> +/* Allocate a new partial_symtab NAME.  */
> +/* Each source file that has not been fully read in is represented by
> +   a partial_symtab.  This contains the information on where in the
> +   executable the debugging symbols for a specific file are, and a
> +   list of names of global symbols which are located in this file.
> +   They are all chained on partial symtab lists.
> +
> +   Even after the source file has been read into a symtab, the
> +   partial_symtab remains around.  They are allocated on an obstack,
> +   objfile_obstack.  */
> +
> +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.

> +/* Setup partial_symtab's describing each source file for which
> +   debugging information is available.  */
> +
> +static void
> +scan_partial_symbols (ctf_file_t *cfp, struct objfile *of)
> +{
> +  ctf_context_t ccx;
> +  bfd *abfd = of->obfd;
> +  char *name = bfd_get_filename (abfd);
> +  struct partial_symtab *pst = create_partial_symtab (name, cfp, of);
> +
> +  ccx.fp = cfp;
> +  ccx.of = of;
> +  if (ctf_type_iter (cfp, ctf_psymtab_type_cb, &ccx) == CTF_ERR)
> +    {
> +      complaint (_("ctf_type_iter scan_partial_symbols failed - %s"),
> +                 ctf_errmsg (ctf_errno (cfp)));
> +    }
> +
> +  if (ctf_variable_iter (cfp, ctf_psymtab_var_cb, &ccx) == CTF_ERR)
> +    {
> +      complaint (_("ctf_variable_iter scan_partial_symbols failed - %s"),
> +                 ctf_errmsg (ctf_errno (cfp)));
> +    }
> +
> +  /* 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?

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()?

Simon