Re: [RFA 2/6] Allocate abbrev_table with new

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 2018-01-07 12:28 AM, Simon Marchi wrote:
> On 2018-01-05 07:26 PM, Tom Tromey wrote:
>> This changes dwarf2read.c to allocate abbrev tables using "new", and
>> then updates the users.
>>
>> This is somewhat complicated because ownership rules for abbrev tables
>> are obscure and involve a more than usual amount of cleanup
>> manipulation.
> 
> Hi Tom,
> 
> After staring at this code for longer than I had planned, I came to the
> conclusion that it would be much simpler if dwarf2_cu did not contain a
> pointer to the abbrev_table.  In fact, I think that dwarf2_cu::abbrev_table
> is actually a disguised function parameter that should be passed alongside
> the cu, not in it.
> 
> This is made clear by the fact that we are setting cu->abbrev_table just
> before calling the DIE-reading function, and reset it just after (with a
> cleanup).  The dwarf2_cu::abbrev_table is only useful for the duration of
> a function call, and then reset.
> 
> The dwarf2_cu structure never actually owns the abbrev_table, it's always
> some frame in the call stack that does.  But they own it indirectly via
> dwarf2_cu::abbrev_table and some cleanup that will free the table and
> reset that field (dwarf2_free_abbrev_table, or auto_free_abbrev_table with
> your patch).
> 
> The parameters for the DIE-reading operations are passed through the
> die_reader_specs structure, so I think it would make sense to put the
> reference to the abbrev_table there (this structure really exists to
> avoid having super long parameter lists).  This way, each frame that
> reads in an abbrev_table owns it through a unique_ptr and passes it to
> DIE-reading functions through parameter (through die_reader_specs).  That
> seems more straightforward.
> 
> Here's a patch that applies over this one, that illustrates the idea.  Let
> me know what you think.
> 
> Simon

Hi Tom,

I just pushed some dwarf2read cleanup patches I had posted a while ago, that
I wanted to push after the 8.1 branch creation.  It impacts this series a little
bit, so I thought I would help you rebase it.  Here's your original series rebased
on master:

https://github.com/simark/binutils-gdb/tree/tromey/dwarf

and here's your series plus my suggestion, rebased on master:

https://github.com/simark/binutils-gdb/tree/tromey/dwarf-plus-suggestion

Simon