Re: [PATCH v2] gdb: CTF support

[Simon Marchi <simark@simark.ca>]

On 2019-09-30 8:36 p.m., Weimin Pan wrote:
> 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).

Great thanks!

I noticed at least one case of leading spaces that should be tab in the attached
patch, so maybe do:

  grep '^        ' ctfread.c

to catch them.

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

Ok thanks.  Actually, can you please use gdb::unique_xmalloc_pointer to
manage that memory?  We're trying to minimize the manual memory management
in GDB.  You'd do:

  gdb::unique_xmalloc_ptr<char> name (ctf_type_aname_raw (fp, tid));

and use `name.get ()` instead of name.  It will get free'd automatically
when exiting scope.

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

Ok, I was hoping it would make the code more robust in case someone does
some refactor in the future and one of these functions get called with an
object of the wrong type, but maybe it would be overkill.

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

Oh ok, I did miss that.

I think we should avoid this.  What happens, for example, in this case:

1. Read psymtab for objfile 1
2. Read psymtab for objfile 2
3. Expand psymtab to symtab for objfile 1

From what I understand, step #2 will overwrite the static variable, such that
it won't be valid anymore in step #3.  I think it would be better to allocate the
context structure dynamically and store it as the objfile, like dwarf2read does.

Thanks,

Simon