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] |
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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |