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

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "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'm not too worried about addrmap crashing; I'm just adding things in a
slightly different order so if there is some weird overlap, addrmap will
just build a different address to block mapping, but this shouldn't
cause crashes from addrmap.  And ideally shouldn't be crashing the rest
of GDB either, we'll just end up with a different block (possibly, and
again, this is only in the case of some weird overlap).

At least, my experience while working on this was less, oops I've built
the wrong addrmap and now GDB crashed, and more, I've built the wrong
addrmap, and now GDB doesn't know where it is stopped, or the local
variables are no longer visible.

Which brings us to your second worry.  This patch, for sure changes the
order in which blocks are added into the addrmap.  More nested blocks
are added before their parents, so if we don't have overlapping
siblings, then this should result in the same mapping.  But I cannot
guarantee that there's not some weird compiler out there that does emit
such cases.  But we don't have tests for such cases yet, and I saw no
comments discussing such things, so I'm hoping that's not a thing.

If we do have overlapping siblings then I don't think current GDB will
work; at least, locals from one of the siblings will not be visible in
the overlap region.  This patch might change which sibling though.

When I originally wrote this patch I did try to retain the original
order in which blocks are added to the addrmap, and I looked at this
again today.  The problem is the particular ordering we use when
building the pending block list (see record_pending_block), as well as
pending block sorting (see end_compunit_symtab_get_static_block).  The
conclusion I've come to is that if the original block order is critical,
or if we just don't want to move away from this ordering, then we'd need
to add another data structure to track the desired order.  I didn't do
this for two reasons (1) saving space and time, and (2) so long as the
bottom up invariant remained, I figured the other changes should be
harmless.

You did give an approve for the original patch; I still need to update
the comments as you suggested, but is there anything else you'd like me
to investigate to help address your concerns?  Otherwise my hope would
be to merge this and address any issues if/when they arise down the
line.

Thanks,
Andrew