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

[...]
Andrew> Now the two lexical blocks are siblings, but if we make the second block
Andrew> overlap the first then this patch does cause problems.
[...]
Andrew> This DOES change GDB's behaviour.

Andrew> Is this sibling case the type of case you were thinking of?  Or have I
Andrew> got the wrong end of the stick here?

I think your analysis is in line with what I was thinking.

My main concern is whether incorrect input can make gdb crash by
violating some addrmap invariant; and then also if sufficiently weird
but nominally valid input can make gdb do the wrong thing.

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

Andrew> Sure, but I wonder if this is a memory use thing?  The addrmap must use
Andrew> more memory than the vector, so maybe for large programs this would be a
Andrew> noticable hit.

Yeah, I think what I meant is gdb could perhaps just dispense with the
vector entirely and only store the addrmap.

Though I don't actually want that outcome because I would like the
blockvector to be expandable.  This isn't readily possible with addrmap
due to the bottom-up requirement.

Andrew> There are a few places where we use the block's ranges for useful work,
Andrew> these would all need to be rewritten to get the same information from
Andrew> the blockvector (really the addrmap within the blockvector).

Ok, thanks.  When this comes back around I will dig for those.

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

Andrew> Wouldn't that cause problems for non-contiguous blocks?  Isn't that the
Andrew> problem that the addrmap was added to solve?

For me the desirable end point is that blockvector is an opaque data
structure, where the primary read operation is "give the block for a
given PC", and queries about the internal structure of the blockvector
are forbidden.  Right now there is code that looks to see if there a map
exists, which is an abstraction violation; and also buildsym seems to be
pretty chummy with blockvector, though that's less of a problem.

Now, the other thread -- the JIT stuff Jan Vrany is working on -- shows
that there are still some problems to iron out here, because for reasons
I don't really understand, some blockvector users seem to want to get
back the static block in response to a PC lookup.  (There was that issue
with some code in libc that is not in a function body.  But I don't
understand why that's not just a bug in libc.)

Other than that, though, I don't think that non-contiguous blocks
present a problem.  Their existence would be hidden by the blockvector
API.

Though I may not appreciate this fully either.  Anyway my current idea
for lazy expansion is to record incomplete symbols for functions, so
that if a blockvector lookup finds a block corresponding to such a
function, it would first be expanded to find the correct enclosed block.

Tom