Re: [PATCH v2] gdb: CTF support

[Wei-min Pan <weimin dot pan at oracle dot com>]

On 10/1/2019 7:47 PM, Simon Marchi wrote:
> 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.

Hi Simon,

Thanks, I think it's now clean after fixing the one below:

1247c1247
<           sym = add_stt_func (ccp, i);
---
>         sym = add_stt_func (ccp, i);

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

It doesn't seem using gdb::unique_xmalloc_pointer is appropriate in 
these cases
because we want to keep these symbol/type names around in the symbol table.

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

Yes, you're right. I've made the following changes:

1322c1322
<   static ctf_context_t ccx;
---
>   ctf_context_t *ccx;
1326,1328c1326,1329
<   ccx.fp = cfp;
<   ccx.of = objfile;
<   pst->read_symtab_private = (void *) &ccx;
---
>   ccx = XOBNEW (&objfile->objfile_obstack, ctf_context_t);
>   ccx->fp = cfp;
>   ccx->of = objfile;
>   pst->read_symtab_private = (void *) ccx;

Many thanks,
Weimin