jit: make gdb_symtab::blocks an std::forward_list
authorSimon Marchi <simon.marchi@polymtl.ca>
Mon, 16 Dec 2019 21:30:50 +0000 (16:30 -0500)
committerSimon Marchi <simon.marchi@efficios.com>
Mon, 16 Dec 2019 21:30:50 +0000 (16:30 -0500)
This patch changes the gdb_symtab::blocks manually maintained linked
list to be an std::forward_list, simplifying memory management.

Currently, the list is sorted as blocks are created.  With an
std::forward_list, it is easier (and probably a bit more efficient) to
sort them once at the end, so this is what I did.

A note about the comment on the "next" field:

  /* gdb_blocks are linked into a tree structure.  Next points to the
     next node at the same depth as this block and parent to the
     parent gdb_block.  */

I don't think it's true that "next" points to the next node at the same
depth.  All nodes are in a simple singly linked list, so necessarily
some node will point to some other node that isn't at the same depth.

gdb/ChangeLog:

* jit.c (struct gdb_block) <next>: Remove field.
(struct gdb_symtab) <~gdb_symtab>: Remove.
<blocks>: Change type to std::forward_list<gdb_block>.
(compare_block): Remove.
(jit_block_open_impl): Adjust to std::forward_list.  Place the new
block at the beginning, don't mind about sorting.
(finalize_symtab): Adjust to std::forward_list, sort the blocks list
before using it.

gdb/ChangeLog
gdb/jit.c

index c797e87703f43491c00128658e14f1d85a79241f..fa3f0a689357b9c998b51177b89bdf4016e12228 100644 (file)
@@ -1,3 +1,14 @@
+2019-12-16  Simon Marchi  <simon.marchi@polymtl.ca>
+
+       * jit.c (struct gdb_block) <next>: Remove field.
+       (struct gdb_symtab) <~gdb_symtab>: Remove.
+       <blocks>: Change type to std::forward_list<gdb_block>.
+       (compare_block): Remove.
+       (jit_block_open_impl): Adjust to std::forward_list.  Place the new
+       block at the beginning, don't mind about sorting.
+       (finalize_symtab): Adjust to std::forward_list, sort the blocks list
+       before using it.
+
 2019-12-16  Simon Marchi  <simon.marchi@polymtl.ca>
 
        * jit.c (struct gdb_block): Add constructor, initialize
index bb6b6bacb5d17a5c3218c284e976ac310e071143..0dd11e14d2f82c4732213351aaccdb8c6776716a 100644 (file)
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -437,10 +437,8 @@ struct gdb_block
       name (name != nullptr ? xstrdup (name) : nullptr)
   {}
 
-  /* gdb_blocks are linked into a tree structure.  Next points to the
-     next node at the same depth as this block and parent to the
-     parent gdb_block.  */
-  struct gdb_block *next = nullptr, *parent;
+  /* The parent of this block.  */
+  struct gdb_block *parent;
 
   /* Points to the "real" block that is being built out of this
      instance.  This block will be added to a blockvector, which will
@@ -463,23 +461,14 @@ struct gdb_symtab
     : file_name (file_name != nullptr ? file_name : "")
   {}
 
-  ~gdb_symtab ()
-  {
-    gdb_block *gdb_block_iter, *gdb_block_iter_tmp;
-
-    for ((gdb_block_iter = this->blocks,
-         gdb_block_iter_tmp = gdb_block_iter->next);
-         gdb_block_iter;
-         gdb_block_iter = gdb_block_iter_tmp)
-      {
-        gdb_block_iter_tmp = gdb_block_iter->next;
-       delete gdb_block_iter;
-      }
-  }
-
   /* The list of blocks in this symtab.  These will eventually be
-     converted to real blocks.  */
-  struct gdb_block *blocks = nullptr;
+     converted to real blocks.
+
+     This is specifically a linked list, instead of, for example, a vector,
+     because the pointers are returned to the user's debug info reader.  So
+     it's important that the objects don't change location during their
+     lifetime (which would happen with a vector of objects getting resized).  */
+  std::forward_list<gdb_block> blocks;
 
   /* The number of blocks inserted.  */
   int nblocks = 0;
@@ -550,28 +539,6 @@ jit_symtab_open_impl (struct gdb_symbol_callbacks *cb,
   return &object->symtabs.front ();
 }
 
-/* Returns true if the block corresponding to old should be placed
-   before the block corresponding to new in the final blockvector.  */
-
-static int
-compare_block (const struct gdb_block *const old,
-              const struct gdb_block *const newobj)
-{
-  if (old == NULL)
-    return 1;
-  if (old->begin < newobj->begin)
-    return 1;
-  else if (old->begin == newobj->begin)
-    {
-      if (old->end > newobj->end)
-       return 1;
-      else
-       return 0;
-    }
-  else
-    return 0;
-}
-
 /* Called by readers to open a new gdb_block.  This function also
    inserts the new gdb_block in the correct place in the corresponding
    gdb_symtab.  */
@@ -581,35 +548,12 @@ jit_block_open_impl (struct gdb_symbol_callbacks *cb,
                     struct gdb_symtab *symtab, struct gdb_block *parent,
                     GDB_CORE_ADDR begin, GDB_CORE_ADDR end, const char *name)
 {
-  struct gdb_block *block = new gdb_block (parent, begin, end, name);
-
-  block->next = symtab->blocks;
-
-  /* Ensure that the blocks are inserted in the correct (reverse of
-     the order expected by blockvector).  */
-  if (compare_block (symtab->blocks, block))
-    {
-      symtab->blocks = block;
-    }
-  else
-    {
-      struct gdb_block *i = symtab->blocks;
-
-      for (;; i = i->next)
-       {
-         /* Guaranteed to terminate, since compare_block (NULL, _)
-            returns 1.  */
-         if (compare_block (i->next, block))
-           {
-             block->next = i->next;
-             i->next = block;
-             break;
-           }
-       }
-    }
+  /* Place the block at the beginning of the list, it will be sorted when the
+     symtab is finalized.  */
+  symtab->blocks.emplace_front (parent, begin, end, name);
   symtab->nblocks++;
 
-  return block;
+  return &symtab->blocks.front ();
 }
 
 /* Readers call this to add a line mapping (from PC to line number) to
@@ -655,14 +599,20 @@ static void
 finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
 {
   struct compunit_symtab *cust;
-  struct gdb_block *gdb_block_iter;
-  struct block *block_iter;
-  int actual_nblocks, i;
   size_t blockvector_size;
   CORE_ADDR begin, end;
   struct blockvector *bv;
 
-  actual_nblocks = FIRST_LOCAL_BLOCK + stab->nblocks;
+  int actual_nblocks = FIRST_LOCAL_BLOCK + stab->nblocks;
+
+  /* Sort the blocks in the order they should appear in the blockvector.  */
+  stab->blocks.sort([] (const gdb_block &a, const gdb_block &b)
+    {
+      if (a.begin != b.begin)
+       return a.begin < b.begin;
+
+      return a.end > b.end;
+    });
 
   cust = allocate_compunit_symtab (objfile, stab->file_name.c_str ());
   allocate_symtab (cust, stab->file_name.c_str ());
@@ -689,19 +639,18 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
                                             blockvector_size);
   COMPUNIT_BLOCKVECTOR (cust) = bv;
 
-  /* (begin, end) will contain the PC range this entire blockvector
-     spans.  */
+  /* At the end of this function, (begin, end) will contain the PC range this
+     entire blockvector spans.  */
   BLOCKVECTOR_MAP (bv) = NULL;
-  begin = stab->blocks->begin;
-  end = stab->blocks->end;
+  begin = stab->blocks.front ().begin;
+  end = stab->blocks.front ().end;
   BLOCKVECTOR_NBLOCKS (bv) = actual_nblocks;
 
   /* First run over all the gdb_block objects, creating a real block
      object for each.  Simultaneously, keep setting the real_block
      fields.  */
-  for (i = (actual_nblocks - 1), gdb_block_iter = stab->blocks;
-       i >= FIRST_LOCAL_BLOCK;
-       i--, gdb_block_iter = gdb_block_iter->next)
+  int block_idx = FIRST_LOCAL_BLOCK;
+  for (gdb_block &gdb_block_iter : stab->blocks)
     {
       struct block *new_block = allocate_block (&objfile->objfile_obstack);
       struct symbol *block_name = allocate_symbol (objfile);
@@ -713,8 +662,8 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
       BLOCK_MULTIDICT (new_block)
        = mdict_create_linear (&objfile->objfile_obstack, NULL);
       /* The address range.  */
-      BLOCK_START (new_block) = (CORE_ADDR) gdb_block_iter->begin;
-      BLOCK_END (new_block) = (CORE_ADDR) gdb_block_iter->end;
+      BLOCK_START (new_block) = (CORE_ADDR) gdb_block_iter.begin;
+      BLOCK_END (new_block) = (CORE_ADDR) gdb_block_iter.end;
 
       /* The name.  */
       SYMBOL_DOMAIN (block_name) = VAR_DOMAIN;
@@ -724,22 +673,24 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
       SYMBOL_BLOCK_VALUE (block_name) = new_block;
 
       block_name->name = obstack_strdup (&objfile->objfile_obstack,
-                                        gdb_block_iter->name.get ());
+                                        gdb_block_iter.name.get ());
 
       BLOCK_FUNCTION (new_block) = block_name;
 
-      BLOCKVECTOR_BLOCK (bv, i) = new_block;
+      BLOCKVECTOR_BLOCK (bv, block_idx) = new_block;
       if (begin > BLOCK_START (new_block))
        begin = BLOCK_START (new_block);
       if (end < BLOCK_END (new_block))
        end = BLOCK_END (new_block);
 
-      gdb_block_iter->real_block = new_block;
+      gdb_block_iter.real_block = new_block;
+
+      block_idx++;
     }
 
   /* Now add the special blocks.  */
-  block_iter = NULL;
-  for (i = 0; i < FIRST_LOCAL_BLOCK; i++)
+  struct block *block_iter = NULL;
+  for (enum block_enum i : { GLOBAL_BLOCK, STATIC_BLOCK })
     {
       struct block *new_block;
 
@@ -762,21 +713,19 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
 
   /* Fill up the superblock fields for the real blocks, using the
      real_block fields populated earlier.  */
-  for (gdb_block_iter = stab->blocks;
-       gdb_block_iter;
-       gdb_block_iter = gdb_block_iter->next)
+  for (gdb_block &gdb_block_iter : stab->blocks)
     {
-      if (gdb_block_iter->parent != NULL)
+      if (gdb_block_iter.parent != NULL)
        {
          /* If the plugin specifically mentioned a parent block, we
             use that.  */
-         BLOCK_SUPERBLOCK (gdb_block_iter->real_block) =
-           gdb_block_iter->parent->real_block;
+         BLOCK_SUPERBLOCK (gdb_block_iter.real_block) =
+           gdb_block_iter.parent->real_block;
        }
       else
        {
          /* And if not, we set a default parent block.  */
-         BLOCK_SUPERBLOCK (gdb_block_iter->real_block) =
+         BLOCK_SUPERBLOCK (gdb_block_iter.real_block) =
            BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
        }
     }