Re: [PATCH 5/7] gdb: update blockvector::lookup to handle non-contiguous blocks

["Jan Vraný" <Jan.Vrany@labware.com>]

On Thu, 2026-02-19 at 13:20 -0700, Tom Tromey wrote:
> > > > > > "Jan" == Jan Vrany <jan.vrany@labware.com> writes:
> 
> Jan> This commit updates blockvector::lookup to handle non-contiguous without
> Jan> help of addrmap. It introduces a new method, block::contains(CORE_ADDR),
> Jan> to check whether given block contains given address. This new method is
> Jan> then used in blockvector::lookup instead of simply using block's
> Jan> start and end addresses.
> 
> Jan> A unit test for non-contiguous blocks will come later in this series.
> 
> Jan> On Debian x86_64 I see no regressions except
> 
> Jan>     FAIL: gdb.dwarf2/debug-names.exp: print _start
> 
> Jan> This is caused by discrepancy between the debug info and the debug names
> Jan> and will be eventually fixed in a separate patch.
> 
> I glossed over this before but is this patch in this series?

No, this is the patch "Fix debug_names function visibility" from your 
branch you said you'll check in [1]. I can include it if you prefer.

[1]: https://sourceware.org/bugzilla/show_bug.cgi?id=33829#c12
> 
> Jan> +bool
> Jan> +block::contains (const CORE_ADDR addr) const
> Jan> +{
> Jan> +  if (addr >= start () && addr < end ())
> Jan> +    {
> Jan> +      if (is_contiguous ())
> Jan> +	return true;
> Jan> +
> Jan> +      for (auto range : ranges ())
> Jan> +	{
> Jan> +	  if (range.start () <= addr && addr < range.end ())
> Jan> +	    return true;
> 
> It took me a long time to understand that this patch was doing the real
> work in this series...
> 
> For this method I guess there is an invariant that the block's start is
> the minimum of the range minima and the block's end is the maximum of
> the range maxima.
> 
> It seems to me that since this is an important feature, it ought to be
> enforced by an assert, if necessary adding checks in the reader to
> ensure that bad DWARF doesn't cause an assertion failure.

Yes. I originally had these assertions there, thinking that blocks's start
is first range's start and end is last range's end but it turned out there's
no defined ordering (and DWARF5 spec does not say IIUC).

So I'd have to iterate over ranges to find min/max to use in asserts. I did not
do it in the end, but doing it in block::set_ranges probably won't hurt much. 
I'll do that in v2.

Other option I was thinking was to change make_blockranges to always sort then,
then the assertions in set_ranges would be trivial and in block::contains one
can binary-search block ranges too (though for up to ~4 ranges perhaps not worth).


> 
> Jan> @@ -857,7 +876,10 @@ blockvector::lookup (CORE_ADDR addr) const
> Jan>        if (b->start () > addr)
> Jan>  	return nullptr;
> Jan>        if (b->end () > addr)
> Jan> -	return b;
> Jan> +	{
> Jan> +	  if (b->contains (addr))
> Jan> +	    return b;
> Jan> +	}
> Jan>        bot--;
> Jan>      }
>  
> The heart of the series.
> 
> I wonder about the efficiency of this.  Like how many blocks can we
> expect to see in the linear phase?
> 
> I don't know what a representative program might be here.

TBH I have no idea. How common is this, how many ranges blocks have, nor
how to force compiler to produce such binary. Maybe torturing the compiler
with -O3 and PGO?

Jan

> 
> Tom