Re: [PATCH 2/2] gdb: use std::vector<> to hold on blocks in struct blockvector

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

On Tue, 2025-10-14 at 15:41 -0400, Simon Marchi wrote:
> On 10/14/25 11:00 AM, Andrew Burgess wrote:
> > > @@ -809,6 +810,41 @@ make_blockranges (struct objfile *objfile,
> > >    return blr;
> > >  }
> > >  
> > > +static bool
> > > +block_ordering_predicate (struct block *b1, struct block *b2)
> > 
> > Functions should have a comment above them.
> > 
> > > +{
> > > +  CORE_ADDR start1 = b1->start ();
> > > +  CORE_ADDR start2 = b2->start ();
> > > +
> > > +  if (start1 != start2)
> > > +    return start1 < start2;
> > > +  return (b2->end () < b1->end ());
> > > +}
> > 
> > I notice that we use a slightly different ordering function in
> > buildsym.c, one that doesn't consider the end address.  Probably doesn't
> > matter, but maybe it's at least worth mentioning in a comment or the
> > commit message, that way, if someone is looking at the code in the
> > future they can understand why these things are different.  Even if the
> > reason is just "we have no motivating example for why these should be
> > the same right now".
> 
> buildsym sorts them (locally, in its singly-linked list) in the inverse
> order, but then fills the blockvector backwards.  So the blocks end up
> in the right order in the blockvector.  I think this is an artefact from
> the past and could be simplified.  The predicate for sorting blocks in a
> blockvector should probably be written just once (in block.h) and used
> by whoever needs it.

I think what Andrew meant is that mdebugread.c (and jit.c) sort blocks
so that blocks with lower start PC appear before blocks with higher start PC
AND if both start at the same PC, enclosing (larger) block appears before
nested (smaller) block. 

Whereas buildsym.c sorts them only by start PC. In buildsym.c there's also
explicitly said that blocks with the same start PC should not be reordered:

      /* Sort blocks by start address in descending order.  Blocks with the
	 same start address must remain in the original order to preserve
	 inline function caller/callee relationships.  */

I was wondering if there's a case where blocks in blockvector created by
buildsym.c would violate mdebugread.c-style ordering (that takes start and
end PC into an account). So I modified blockvector::set_block() as follows:

void
blockvector::set_block (int i, struct block *block)
{
  if (i == GLOBAL_BLOCK)
    gdb_assert (block->is_global_block ());
  else if (i == STATIC_BLOCK)
    gdb_assert (block->is_static_block ());
  else
    {
      if ((i - 1) >= FIRST_LOCAL_BLOCK && m_blocks[i - 1] != nullptr)
	gdb_assert (block_ordering_predicate (m_blocks[i - 1], block)
		    || m_blocks[i - 1]->end () == block->end ());
      if ((i + 1) < m_blocks.size () && m_blocks[i + 1] != nullptr)
	gdb_assert (block_ordering_predicate (block, m_blocks[i + 1])
		    || block->end () == m_blocks[i + 1]->end ());
    }
  m_blocks[i] = block;
}

Running db.base/*.exp and gdb.dwarf2/*.exp shown no regression.

I also tried to change buildsym.c to use mdebugread.c-style ordering:

         same start address must remain in the original order to preserve
         inline function caller/callee relationships.  */
       std::stable_sort (barray.begin (), barray.end (),
-                       [] (const block *a, const block *b)
-                       {
-                         return a->start () > b->start ();
-                       });
+                       blockvector::block_ordering_predicate);
 
       int i = 0;

Again no regression. Not that this means it is safe to use the
same ordering predicate in all three cases.

Jan

> 
> Simon