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

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

[Jan Vrany]

These two patches change make blockvector to be heap-allocated and
use std::vector to hold on blocks (rather than flexible array).

This is useful for lazy CU expansion and for Python "JIT" API as it
allows for adding more blocks dynamically.

Thanks,
Jan

--
Jan Vrany (2):
  gdb: allocate blockvector on heap
  gdb: use std::vector<> to hold on blocks in struct blockvector

 gdb/block.c      | 36 ++++++++++++++++++++++++++++++++++++
 gdb/block.h      | 47 +++++++++++++++++++++++++++++++----------------
 gdb/buildsym.c   |  5 +----
 gdb/jit.c        |  8 +-------
 gdb/mdebugread.c | 46 +++-------------------------------------------
 5 files changed, 72 insertions(+), 70 deletions(-)

-- 
2.51.0

[PATCH 1/2] gdb: allocate blockvector on heap

[Jan Vrany]

This patch changes blockvector to be allocated on the heap, using
'new'.

The code is little awkward since blockvector uses flexible array member
to hold on blocks. So in order to make it working, this commit overloads
'new' and 'delete' operators to take the number of blocks as parameter
and allocates/deallocates memory using std::malloc and std::free.
This will go away in following commit that will change blockvector to
use std::vector rather than flexible array member.
---
 gdb/block.h      | 18 ++++++++++++++++++
 gdb/buildsym.c   |  5 +----
 gdb/jit.c        |  8 +-------
 gdb/mdebugread.c | 38 +++++++++++---------------------------
 4 files changed, 31 insertions(+), 38 deletions(-)

diff --git a/gdb/block.h b/gdb/block.h
index 8a3ea822d04..d1ca41d7482 100644
--- a/gdb/block.h
+++ b/gdb/block.h
@@ -420,6 +420,24 @@ struct global_block : public block
 
 struct blockvector
 {
+  void *operator new (size_t size, size_t num_blocks)
+  {
+    size_t size_in_bytes = size + (num_blocks - 1) * (sizeof (struct block *));
+    return std::malloc (size_in_bytes);
+  }
+
+  void operator delete (void *memory)
+  {
+    std::free (memory);
+  }
+
+  blockvector (int nblocks)
+    : m_map (nullptr),
+      m_num_blocks (nblocks)
+  {
+    std::memset (m_blocks, 0, nblocks * sizeof (struct block *));
+  }
+
   /* Return a view on the blocks of this blockvector.  */
   gdb::array_view<struct block *> blocks ()
   {
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 2a8e95e078b..a61ef68ada1 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -427,10 +427,7 @@ buildsym_compunit::make_blockvector ()
     {
     }
 
-  blockvector = (struct blockvector *)
-    obstack_alloc (&m_objfile->objfile_obstack,
-		   (sizeof (struct blockvector)
-		    + (i - 1) * sizeof (struct block *)));
+  blockvector = new (i) struct blockvector (i);
 
   /* Copy the blocks into the blockvector.  This is done in reverse
      order, which happens to put the blocks into the proper order
diff --git a/gdb/jit.c b/gdb/jit.c
index ca817e85c71..d95fe542a29 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -517,7 +517,6 @@ jit_symtab_close_impl (struct gdb_symbol_callbacks *cb,
 static void
 finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
 {
-  size_t blockvector_size;
   CORE_ADDR begin, end;
   struct blockvector *bv;
 
@@ -554,18 +553,13 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
       filetab->set_linetable (new_table);
     }
 
-  blockvector_size = (sizeof (struct blockvector)
-		      + (actual_nblocks - 1) * sizeof (struct block *));
-  bv = (struct blockvector *) obstack_alloc (&objfile->objfile_obstack,
-					     blockvector_size);
+  bv = new (actual_nblocks) blockvector (actual_nblocks);
   cust->set_blockvector (bv);
 
   /* At the end of this function, (begin, end) will contain the PC range this
      entire blockvector spans.  */
-  bv->set_map (nullptr);
   begin = stab->blocks.front ().begin;
   end = stab->blocks.front ().end;
-  bv->set_num_blocks (actual_nblocks);
 
   /* First run over all the gdb_block objects, creating a real block
      object for each.  Simultaneously, keep setting the real_block
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 9504e5e1670..3cedf0e4fc2 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -242,8 +242,6 @@ static struct compunit_symtab *new_symtab (const char *, int, struct objfile *);
 
 static struct linetable *new_linetable (int);
 
-static struct blockvector *new_bvect (int);
-
 static struct type *parse_type (int, union aux_ext *, unsigned int, int *,
 				int, const char *);
 
@@ -4501,18 +4499,19 @@ add_block (struct block *b, struct symtab *s)
 {
   /* Cast away "const", but that's ok because we're building the
      symtab and blockvector here.  */
-  struct blockvector *bv
+  struct blockvector *old_bv
     = (struct blockvector *) s->compunit ()->blockvector ();
 
-  bv = (struct blockvector *) xrealloc ((void *) bv,
-					(sizeof (struct blockvector)
-					 + bv->num_blocks ()
-					 * sizeof (struct block)));
-  if (bv != s->compunit ()->blockvector ())
-    s->compunit ()->set_blockvector (bv);
+  struct blockvector *new_bv = new (old_bv->num_blocks () + 1)
+    blockvector (old_bv->num_blocks () + 1);
+
+  s->compunit ()->set_blockvector (old_bv);
 
-  bv->set_block (bv->num_blocks (), b);
-  bv->set_num_blocks (bv->num_blocks () + 1);
+  for (int i = 0; i < old_bv->num_blocks (); i++)
+    new_bv->set_block (i, old_bv->block (i));
+  new_bv->set_block (new_bv->num_blocks () - 1, b);
+
+  delete old_bv;
 }
 
 /* Add a new linenumber entry (LINENO,ADR) to a linevector LT.
@@ -4636,7 +4635,7 @@ new_symtab (const char *name, int maxlines, struct objfile *objfile)
   lang = cust->language ();
 
   /* All symtabs must have at least two blocks.  */
-  bv = new_bvect (2);
+  bv = new (2) blockvector (2);
   bv->set_block (GLOBAL_BLOCK, new_block (objfile, NON_FUNCTION_BLOCK, lang));
   bv->set_block (STATIC_BLOCK, new_block (objfile, NON_FUNCTION_BLOCK, lang));
   bv->static_block ()->set_superblock (bv->global_block ());
@@ -4704,21 +4703,6 @@ shrink_linetable (struct linetable *lt)
 					    * sizeof (lt->item))));
 }
 
-/* Allocate and zero a new blockvector of NBLOCKS blocks.  */
-
-static struct blockvector *
-new_bvect (int nblocks)
-{
-  struct blockvector *bv;
-  int size;
-
-  size = sizeof (struct blockvector) + nblocks * sizeof (struct block *);
-  bv = (struct blockvector *) xzalloc (size);
-  bv->set_num_blocks (nblocks);
-
-  return bv;
-}
-
 /* Allocate and zero a new block of language LANGUAGE, and set its
    BLOCK_MULTIDICT.  If function is non-zero, assume the block is
    associated to a function, and make sure that the symbols are stored
-- 
2.51.0

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

[Jan Vrany]

This commit changes internal implementation of struct blockvector to use
std::vector<> rather than flexible array, allowing one to easily add
blocks to existing blockvector.

This is needed to for lazy CU expansion and for Python API to build
objfiles, compunits and symtabs dynamically (similarly to JIT reader
API).

To avoid extensive reallocation when adding a lot of blocks, ctor takes
initial number of blocks to be stored in blockvector.

While at is, it also adds blockvector::add_block() to add new blocks
and changes mdebugread.c to use it.

The downside is higher memory consumption. The size of std::vector with
obstack allocator is 32 bytes (GCC 14) compared to 8 bytes used
currently to store the number of blocks (m_num_blocks). Stopping gdb at
its main(), followed by "maint expand-symtabs" results in 4593
compunit symtabs so in this case the overhead is 24*4593 = 110232 bytes
which I hope is acceptable.
---
 gdb/block.c      | 36 +++++++++++++++++++++++++++++
 gdb/block.h      | 59 +++++++++++++++++++++++-------------------------
 gdb/buildsym.c   |  2 +-
 gdb/jit.c        |  2 +-
 gdb/mdebugread.c | 30 +++---------------------
 5 files changed, 69 insertions(+), 60 deletions(-)

diff --git a/gdb/block.c b/gdb/block.c
index e09bccdcf86..a9191c3fdff 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -17,6 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+#include <cstring>
 #include "block.h"
 #include "symtab.h"
 #include "symfile.h"
@@ -809,6 +810,41 @@ make_blockranges (struct objfile *objfile,
   return blr;
 }
 
+static bool
+block_ordering_predicate (struct block *b1, struct block *b2)
+{
+  CORE_ADDR start1 = b1->start ();
+  CORE_ADDR start2 = b2->start ();
+
+  if (start1 != start2)
+    return start1 < start2;
+  return (b2->end () < b1->end ());
+}
+
+/* See block.h.  */
+
+void
+blockvector::add_block (struct block *block)
+{
+  if (num_blocks () <= FIRST_LOCAL_BLOCK)
+    {
+      /* No blocks (except global and static block).  */
+      m_blocks.push_back (block);
+    }
+  else
+    {
+      /* blockvector already contains some blocks.  Insert new block
+   to a correct place.  */
+      auto first = m_blocks.begin () + FIRST_LOCAL_BLOCK;
+      auto last = m_blocks.end ();
+
+      auto insert_before = std::upper_bound (first, last, block,
+					     block_ordering_predicate);
+
+      m_blocks.insert (insert_before, block);
+    }
+}
+
 /* Implement 'maint info blocks' command.  If passed an argument then
    print a list of all blocks at the given address.  With no arguments
    then list all blocks at the current address of the current inferior.  */
diff --git a/gdb/block.h b/gdb/block.h
index d1ca41d7482..c86650753a4 100644
--- a/gdb/block.h
+++ b/gdb/block.h
@@ -20,6 +20,7 @@
 #ifndef GDB_BLOCK_H
 #define GDB_BLOCK_H
 
+#include <algorithm>
 #include "dictionary.h"
 #include "gdbsupport/array-view.h"
 #include "gdbsupport/next-iterator.h"
@@ -420,59 +421,58 @@ struct global_block : public block
 
 struct blockvector
 {
-  void *operator new (size_t size, size_t num_blocks)
-  {
-    size_t size_in_bytes = size + (num_blocks - 1) * (sizeof (struct block *));
-    return std::malloc (size_in_bytes);
-  }
-
-  void operator delete (void *memory)
-  {
-    std::free (memory);
-  }
-
   blockvector (int nblocks)
     : m_map (nullptr),
-      m_num_blocks (nblocks)
-  {
-    std::memset (m_blocks, 0, nblocks * sizeof (struct block *));
-  }
+      m_blocks (nblocks, nullptr)
+  {}
 
   /* Return a view on the blocks of this blockvector.  */
   gdb::array_view<struct block *> blocks ()
   {
-    return gdb::array_view<struct block *> (m_blocks, m_num_blocks);
+    return gdb::array_view<struct block *> (m_blocks.data (),
+					    m_blocks.size ());
   }
 
   /* Const version of the above.  */
   gdb::array_view<const struct block *const> blocks () const
   {
-    const struct block **blocks = (const struct block **) m_blocks;
-    return gdb::array_view<const struct block *const> (blocks, m_num_blocks);
+    const struct block **blocks = (const struct block **) m_blocks.data ();
+    return gdb::array_view<const struct block *const> (blocks,
+						       m_blocks.size ());
   }
 
   /* Return the block at index I.  */
   struct block *block (size_t i)
-  { return this->blocks ()[i]; }
+  {
+    return m_blocks[i];
+  }
 
   /* Const version of the above.  */
   const struct block *block (size_t i) const
-  { return this->blocks ()[i]; }
+  {
+    return m_blocks[i];
+  }
 
   /* Set the block at index I.  */
   void set_block (int i, struct block *block)
   { m_blocks[i] = block; }
 
-  /* Set the number of blocks of this blockvector.
-
-     The storage of blocks is done using a flexible array member, so the number
-     of blocks set here must agree with what was effectively allocated.  */
-  void set_num_blocks (int num_blocks)
-  { m_num_blocks = num_blocks; }
+  /* Add BLOCK, making sure blocks are ordered by code-addresses
+     as required.  Update global and static block start and end
+     adresses accordingly.  */
+  void add_block (struct block *block);
 
   /* Return the number of blocks in this blockvector.  */
   int num_blocks () const
-  { return m_num_blocks; }
+  {
+    return m_blocks.size ();
+  }
+
+  /* Set the number of blocks of this blockvector.  */
+  void set_num_blocks (int num_blocks)
+  {
+    m_blocks.resize (num_blocks, nullptr);
+  }
 
   /* Return the global block of this blockvector.  */
   struct global_block *global_block ()
@@ -511,11 +511,8 @@ struct blockvector
      enough.  */
   addrmap_fixed *m_map;
 
-  /* Number of blocks in the list.  */
-  int m_num_blocks;
-
   /* The blocks themselves.  */
-  struct block *m_blocks[1];
+  std::vector<struct block *> m_blocks;
 };
 
 extern const struct blockvector *blockvector_for_pc (CORE_ADDR,
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index a61ef68ada1..8daa68d296b 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -427,7 +427,7 @@ buildsym_compunit::make_blockvector ()
     {
     }
 
-  blockvector = new (i) struct blockvector (i);
+  blockvector = new struct blockvector (i);
 
   /* Copy the blocks into the blockvector.  This is done in reverse
      order, which happens to put the blocks into the proper order
diff --git a/gdb/jit.c b/gdb/jit.c
index d95fe542a29..4e3abd1802c 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -553,7 +553,7 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
       filetab->set_linetable (new_table);
     }
 
-  bv = new (actual_nblocks) blockvector (actual_nblocks);
+  bv = new blockvector (actual_nblocks);
   cust->set_blockvector (bv);
 
   /* At the end of this function, (begin, end) will contain the PC range this
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 3cedf0e4fc2..90bbcd81ed2 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -256,8 +256,6 @@ static legacy_psymtab *new_psymtab (const char *, psymtab_storage *,
 static void mdebug_expand_psymtab (legacy_psymtab *pst,
 				  struct objfile *objfile);
 
-static void add_block (struct block *, struct symtab *);
-
 static void add_symbol (struct symbol *, struct symtab *, struct block *);
 
 static int add_line (struct linetable *, int, CORE_ADDR, int);
@@ -808,7 +806,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
       b->set_start (sh->value);
       b->set_end (sh->value);
       b->set_superblock (top_stack->cur_block);
-      add_block (b, top_stack->cur_st);
+      top_stack->cur_st->compunit ()->blockvector ()->add_block (b);
 
       /* Not if we only have partial info.  */
       if (SC_IS_UNDEF (sh->sc) || sh->sc == scNil)
@@ -1136,7 +1134,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
       b->set_start (sh->value + top_stack->procadr);
       b->set_superblock (top_stack->cur_block);
       top_stack->cur_block = b;
-      add_block (b, top_stack->cur_st);
+      top_stack->cur_st->compunit ()->blockvector ()->add_block (b);
       break;
 
     case stEnd:		/* end (of anything) */
@@ -4492,28 +4490,6 @@ add_symbol (struct symbol *s, struct symtab *symtab, struct block *b)
   mdict_add_symbol (b->multidict (), s);
 }
 
-/* Add a new block B to a symtab S.  */
-
-static void
-add_block (struct block *b, struct symtab *s)
-{
-  /* Cast away "const", but that's ok because we're building the
-     symtab and blockvector here.  */
-  struct blockvector *old_bv
-    = (struct blockvector *) s->compunit ()->blockvector ();
-
-  struct blockvector *new_bv = new (old_bv->num_blocks () + 1)
-    blockvector (old_bv->num_blocks () + 1);
-
-  s->compunit ()->set_blockvector (old_bv);
-
-  for (int i = 0; i < old_bv->num_blocks (); i++)
-    new_bv->set_block (i, old_bv->block (i));
-  new_bv->set_block (new_bv->num_blocks () - 1, b);
-
-  delete old_bv;
-}
-
 /* Add a new linenumber entry (LINENO,ADR) to a linevector LT.
    MIPS' linenumber encoding might need more than one byte
    to describe it, LAST is used to detect these continuation lines.
@@ -4635,7 +4611,7 @@ new_symtab (const char *name, int maxlines, struct objfile *objfile)
   lang = cust->language ();
 
   /* All symtabs must have at least two blocks.  */
-  bv = new (2) blockvector (2);
+  bv = new blockvector (2);
   bv->set_block (GLOBAL_BLOCK, new_block (objfile, NON_FUNCTION_BLOCK, lang));
   bv->set_block (STATIC_BLOCK, new_block (objfile, NON_FUNCTION_BLOCK, lang));
   bv->static_block ()->set_superblock (bv->global_block ());
-- 
2.51.0

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

[Andrew Burgess]

Hi Jan,

Thanks for working on this.

I think that this commit should be merged with the previous one.  For me
at least, I'm unable to build the previous commit as I get compiler
errors about malloc/free being poisoned.  Not sure why this didn't
impact you, but I didn't look too closely once I realised patch #2
removes those calls.  Given the overlap of the patches, I think the
easiest solution would be to just merge the two.

Jan Vrany <jan.vrany@labware.com> writes:

> This commit changes internal implementation of struct blockvector to use
> std::vector<> rather than flexible array, allowing one to easily add
> blocks to existing blockvector.
>
> This is needed to for lazy CU expansion and for Python API to build
> objfiles, compunits and symtabs dynamically (similarly to JIT reader
> API).
>
> To avoid extensive reallocation when adding a lot of blocks, ctor takes
> initial number of blocks to be stored in blockvector.
>
> While at is, it also adds blockvector::add_block() to add new blocks
> and changes mdebugread.c to use it.
>
> The downside is higher memory consumption. The size of std::vector with
> obstack allocator is 32 bytes (GCC 14) compared to 8 bytes used
> currently to store the number of blocks (m_num_blocks). Stopping gdb at
> its main(), followed by "maint expand-symtabs" results in 4593
> compunit symtabs so in this case the overhead is 24*4593 = 110232 bytes
> which I hope is acceptable.
> ---
>  gdb/block.c      | 36 +++++++++++++++++++++++++++++
>  gdb/block.h      | 59 +++++++++++++++++++++++-------------------------
>  gdb/buildsym.c   |  2 +-
>  gdb/jit.c        |  2 +-
>  gdb/mdebugread.c | 30 +++---------------------
>  5 files changed, 69 insertions(+), 60 deletions(-)
>
> diff --git a/gdb/block.c b/gdb/block.c
> index e09bccdcf86..a9191c3fdff 100644
> --- a/gdb/block.c
> +++ b/gdb/block.c
> @@ -17,6 +17,7 @@
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
> +#include <cstring>

I couldn't see why this should be needed.

>  #include "block.h"
>  #include "symtab.h"
>  #include "symfile.h"
> @@ -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".

> +
> +/* See block.h.  */
> +
> +void
> +blockvector::add_block (struct block *block)
> +{
> +  if (num_blocks () <= FIRST_LOCAL_BLOCK)
> +    {
> +      /* No blocks (except global and static block).  */
> +      m_blocks.push_back (block);
> +    }
> +  else
> +    {
> +      /* blockvector already contains some blocks.  Insert new block
> +   to a correct place.  */

The indentation of the second line is wrong here.

> +      auto first = m_blocks.begin () + FIRST_LOCAL_BLOCK;
> +      auto last = m_blocks.end ();
> +
> +      auto insert_before = std::upper_bound (first, last, block,
> +					     block_ordering_predicate);
> +
> +      m_blocks.insert (insert_before, block);
> +    }
> +}
> +
>  /* Implement 'maint info blocks' command.  If passed an argument then
>     print a list of all blocks at the given address.  With no arguments
>     then list all blocks at the current address of the current inferior.  */
> diff --git a/gdb/block.h b/gdb/block.h
> index d1ca41d7482..c86650753a4 100644
> --- a/gdb/block.h
> +++ b/gdb/block.h
> @@ -20,6 +20,7 @@
>  #ifndef GDB_BLOCK_H
>  #define GDB_BLOCK_H
>  
> +#include <algorithm>
>  #include "dictionary.h"
>  #include "gdbsupport/array-view.h"
>  #include "gdbsupport/next-iterator.h"
> @@ -420,59 +421,58 @@ struct global_block : public block
>  
>  struct blockvector
>  {
> -  void *operator new (size_t size, size_t num_blocks)
> -  {
> -    size_t size_in_bytes = size + (num_blocks - 1) * (sizeof (struct block *));
> -    return std::malloc (size_in_bytes);
> -  }
> -
> -  void operator delete (void *memory)
> -  {
> -    std::free (memory);
> -  }
> -
>    blockvector (int nblocks)
>      : m_map (nullptr),
> -      m_num_blocks (nblocks)
> -  {
> -    std::memset (m_blocks, 0, nblocks * sizeof (struct block *));
> -  }
> +      m_blocks (nblocks, nullptr)
> +  {}
>  
>    /* Return a view on the blocks of this blockvector.  */
>    gdb::array_view<struct block *> blocks ()
>    {
> -    return gdb::array_view<struct block *> (m_blocks, m_num_blocks);
> +    return gdb::array_view<struct block *> (m_blocks.data (),
> +					    m_blocks.size ());
>    }
>  
>    /* Const version of the above.  */
>    gdb::array_view<const struct block *const> blocks () const
>    {
> -    const struct block **blocks = (const struct block **) m_blocks;
> -    return gdb::array_view<const struct block *const> (blocks, m_num_blocks);
> +    const struct block **blocks = (const struct block **) m_blocks.data ();
> +    return gdb::array_view<const struct block *const> (blocks,
> +						       m_blocks.size ());
>    }
>  
>    /* Return the block at index I.  */
>    struct block *block (size_t i)
> -  { return this->blocks ()[i]; }
> +  {
> +    return m_blocks[i];
> +  }
>  
>    /* Const version of the above.  */
>    const struct block *block (size_t i) const
> -  { return this->blocks ()[i]; }
> +  {
> +    return m_blocks[i];
> +  }
>  
>    /* Set the block at index I.  */
>    void set_block (int i, struct block *block)
>    { m_blocks[i] = block; }
>  
> -  /* Set the number of blocks of this blockvector.
> -
> -     The storage of blocks is done using a flexible array member, so the number
> -     of blocks set here must agree with what was effectively allocated.  */
> -  void set_num_blocks (int num_blocks)
> -  { m_num_blocks = num_blocks; }
> +  /* Add BLOCK, making sure blocks are ordered by code-addresses
> +     as required.  Update global and static block start and end
> +     adresses accordingly.  */
> +  void add_block (struct block *block);

Maybe this comment needs tweaking.  In the general case the global
and/or static blocks are not updated, but I can see why you would
consider them as updated if we started from an empty block list.  But I
think expanding this comment would make things clearer.

>  
>    /* Return the number of blocks in this blockvector.  */
>    int num_blocks () const
> -  { return m_num_blocks; }
> +  {
> +    return m_blocks.size ();
> +  }
> +
> +  /* Set the number of blocks of this blockvector.  */
> +  void set_num_blocks (int num_blocks)
> +  {
> +    m_blocks.resize (num_blocks, nullptr);
> +  }
>  
>    /* Return the global block of this blockvector.  */
>    struct global_block *global_block ()
> @@ -511,11 +511,8 @@ struct blockvector
>       enough.  */
>    addrmap_fixed *m_map;
>  
> -  /* Number of blocks in the list.  */
> -  int m_num_blocks;
> -
>    /* The blocks themselves.  */
> -  struct block *m_blocks[1];
> +  std::vector<struct block *> m_blocks;
>  };
>  
>  extern const struct blockvector *blockvector_for_pc (CORE_ADDR,
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index a61ef68ada1..8daa68d296b 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -427,7 +427,7 @@ buildsym_compunit::make_blockvector ()
>      {
>      }
>  
> -  blockvector = new (i) struct blockvector (i);
> +  blockvector = new struct blockvector (i);
>  
>    /* Copy the blocks into the blockvector.  This is done in reverse
>       order, which happens to put the blocks into the proper order
> diff --git a/gdb/jit.c b/gdb/jit.c
> index d95fe542a29..4e3abd1802c 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -553,7 +553,7 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
>        filetab->set_linetable (new_table);
>      }
>  
> -  bv = new (actual_nblocks) blockvector (actual_nblocks);
> +  bv = new blockvector (actual_nblocks);
>    cust->set_blockvector (bv);
>  
>    /* At the end of this function, (begin, end) will contain the PC range this
> diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
> index 3cedf0e4fc2..90bbcd81ed2 100644
> --- a/gdb/mdebugread.c
> +++ b/gdb/mdebugread.c
> @@ -256,8 +256,6 @@ static legacy_psymtab *new_psymtab (const char *, psymtab_storage *,
>  static void mdebug_expand_psymtab (legacy_psymtab *pst,
>  				  struct objfile *objfile);
>  
> -static void add_block (struct block *, struct symtab *);
> -
>  static void add_symbol (struct symbol *, struct symtab *, struct block *);
>  
>  static int add_line (struct linetable *, int, CORE_ADDR, int);
> @@ -808,7 +806,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
>        b->set_start (sh->value);
>        b->set_end (sh->value);
>        b->set_superblock (top_stack->cur_block);
> -      add_block (b, top_stack->cur_st);
> +      top_stack->cur_st->compunit ()->blockvector ()->add_block (b);

I don't see it being called out anywhere that this change is now going
to reorder the blocks.  I have no idea if this change is important or
not, but I think it is usually a good idea to talk about this sort thing
in the commit message -- imagine in a few months someone hits a bug in
ECOFF support related to block ordering, they look at this commit, and
they have to figure out why the block ordering changed....

Thanks,
Andrew

>  
>        /* Not if we only have partial info.  */
>        if (SC_IS_UNDEF (sh->sc) || sh->sc == scNil)
> @@ -1136,7 +1134,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
>        b->set_start (sh->value + top_stack->procadr);
>        b->set_superblock (top_stack->cur_block);
>        top_stack->cur_block = b;
> -      add_block (b, top_stack->cur_st);
> +      top_stack->cur_st->compunit ()->blockvector ()->add_block (b);
>        break;
>  
>      case stEnd:		/* end (of anything) */
> @@ -4492,28 +4490,6 @@ add_symbol (struct symbol *s, struct symtab *symtab, struct block *b)
>    mdict_add_symbol (b->multidict (), s);
>  }
>  
> -/* Add a new block B to a symtab S.  */
> -
> -static void
> -add_block (struct block *b, struct symtab *s)
> -{
> -  /* Cast away "const", but that's ok because we're building the
> -     symtab and blockvector here.  */
> -  struct blockvector *old_bv
> -    = (struct blockvector *) s->compunit ()->blockvector ();
> -
> -  struct blockvector *new_bv = new (old_bv->num_blocks () + 1)
> -    blockvector (old_bv->num_blocks () + 1);
> -
> -  s->compunit ()->set_blockvector (old_bv);
> -
> -  for (int i = 0; i < old_bv->num_blocks (); i++)
> -    new_bv->set_block (i, old_bv->block (i));
> -  new_bv->set_block (new_bv->num_blocks () - 1, b);
> -
> -  delete old_bv;
> -}
> -
>  /* Add a new linenumber entry (LINENO,ADR) to a linevector LT.
>     MIPS' linenumber encoding might need more than one byte
>     to describe it, LAST is used to detect these continuation lines.
> @@ -4635,7 +4611,7 @@ new_symtab (const char *name, int maxlines, struct objfile *objfile)
>    lang = cust->language ();
>  
>    /* All symtabs must have at least two blocks.  */
> -  bv = new (2) blockvector (2);
> +  bv = new blockvector (2);
>    bv->set_block (GLOBAL_BLOCK, new_block (objfile, NON_FUNCTION_BLOCK, lang));
>    bv->set_block (STATIC_BLOCK, new_block (objfile, NON_FUNCTION_BLOCK, lang));
>    bv->static_block ()->set_superblock (bv->global_block ());
> -- 
> 2.51.0

Re: [PATCH 1/2] gdb: allocate blockvector on heap

[Simon Marchi]

On 10/13/25 2:23 PM, Jan Vrany wrote:
> This patch changes blockvector to be allocated on the heap, using
> 'new'.
> 
> The code is little awkward since blockvector uses flexible array member
> to hold on blocks. So in order to make it working, this commit overloads
> 'new' and 'delete' operators to take the number of blocks as parameter
> and allocates/deallocates memory using std::malloc and std::free.
> This will go away in following commit that will change blockvector to
> use std::vector rather than flexible array member.

It's already written, so if it works, I don't mind, but otherwise I
think you could have made the switch to 'new' and to std::vector in the
same patch, since they are not very large changes.

> ---
>  gdb/block.h      | 18 ++++++++++++++++++
>  gdb/buildsym.c   |  5 +----
>  gdb/jit.c        |  8 +-------
>  gdb/mdebugread.c | 38 +++++++++++---------------------------
>  4 files changed, 31 insertions(+), 38 deletions(-)
> 
> diff --git a/gdb/block.h b/gdb/block.h
> index 8a3ea822d04..d1ca41d7482 100644
> --- a/gdb/block.h
> +++ b/gdb/block.h
> @@ -420,6 +420,24 @@ struct global_block : public block
>  
>  struct blockvector
>  {
> +  void *operator new (size_t size, size_t num_blocks)
> +  {
> +    size_t size_in_bytes = size + (num_blocks - 1) * (sizeof (struct block *));
> +    return std::malloc (size_in_bytes);
> +  }
> +
> +  void operator delete (void *memory)
> +  {
> +    std::free (memory);
> +  }
> +
> +  blockvector (int nblocks)

explicit

> +    : m_map (nullptr),

Can you initialize m_map inline?

  addrmap_fixed *m_map = nullptr;

Otherwise, LGTM.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

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

[Simon Marchi]

On 10/13/25 2:23 PM, Jan Vrany wrote:
> This commit changes internal implementation of struct blockvector to use
> std::vector<> rather than flexible array, allowing one to easily add
> blocks to existing blockvector.
> 
> This is needed to for lazy CU expansion and for Python API to build
> objfiles, compunits and symtabs dynamically (similarly to JIT reader
> API).
> 
> To avoid extensive reallocation when adding a lot of blocks, ctor takes
> initial number of blocks to be stored in blockvector.
> 
> While at is, it also adds blockvector::add_block() to add new blocks
> and changes mdebugread.c to use it.
> 
> The downside is higher memory consumption. The size of std::vector with
> obstack allocator is 32 bytes (GCC 14) compared to 8 bytes used
> currently to store the number of blocks (m_num_blocks). Stopping gdb at
> its main(), followed by "maint expand-symtabs" results in 4593
> compunit symtabs so in this case the overhead is 24*4593 = 110232 bytes
> which I hope is acceptable.
> ---
>  gdb/block.c      | 36 +++++++++++++++++++++++++++++
>  gdb/block.h      | 59 +++++++++++++++++++++++-------------------------
>  gdb/buildsym.c   |  2 +-
>  gdb/jit.c        |  2 +-
>  gdb/mdebugread.c | 30 +++---------------------
>  5 files changed, 69 insertions(+), 60 deletions(-)
> 
> diff --git a/gdb/block.c b/gdb/block.c
> index e09bccdcf86..a9191c3fdff 100644
> --- a/gdb/block.c
> +++ b/gdb/block.c
> @@ -17,6 +17,7 @@
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
> +#include <cstring>

I'm not sure which change of yours requires cstring (perhaps this include has been
auto-inserted, but if it's needed, I'd use <string.h>, just for consistency with the
rest of the codebase.

>  #include "block.h"
>  #include "symtab.h"
>  #include "symfile.h"
> @@ -809,6 +810,41 @@ make_blockranges (struct objfile *objfile,
>    return blr;
>  }
>  
> +static bool
> +block_ordering_predicate (struct block *b1, struct block *b2)
> +{
> +  CORE_ADDR start1 = b1->start ();
> +  CORE_ADDR start2 = b2->start ();
> +
> +  if (start1 != start2)
> +    return start1 < start2;
> +  return (b2->end () < b1->end ());
> +}
> +
> +/* See block.h.  */
> +
> +void
> +blockvector::add_block (struct block *block)
> +{
> +  if (num_blocks () <= FIRST_LOCAL_BLOCK)
> +    {
> +      /* No blocks (except global and static block).  */
> +      m_blocks.push_back (block);
> +    }
> +  else
> +    {
> +      /* blockvector already contains some blocks.  Insert new block
> +   to a correct place.  */

Indentation is off.

> +      auto first = m_blocks.begin () + FIRST_LOCAL_BLOCK;
> +      auto last = m_blocks.end ();
> +
> +      auto insert_before = std::upper_bound (first, last, block,
> +					     block_ordering_predicate);
> +
> +      m_blocks.insert (insert_before, block);
> +    }
> +}

This function makes me a bit worried, because I feel like it could (even
though unlikely) end up mis-used and end up with a situation like this:

https://inbox.sourceware.org/gdb-patches/b5853237-a041-4a74-aceb-0406eeb926ec@polymtl.ca/T/#m95998810ae7943611bec2a499070b735f6ec4928

That is, if you add some blocks not in the sorted order already and
insert blocks in the middle of the vector, you end up shifting a lot of
elements, and it can start to get noticeable.  The only user of this
function right now is mdebugread, and it didn't seem to need this
"insert" in the middle approach.  For that case, we could perhaps have
an append_block method, which would have the precondition (could be
quickly checked with a gdb_assert) that it must sort after the last
element of the vector?

However, I guess that for lazy CU expansion, we'll need the ability to
insert blocks in the middle, as we expand more of a CU?  In this case,
we would perhaps want a way to add all the new blocks, and then do one
sort at the end, rather than sorted insertions.

I also wish we could avoid having a "set_block" method, because it
requires having the blockvector in an invalid state (some blocks slots
set to nullptr) while we build it, which could be error prone.  The
users of the set_block method are buildsym and jit.  From what i saw,
they could both very well prepare an std::vector with their blocks and
std::move that vector into a blockvector constructor.  That constructor
could assert that the blocks are correctly ordered, or to the sort
itself (something that both buildsym and jit do already).  buildsym
starts with a singly-linked list (m_pending_blocks), moves the blocks to a
temporary vector (in end_compunit_symtab_get_static_block), sorts that
vector, builds a singly-linked list again, and then traverses that list
to insert the blocks into the blockvector.  I think that could all be
simplified by making m_pending_blocks an std::vector<block *> from the
start.

Hopefully, something like this would make it harder to build a
blockvector in an invalid state.

Simon

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

[Simon Marchi]

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.

Simon

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

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> I also wish we could avoid having a "set_block" method, because it
Simon> requires having the blockvector in an invalid state (some blocks slots
Simon> set to nullptr) while we build it, which could be error prone.  The
Simon> users of the set_block method are buildsym and jit.  From what i saw,
Simon> they could both very well prepare an std::vector with their blocks and
Simon> std::move that vector into a blockvector constructor.  That constructor
Simon> could assert that the blocks are correctly ordered, or to the sort
Simon> itself (something that both buildsym and jit do already).  buildsym
Simon> starts with a singly-linked list (m_pending_blocks), moves the blocks to a
Simon> temporary vector (in end_compunit_symtab_get_static_block), sorts that
Simon> vector, builds a singly-linked list again, and then traverses that list
Simon> to insert the blocks into the blockvector.  I think that could all be
Simon> simplified by making m_pending_blocks an std::vector<block *> from the
Simon> start.

Are you proposing that Jan do this in this patch?  Or is this just a
future-looking comment?  It wasn't clear to me.

Tom

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

[Simon Marchi]

On 10/14/25 4:05 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> I also wish we could avoid having a "set_block" method, because it
> Simon> requires having the blockvector in an invalid state (some blocks slots
> Simon> set to nullptr) while we build it, which could be error prone.  The
> Simon> users of the set_block method are buildsym and jit.  From what i saw,
> Simon> they could both very well prepare an std::vector with their blocks and
> Simon> std::move that vector into a blockvector constructor.  That constructor
> Simon> could assert that the blocks are correctly ordered, or to the sort
> Simon> itself (something that both buildsym and jit do already).  buildsym
> Simon> starts with a singly-linked list (m_pending_blocks), moves the blocks to a
> Simon> temporary vector (in end_compunit_symtab_get_static_block), sorts that
> Simon> vector, builds a singly-linked list again, and then traverses that list
> Simon> to insert the blocks into the blockvector.  I think that could all be
> Simon> simplified by making m_pending_blocks an std::vector<block *> from the
> Simon> start.
> 
> Are you proposing that Jan do this in this patch?  Or is this just a
> future-looking comment?  It wasn't clear to me.
> 
> Tom

Sorry about that.  I would not ask Jan to refactor or clean up
everything in buildsym and jit.  But perhaps we can agree on what a good
(easy to use, hard to mis-use) API for blockvector is, and then Jan can
do the bare minimum to wire up buildsym and jit to that API.  We can
then do some cleanups later to remove some unnecessary steps in buildsym
and jit.

Simon

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

[Jan Vraný]

On Tue, 2025-10-14 at 16:19 -0400, Simon Marchi wrote:
> On 10/14/25 4:05 PM, Tom Tromey wrote:
> > > > > > > "Simon" == Simon Marchi <simark@simark.ca> writes:
> > 
> > Simon> I also wish we could avoid having a "set_block" method, because it
> > Simon> requires having the blockvector in an invalid state (some blocks slots
> > Simon> set to nullptr) while we build it, which could be error prone.  The
> > Simon> users of the set_block method are buildsym and jit.  From what i saw,
> > Simon> they could both very well prepare an std::vector with their blocks and
> > Simon> std::move that vector into a blockvector constructor.  That constructor
> > Simon> could assert that the blocks are correctly ordered, or to the sort
> > Simon> itself (something that both buildsym and jit do already).  buildsym
> > Simon> starts with a singly-linked list (m_pending_blocks), moves the blocks to a
> > Simon> temporary vector (in end_compunit_symtab_get_static_block), sorts that
> > Simon> vector, builds a singly-linked list again, and then traverses that list
> > Simon> to insert the blocks into the blockvector.  I think that could all be
> > Simon> simplified by making m_pending_blocks an std::vector<block *> from the
> > Simon> start.
> > 
> > Are you proposing that Jan do this in this patch?  Or is this just a
> > future-looking comment?  It wasn't clear to me.
> > 
> > Tom
> 
> Sorry about that.  I would not ask Jan to refactor or clean up
> everything in buildsym and jit.  But perhaps we can agree on what a good
> (easy to use, hard to mis-use) API for blockvector is, and then Jan can
> do the bare minimum to wire up buildsym and jit to that API.  We can
> then do some cleanups later to remove some unnecessary steps in buildsym
> and jit.

At the moment it seems to me that the first step is to drop add_block(),
add append_block() and common ordering predicate and take it from there.
I'll try to have a look tomorrow.

Thanks! 

Jan

> 
> Simon

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

[Simon Marchi]

On 2025-10-14 16:40, Jan Vraný wrote:
> On Tue, 2025-10-14 at 16:19 -0400, Simon Marchi wrote:
>> On 10/14/25 4:05 PM, Tom Tromey wrote:
>>>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
>>>
>>> Simon> I also wish we could avoid having a "set_block" method, because it
>>> Simon> requires having the blockvector in an invalid state (some blocks slots
>>> Simon> set to nullptr) while we build it, which could be error prone.  The
>>> Simon> users of the set_block method are buildsym and jit.  From what i saw,
>>> Simon> they could both very well prepare an std::vector with their blocks and
>>> Simon> std::move that vector into a blockvector constructor.  That constructor
>>> Simon> could assert that the blocks are correctly ordered, or to the sort
>>> Simon> itself (something that both buildsym and jit do already).  buildsym
>>> Simon> starts with a singly-linked list (m_pending_blocks), moves the blocks to a
>>> Simon> temporary vector (in end_compunit_symtab_get_static_block), sorts that
>>> Simon> vector, builds a singly-linked list again, and then traverses that list
>>> Simon> to insert the blocks into the blockvector.  I think that could all be
>>> Simon> simplified by making m_pending_blocks an std::vector<block *> from the
>>> Simon> start.
>>>
>>> Are you proposing that Jan do this in this patch?  Or is this just a
>>> future-looking comment?  It wasn't clear to me.
>>>
>>> Tom
>>
>> Sorry about that.  I would not ask Jan to refactor or clean up
>> everything in buildsym and jit.  But perhaps we can agree on what a good
>> (easy to use, hard to mis-use) API for blockvector is, and then Jan can
>> do the bare minimum to wire up buildsym and jit to that API.  We can
>> then do some cleanups later to remove some unnecessary steps in buildsym
>> and jit.
> 
> At the moment it seems to me that the first step is to drop add_block(),
> add append_block() and common ordering predicate and take it from there.
> I'll try to have a look tomorrow.

Sounds fine to me.  And I think it's fine to have set_block for now.
Code today pokes the block list directly, so set_block isn't introducing
anything new.

Simon

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

[Jan Vraný]

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