Re: [PATCHv3 5/7] gdb: create address map after parsing all DIE

[Tom Tromey <tom@tromey.com>]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> Continuing the work done in the last two commits, this commit defers
Andrew> building the addrmap for a blockvector until after all the DIE have
Andrew> been read, and the line table processed.

Andrew> The benefit of this is that any changes to a block's ranges done
Andrew> during line table processing (see the next commit) will be reflected
Andrew> in the blockvector's addrmap.

Andrew> The alternative to this is to build the addrmap as we initially see
Andrew> each block, but then adjust the addrmap if we later decide to modify a
Andrew> block.  I think defering the addrmap creation is cleaner, and is less
Andrew> work overall.

Andrew> The addrmap requires that we add the most inner blocks first.  I
Andrew> achieve this by walking the blockvector backward, as we always add
Andrew> parent blocks before their more inner child blocks.

I wonder if this is guaranteed to be correct.  Like, does gdb cope
properly if a compiler happens to emit multiple lexical scopes that each
have non-contiguous ranges, and then where the ranges happen to overlap.

However I think your patch probably does not make gdb worse in this
regard.

Andrew> @@ -410,8 +410,6 @@ buildsym_compunit::record_block_range (struct block *block,
Andrew>    if (start != block->start ()
Andrew>        || end_inclusive + 1 != block->end ())
Andrew>      m_pending_addrmap_interesting = true;
Andrew> -
Andrew> -  m_pending_addrmap.set_empty (start, end_inclusive, block);
Andrew>  }

I think the comment before this method and the comment in the method
both need to be updated, as this doesn't actually record the range any
more.

I wonder why buildsym even tries to see if the pending addrmap might be
interesting.  It seems to me that a fixed addrmap is basically the same
data structure as the blockvector: it maps addresses to blocks and uses
a binary search to find the correct entry.

So I'm wondering if, in the longer term -- you definitely don't need to
to this here -- if we should just switch to a single representation.

Furthermore I was wondering if it makes sense for blocks to even track
their ranges.  That is, for a block with multiple ranges, could we just
have multiple entries in the blockvector and get rid of block::m_ranges?

FWIW I've been looking into this area a bit while experimenting with
lazy CU expansion.  There I think I want to make it so blockvectors are
always expandable, so no fixed addrmap at least -- and mutable addrmaps
have some issues with updating, so probably moving to just having a
vector.

Anyway this seems fine with the comment cleanup.
Approved-By: Tom Tromey <tom@tromey.com>

Tom