Re: [PATCH v2] gdb: CTF support

[Wei-min Pan <weimin.pan@oracle.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