[PATCH v2 0/5] Fix and cleanups in jit.c

[PATCH v2 0/5] Fix and cleanups in jit.c

[Simon Marchi]

This is a follow-up to:

  https://sourceware.org/ml/gdb-patches/2019-12/msg00568.html

Again, the first patch is a fix and the rest is some c++ification.

I think I fixed all review comments from v1, the biggest change being
the use of std::forward_list instead of std::vector.

Simon Marchi (5):
  Fix double-free when creating more than one block in JIT debug info
    reader
  jit: c++-ify gdb_symtab
  jit: make gdb_object::symtabs an std::forward_list
  jit: c++-ify gdb_block
  jit: make gdb_symtab::blocks an std::forward_list

 gdb/jit.c                             | 207 ++++++++++----------------
 gdb/testsuite/gdb.base/jit-reader.exp |  14 +-
 gdb/testsuite/gdb.base/jithost.c      |  45 ++++--
 gdb/testsuite/gdb.base/jithost.h      |  15 +-
 gdb/testsuite/gdb.base/jitreader.c    |  34 +++--
 5 files changed, 158 insertions(+), 157 deletions(-)

-- 
2.24.1

[PATCH v2 3/5] jit: make gdb_object::symtabs an std::forward_list

[Simon Marchi]

Replace the manual linked list with an std::forward_list, simplifying
the memory management.  This requires allocating gdb_object with new and
free'ing it with delete.

gdb/ChangeLog:

	* jit.c: Include forward_list.
	(struct gdb_symtab) <next>: Remove field.
	(struct gdb_object) <symtabs>: Change type to
	std::forward_list<gdb_symtab>.
	(jit_object_open_impl): Allocate gdb_object with new.
	(jit_symtab_open_impl): Adjust to std::forward_list.
	(finalize_symtab): Don't delete symtab.
	(jit_object_close_impl):  Adjust to std::forward_list.  Free
	gdb_object with delete.
---
 gdb/jit.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index 07767275f533..a731edd870d3 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -41,6 +41,7 @@
 #include "gdb_bfd.h"
 #include "readline/tilde.h"
 #include "completer.h"
+#include <forward_list>
 
 static std::string jit_reader_dir;
 
@@ -481,15 +482,19 @@ struct gdb_symtab
 
   /* The source file for this symtab.  */
   std::string file_name;
-
-  struct gdb_symtab *next = nullptr;
 };
 
 /* Proxy object for building an object.  */
 
 struct gdb_object
 {
-  struct gdb_symtab *symtabs;
+  /* Symtabs of this object.
+
+     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_symtab> symtabs;
 };
 
 /* The type of the `private' data passed around by the callback
@@ -521,7 +526,7 @@ jit_object_open_impl (struct gdb_symbol_callbacks *cb)
   /* CB is not required right now, but sometime in the future we might
      need a handle to it, and we'd like to do that without breaking
      the ABI.  */
-  return XCNEW (struct gdb_object);
+  return new gdb_object;
 }
 
 /* Readers call into this function to open a new gdb_symtab, which,
@@ -534,10 +539,8 @@ jit_symtab_open_impl (struct gdb_symbol_callbacks *cb,
 {
   /* CB stays unused.  See comment in jit_object_open_impl.  */
 
-  gdb_symtab *ret = new gdb_symtab (file_name);
-  ret->next = object->symtabs;
-  object->symtabs = ret;
-  return ret;
+  object->symtabs.emplace_front (file_name);
+  return &object->symtabs.front ();
 }
 
 /* Returns true if the block corresponding to old should be placed
@@ -774,8 +777,6 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
 	    BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 	}
     }
-
-  delete stab;
 }
 
 /* Called when closing a gdb_objfile.  Converts OBJ to a proper
@@ -785,7 +786,6 @@ static void
 jit_object_close_impl (struct gdb_symbol_callbacks *cb,
 		       struct gdb_object *obj)
 {
-  struct gdb_symtab *i, *j;
   struct objfile *objfile;
   jit_dbg_reader_data *priv_data;
 
@@ -795,14 +795,12 @@ jit_object_close_impl (struct gdb_symbol_callbacks *cb,
 			   OBJF_NOT_FILENAME);
   objfile->per_bfd->gdbarch = target_gdbarch ();
 
-  j = NULL;
-  for (i = obj->symtabs; i; i = j)
-    {
-      j = i->next;
-      finalize_symtab (i, objfile);
-    }
+  for (gdb_symtab &symtab : obj->symtabs)
+    finalize_symtab (&symtab, objfile);
+
   add_objfile_entry (objfile, *priv_data);
-  xfree (obj);
+
+  delete obj;
 }
 
 /* Try to read CODE_ENTRY using the loaded jit reader (if any).
-- 
2.24.1

[PATCH v2 2/5] jit: c++-ify gdb_symtab

[Simon Marchi]

This patch makes the gdb_symtab bit more c++y, in preparation for the
next patch that will use an std::forward_list<gdb_symtab>.  It changes
the fields to use automatic memory management, in the form of
std::string and gdb::unique_xmalloc_ptr, and adds a constructor and a
destructor.

gdb/ChangeLog:

	* jit.c (struct gdb_symtab): Add constructor, destructor,
	initialize fields.
	<linetable>: Change type to unique_xmalloc_ptr.
	<file_name>: Change type to std::string.
	(jit_symtab_open_impl): Allocate gdb_symtab with new.
	(jit_symtab_line_mapping_add_impl): Adjust.
	(finalize_symtab): Adjust, call delete on stab.
---
 gdb/jit.c | 62 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index 1cd487502c5e..07767275f533 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -450,19 +450,39 @@ struct gdb_block
 
 struct gdb_symtab
 {
+  explicit gdb_symtab (const char *file_name)
+    : 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;
+        xfree ((void *) gdb_block_iter->name);
+        xfree (gdb_block_iter);
+      }
+  }
+
   /* The list of blocks in this symtab.  These will eventually be
      converted to real blocks.  */
-  struct gdb_block *blocks;
+  struct gdb_block *blocks = nullptr;
 
   /* The number of blocks inserted.  */
-  int nblocks;
+  int nblocks = 0;
 
   /* A mapping between line numbers to PC.  */
-  struct linetable *linetable;
+  gdb::unique_xmalloc_ptr<struct linetable> linetable;
 
   /* The source file for this symtab.  */
-  const char *file_name;
-  struct gdb_symtab *next;
+  std::string file_name;
+
+  struct gdb_symtab *next = nullptr;
 };
 
 /* Proxy object for building an object.  */
@@ -512,12 +532,9 @@ jit_symtab_open_impl (struct gdb_symbol_callbacks *cb,
 		      struct gdb_object *object,
 		      const char *file_name)
 {
-  struct gdb_symtab *ret;
-
   /* CB stays unused.  See comment in jit_object_open_impl.  */
 
-  ret = XCNEW (struct gdb_symtab);
-  ret->file_name = file_name ? xstrdup (file_name) : xstrdup ("");
+  gdb_symtab *ret = new gdb_symtab (file_name);
   ret->next = object->symtabs;
   object->symtabs = ret;
   return ret;
@@ -605,7 +622,7 @@ jit_symtab_line_mapping_add_impl (struct gdb_symbol_callbacks *cb,
 
   alloc_len = sizeof (struct linetable)
 	      + (nlines - 1) * sizeof (struct linetable_entry);
-  stab->linetable = (struct linetable *) xmalloc (alloc_len);
+  stab->linetable.reset (XNEWVAR (struct linetable, alloc_len));
   stab->linetable->nitems = nlines;
   for (i = 0; i < nlines; i++)
     {
@@ -632,7 +649,7 @@ static void
 finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
 {
   struct compunit_symtab *cust;
-  struct gdb_block *gdb_block_iter, *gdb_block_iter_tmp;
+  struct gdb_block *gdb_block_iter;
   struct block *block_iter;
   int actual_nblocks, i;
   size_t blockvector_size;
@@ -641,8 +658,8 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
 
   actual_nblocks = FIRST_LOCAL_BLOCK + stab->nblocks;
 
-  cust = allocate_compunit_symtab (objfile, stab->file_name);
-  allocate_symtab (cust, stab->file_name);
+  cust = allocate_compunit_symtab (objfile, stab->file_name.c_str ());
+  allocate_symtab (cust, stab->file_name.c_str ());
   add_compunit_symtab_to_objfile (cust);
 
   /* JIT compilers compile in memory.  */
@@ -656,8 +673,8 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
 		     + sizeof (struct linetable));
       SYMTAB_LINETABLE (COMPUNIT_FILETABS (cust))
 	= (struct linetable *) obstack_alloc (&objfile->objfile_obstack, size);
-      memcpy (SYMTAB_LINETABLE (COMPUNIT_FILETABS (cust)), stab->linetable,
-	      size);
+      memcpy (SYMTAB_LINETABLE (COMPUNIT_FILETABS (cust)),
+	      stab->linetable.get (), size);
     }
 
   blockvector_size = (sizeof (struct blockvector)
@@ -758,20 +775,7 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
 	}
     }
 
-  /* Free memory.  */
-  gdb_block_iter = stab->blocks;
-
-  for (gdb_block_iter = stab->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;
-      xfree ((void *) gdb_block_iter->name);
-      xfree (gdb_block_iter);
-    }
-  xfree (stab->linetable);
-  xfree ((char *) stab->file_name);
-  xfree (stab);
+  delete stab;
 }
 
 /* Called when closing a gdb_objfile.  Converts OBJ to a proper
-- 
2.24.1

[PATCH v2 1/5] Fix double-free when creating more than one block in JIT debug info reader

[Simon Marchi]

A double-free happens when using a JIT debug info reader that creates
more than one block.  In the loop that frees blocks in finalize_symtab,
at the very end, the gdb_block_iter_tmp variable is set initially, but
not changed as the loop advances.  If we have two blocks, the first
iteration frees the first block, the second iteration frees the second
block, but the third iteration tries to free the second block again, as
gdb_block_iter_tmp keeps pointing on the second block.

Fix it by assigning the gdb_block_iter_tmp variable in the loop.

I have improved the jit-reader.exp test to cover this case, by adding a
second "JIT-ed" function and creating a block for it.  I have renamed
the existing function to something I find a bit more descriptive.  There
are no significant changes to jit-reader.exp itself, only updates
following the renaming.  The important changes are in jithost.c
(generate a new function) and in jitreader.c (create a gdb_block for
that function).

This was found because of an ASan report:

$ ./gdb testsuite/outputs/gdb.base/jit-reader/jit-reader -ex "jit-reader-load /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/jit-reader/jitreader.so" -ex r
Reading symbols from testsuite/outputs/gdb.base/jit-reader/jit-reader...
Starting program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/jit-reader/jit-reader
=================================================================
==1751048==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000042eb8 at pc 0x5650ef8eec88 bp 0x7ffe52767290 sp 0x7ffe52767280
READ of size 8 at 0x604000042eb8 thread T0
    #0 0x5650ef8eec87 in finalize_symtab /home/simark/src/binutils-gdb/gdb/jit.c:768
    #1 0x5650ef8eef88 in jit_object_close_impl /home/simark/src/binutils-gdb/gdb/jit.c:797
    #2 0x7fbbda986278 in read_debug_info /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/jitreader.c:71
    #3 0x5650ef8ef56b in jit_reader_try_read_symtab /home/simark/src/binutils-gdb/gdb/jit.c:850
    #4 0x5650ef8effe3 in jit_register_code /home/simark/src/binutils-gdb/gdb/jit.c:948
    #5 0x5650ef8f2c92 in jit_event_handler(gdbarch*) /home/simark/src/binutils-gdb/gdb/jit.c:1396
    #6 0x5650ef0d137e in handle_jit_event /home/simark/src/binutils-gdb/gdb/breakpoint.c:5470
    [snip]

0x604000042eb8 is located 40 bytes inside of 48-byte region [0x604000042e90,0x604000042ec0)
freed by thread T0 here:
    #0 0x7fbbe57376b0 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:122
    #1 0x5650ef8f350b in xfree<gdb_block> /home/simark/src/binutils-gdb/gdb/gdbsupport/common-utils.h:62
    #2 0x5650ef8eeca9 in finalize_symtab /home/simark/src/binutils-gdb/gdb/jit.c:769
    #3 0x5650ef8eef88 in jit_object_close_impl /home/simark/src/binutils-gdb/gdb/jit.c:797
    #4 0x7fbbda986278 in read_debug_info /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/jitreader.c:71
    #5 0x5650ef8ef56b in jit_reader_try_read_symtab /home/simark/src/binutils-gdb/gdb/jit.c:850
    #6 0x5650ef8effe3 in jit_register_code /home/simark/src/binutils-gdb/gdb/jit.c:948
    #7 0x5650ef8f2c92 in jit_event_handler(gdbarch*) /home/simark/src/binutils-gdb/gdb/jit.c:1396
    #8 0x5650ef0d137e in handle_jit_event /home/simark/src/binutils-gdb/gdb/breakpoint.c:5470
    [snip]

previously allocated by thread T0 here:
    #0 0x7fbbe5737cd8 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:153
    #1 0x5650eef662f3 in xcalloc /home/simark/src/binutils-gdb/gdb/alloc.c:100
    #2 0x5650ef8f34ea in xcnew<gdb_block> /home/simark/src/binutils-gdb/gdb/gdbsupport/poison.h:122
    #3 0x5650ef8ed467 in jit_block_open_impl /home/simark/src/binutils-gdb/gdb/jit.c:557
    #4 0x7fbbda98620a in read_debug_info /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/jitreader.c:60
    #5 0x5650ef8ef56b in jit_reader_try_read_symtab /home/simark/src/binutils-gdb/gdb/jit.c:850
    #6 0x5650ef8effe3 in jit_register_code /home/simark/src/binutils-gdb/gdb/jit.c:948
    #7 0x5650ef8f2c92 in jit_event_handler(gdbarch*) /home/simark/src/binutils-gdb/gdb/jit.c:1396
    #8 0x5650ef0d137e in handle_jit_event /home/simark/src/binutils-gdb/gdb/breakpoint.c:5470
    [snip]

gdb/ChangeLog:

	* jit.c (finalize_symtab): Set gdb_block_iter_tmp in loop.

gdb/testsuite/ChangeLog:

	* gdb.base/jit-reader.exp (jit_reader_test): Rename
	jit_function_00 to jit_function_stack_mangle.
	* gdb.base/jithost.c (jit_function_t): Rename to...
	(jit_function_stack_mangle_t): ... this.
	(jit_function_add_t): New typedef.
	(jit_function_00_code): Rename to...
	(jit_function_stack_mangle_code): ... this, make static.
	(jit_function_add_code): New.
	(main): Generate "add" function and call it.  Adjust to changes
	in jithost_abi.
	* gdb.base/jithost.h (struct jithost_abi_bounds): New.
	(struct jithost_abi) <begin, end>: Remove fields.
	<object, function_stack_mangle, function_add>: New fields.
	* gdb.base/jitreader.c (struct reader_state) <code_begin,
	code_end>: Remove fields.
	<func_stack_mangle>: New field.
	(read_debug_info): Adjust to renaming, create block for "add"
	function.
	(read_sp, unwind_frame, get_frame_id): Adjust to other changes.
---
 gdb/jit.c                             |  1 +
 gdb/testsuite/gdb.base/jit-reader.exp | 14 ++++-----
 gdb/testsuite/gdb.base/jithost.c      | 45 ++++++++++++++++++++-------
 gdb/testsuite/gdb.base/jithost.h      | 15 +++++++--
 gdb/testsuite/gdb.base/jitreader.c    | 34 +++++++++++++-------
 5 files changed, 78 insertions(+), 31 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index 9ea68330212a..1cd487502c5e 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -765,6 +765,7 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
        gdb_block_iter;
        gdb_block_iter = gdb_block_iter_tmp)
     {
+      gdb_block_iter_tmp = gdb_block_iter->next;
       xfree ((void *) gdb_block_iter->name);
       xfree (gdb_block_iter);
     }
diff --git a/gdb/testsuite/gdb.base/jit-reader.exp b/gdb/testsuite/gdb.base/jit-reader.exp
index 1ef3341bd253..639c95f7402f 100644
--- a/gdb/testsuite/gdb.base/jit-reader.exp
+++ b/gdb/testsuite/gdb.base/jit-reader.exp
@@ -120,7 +120,7 @@ proc jit_reader_test {} {
 	with_test_prefix "before mangling" {
 	    gdb_test "bt" \
 		[multi_line \
-		     "#0 ${any} in jit_function_00 ${any}" \
+		     "#0 ${any} in jit_function_stack_mangle ${any}" \
 		     "#1 ${any} in main ${any}" \
 		    ] \
 		"bt works"
@@ -128,7 +128,7 @@ proc jit_reader_test {} {
 	    set sp_before_mangling \
 		[get_hexadecimal_valueof "\$sp" 0 "get sp"]
 
-	    gdb_test "up" "#1  $any in main $any\r\n$any  function $any" \
+	    gdb_test "up" "#1  $any in main $any\r\n$any  function_stack_mangle $any" \
 		"move up to caller"
 
 	    set caller_sp \
@@ -140,7 +140,7 @@ proc jit_reader_test {} {
 	# reader's unwinder understands the mangling and should thus
 	# be able to unwind at that location.
 	with_test_prefix "after mangling" {
-	    gdb_test "si" "in jit_function_00 .*" "step over stack mangling"
+	    gdb_test "si" "in jit_function_stack_mangle .*" "step over stack mangling"
 
 	    set sp_after_mangling \
 		[get_hexadecimal_valueof "\$sp" 0 "get sp"]
@@ -152,7 +152,7 @@ proc jit_reader_test {} {
 	    # the mangled stack pointer.
 	    gdb_test "bt" \
 		[multi_line \
-		     "#0 ${any} in jit_function_00 ${any}" \
+		     "#0 ${any} in jit_function_stack_mangle ${any}" \
 		     "#1 ${any} in main ${any}" \
 		    ] \
 		"bt works"
@@ -161,11 +161,11 @@ proc jit_reader_test {} {
 		info_registers_current_frame $sp_after_mangling
 
 		gdb_test "info frame" \
-		    "Stack level 0, frame at $sp_before_mangling.*in jit_function_00.*"
+		    "Stack level 0, frame at $sp_before_mangling.*in jit_function_stack_mangle.*"
 	    }
 
 	    with_test_prefix "caller frame" {
-		gdb_test "up" "#1  $any in main $any\r\n$any  function $any" \
+		gdb_test "up" "#1  $any in main $any\r\n$any  function_stack_mangle $any" \
 		    "up to caller"
 
 		# Since the JIT unwinder only provides RIP/RSP/RBP,
@@ -243,7 +243,7 @@ proc jit_reader_test {} {
 	# the mangled stack pointer.
 	gdb_test "bt" \
 	    [multi_line \
-		 "#0 ${any} in jit_function_00 ${any}" \
+		 "#0 ${any} in jit_function_stack_mangle ${any}" \
 		 "#1 ${any} in main ${any}" \
 		]
     }
diff --git a/gdb/testsuite/gdb.base/jithost.c b/gdb/testsuite/gdb.base/jithost.c
index 28eb10421987..5b9834ab5359 100644
--- a/gdb/testsuite/gdb.base/jithost.c
+++ b/gdb/testsuite/gdb.base/jithost.c
@@ -31,7 +31,8 @@ void __attribute__((noinline)) __jit_debug_register_code () { }
 struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 };
 struct jit_code_entry only_entry;
 
-typedef void (jit_function_t) ();
+typedef void (jit_function_stack_mangle_t) (void);
+typedef long (jit_function_add_t) (long a, long b);
 
 /* The code of the jit_function_00 function that is copied into an
    mmapped buffer in the inferior at run time.
@@ -39,26 +40,47 @@ typedef void (jit_function_t) ();
    The second instruction mangles the stack pointer, meaning that when
    stopped at the third instruction, GDB needs assistance from the JIT
    unwinder in order to be able to unwind successfully.  */
-const unsigned char jit_function_00_code[] = {
+static const unsigned char jit_function_stack_mangle_code[] = {
   0xcc,				/* int3 */
   0x48, 0x83, 0xf4, 0xff,	/* xor $0xffffffffffffffff, %rsp */
   0x48, 0x83, 0xf4, 0xff,	/* xor $0xffffffffffffffff, %rsp */
   0xc3				/* ret */
 };
 
+/* And another "JIT-ed" function, with the prototype `jit_function_add_t`.  */
+static const unsigned char jit_function_add_code[] = {
+  0x48, 0x01, 0xfe,		/* add %rdi,%rsi */
+  0x48, 0x89, 0xf0,		/* mov %rsi,%rax */
+  0xc3,				/* retq */
+};
+
 int
 main (int argc, char **argv)
 {
-  struct jithost_abi *symfile;
+  struct jithost_abi *symfile = malloc (sizeof (struct jithost_abi));
   char *code = mmap (NULL, getpagesize (), PROT_WRITE | PROT_EXEC,
 		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
-  jit_function_t *function = (jit_function_t *) code;
-
-  memcpy (code, jit_function_00_code, sizeof (jit_function_00_code));
-
-  symfile = malloc (sizeof (struct jithost_abi));
-  symfile->begin = code;
-  symfile->end = code + sizeof (jit_function_00_code);
+  char *code_end = code;
+
+  /* "JIT" function_stack_mangle.  */
+  memcpy (code_end, jit_function_stack_mangle_code,
+	  sizeof (jit_function_stack_mangle_code));
+  jit_function_stack_mangle_t *function_stack_mangle
+    = (jit_function_stack_mangle_t *) code_end;
+  symfile->function_stack_mangle.begin = code_end;
+  code_end += sizeof (jit_function_stack_mangle_code);
+  symfile->function_stack_mangle.end = code_end;
+
+  /* "JIT" function_add.  */
+  memcpy (code_end, jit_function_add_code, sizeof (jit_function_add_code));
+  jit_function_add_t *function_add = (jit_function_add_t *) code_end;
+  symfile->function_add.begin = code_end;
+  code_end += sizeof (jit_function_add_code);
+  symfile->function_add.end = code_end;
+
+  /* Bounds of the whole object.  */
+  symfile->object.begin = code;
+  symfile->object.end = code_end;
 
   only_entry.symfile_addr = symfile;
   only_entry.symfile_size = sizeof (struct jithost_abi);
@@ -69,7 +91,8 @@ main (int argc, char **argv)
   __jit_debug_descriptor.version = 1;
   __jit_debug_register_code ();
 
-  function ();
+  function_stack_mangle ();
+  function_add (5, 6);
 
   return 0;
 }
diff --git a/gdb/testsuite/gdb.base/jithost.h b/gdb/testsuite/gdb.base/jithost.h
index 806593197f26..a01d84605db8 100644
--- a/gdb/testsuite/gdb.base/jithost.h
+++ b/gdb/testsuite/gdb.base/jithost.h
@@ -18,10 +18,21 @@
 #ifndef JITHOST_H
 #define JITHOST_H
 
+struct jithost_abi_bounds
+{
+  const char *begin, *end;
+};
+
 struct jithost_abi
 {
-  const char *begin;
-  const char *end;
+  /* Beginning and past-the-end for the whole object. */
+  struct jithost_abi_bounds object;
+
+  /* Beginning and past-the-end for function_stack_mangle.  */
+  struct jithost_abi_bounds function_stack_mangle;
+
+  /* Beginning and past-the-end for function_add.  */
+  struct jithost_abi_bounds function_add;
 };
 
 #endif /* JITHOST_H */
diff --git a/gdb/testsuite/gdb.base/jitreader.c b/gdb/testsuite/gdb.base/jitreader.c
index 05d53fdbb3dc..6716c5b0f70e 100644
--- a/gdb/testsuite/gdb.base/jitreader.c
+++ b/gdb/testsuite/gdb.base/jitreader.c
@@ -34,8 +34,10 @@ enum register_mapping
 
 struct reader_state
 {
-  uintptr_t code_begin;
-  uintptr_t code_end;
+  struct {
+    uintptr_t begin;
+    uintptr_t end;
+  } func_stack_mangle;
 };
 
 static enum gdb_status
@@ -46,15 +48,24 @@ read_debug_info (struct gdb_reader_funcs *self,
   struct jithost_abi *symfile = memory;
   struct gdb_object *object = cbs->object_open (cbs);
   struct gdb_symtab *symtab = cbs->symtab_open (cbs, object, "");
-  GDB_CORE_ADDR begin = (GDB_CORE_ADDR) symfile->begin;
-  GDB_CORE_ADDR end = (GDB_CORE_ADDR) symfile->end;
+
   struct reader_state *state = (struct reader_state *) self->priv_data;
 
-  /* Record the function's range, for the unwinder.  */
-  state->code_begin = begin;
-  state->code_end = end;
+  /* Record the stack mangle function's range, for the unwinder.  */
+  state->func_stack_mangle.begin
+    = (uintptr_t) symfile->function_stack_mangle.begin;
+  state->func_stack_mangle.end
+    = (uintptr_t) symfile->function_stack_mangle.end;
+
+  cbs->block_open (cbs, symtab, NULL,
+		   (GDB_CORE_ADDR) symfile->function_stack_mangle.begin,
+		   (GDB_CORE_ADDR) symfile->function_stack_mangle.end,
+		   "jit_function_stack_mangle");
 
-  cbs->block_open (cbs, symtab, NULL, begin, end, "jit_function_00");
+  cbs->block_open (cbs, symtab, NULL,
+		   (GDB_CORE_ADDR) symfile->function_add.begin,
+		   (GDB_CORE_ADDR) symfile->function_add.end,
+		   "jit_function_add");
 
   cbs->symtab_close (cbs, symtab);
   cbs->object_close (cbs, object);
@@ -113,7 +124,7 @@ read_sp (struct gdb_reader_funcs *self, struct gdb_unwind_callbacks *cbs,
 
   /* If stopped at the instruction after the "xor $-1, %rsp", demangle
      the stack pointer back.  */
-  if (ip == state->code_begin + 5)
+  if (ip == state->func_stack_mangle.begin + 5)
     sp ^= (uintptr_t) -1;
 
   *value = sp;
@@ -132,7 +143,8 @@ unwind_frame (struct gdb_reader_funcs *self, struct gdb_unwind_callbacks *cbs)
   if (!read_register (cbs, AMD64_RA, &this_ip))
     return GDB_FAIL;
 
-  if (this_ip >= state->code_end || this_ip < state->code_begin)
+  if (this_ip >= state->func_stack_mangle.end
+      || this_ip < state->func_stack_mangle.begin)
     return GDB_FAIL;
 
   /* Unwind RBP in order to make the unwinder that tries to unwind
@@ -168,7 +180,7 @@ get_frame_id (struct gdb_reader_funcs *self, struct gdb_unwind_callbacks *cbs)
   read_register (cbs, AMD64_RA, &ip);
   read_sp (self, cbs, ip, &sp);
 
-  frame_id.code_address = (GDB_CORE_ADDR) state->code_begin;
+  frame_id.code_address = (GDB_CORE_ADDR) state->func_stack_mangle.begin;
   frame_id.stack_address = (GDB_CORE_ADDR) sp;
 
   return frame_id;
-- 
2.24.1

[PATCH v2 5/5] jit: make gdb_symtab::blocks an std::forward_list

[Simon Marchi]

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/jit.c | 137 +++++++++++++++++-------------------------------------
 1 file changed, 43 insertions(+), 94 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index bb6b6bacb5d1..0dd11e14d2f8 100644
--- 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);
 	}
     }
-- 
2.24.1

[PATCH v2 4/5] jit: c++-ify gdb_block

[Simon Marchi]

Add a constructor to gdb_block, change the name field to be a
gdb::unique_xmalloc_ptr.  This is in preparation for using an
std::forward_list<gdb_block> in the next patch.

gdb/ChangeLog:

	* jit.c (struct gdb_block): Add constructor, initialize
	real_block and next fields.
	<name>: Change type to gdb::unique_xmalloc_ptr.
	(struct gdb_symtab) <~gdb_symtab>: Free blocks with delete.
	(jit_block_open_impl): Allocate gdb_block with new.
	(finalize_symtab): Adjust to gdb::unique_xmalloc_ptr.
---
 gdb/jit.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index a731edd870d3..bb6b6bacb5d1 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -429,22 +429,30 @@ jit_read_code_entry (struct gdbarch *gdbarch,
 
 struct gdb_block
 {
+  gdb_block (gdb_block *parent, CORE_ADDR begin, CORE_ADDR end,
+	     const char *name)
+    : parent (parent),
+      begin (begin),
+      end (end),
+      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, *parent;
+  struct gdb_block *next = nullptr, *parent;
 
   /* Points to the "real" block that is being built out of this
      instance.  This block will be added to a blockvector, which will
      then be added to a symtab.  */
-  struct block *real_block;
+  struct block *real_block = nullptr;
 
   /* The first and last code address corresponding to this block.  */
   CORE_ADDR begin, end;
 
   /* The name of this block (if any).  If this is non-NULL, the
      FUNCTION symbol symbol is set to this value.  */
-  const char *name;
+  gdb::unique_xmalloc_ptr<char> name;
 };
 
 /* Proxy object for building a symtab.  */
@@ -465,8 +473,7 @@ struct gdb_symtab
          gdb_block_iter = gdb_block_iter_tmp)
       {
         gdb_block_iter_tmp = gdb_block_iter->next;
-        xfree ((void *) gdb_block_iter->name);
-        xfree (gdb_block_iter);
+	delete gdb_block_iter;
       }
   }
 
@@ -574,13 +581,9 @@ 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 = XCNEW (struct gdb_block);
+  struct gdb_block *block = new gdb_block (parent, begin, end, name);
 
   block->next = symtab->blocks;
-  block->begin = (CORE_ADDR) begin;
-  block->end = (CORE_ADDR) end;
-  block->name = name ? xstrdup (name) : NULL;
-  block->parent = parent;
 
   /* Ensure that the blocks are inserted in the correct (reverse of
      the order expected by blockvector).  */
@@ -721,7 +724,7 @@ 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);
+					 gdb_block_iter->name.get ());
 
       BLOCK_FUNCTION (new_block) = block_name;
 
-- 
2.24.1

Re: [PATCH v2 0/5] Fix and cleanups in jit.c

[Pedro Alves]

On 12/16/19 3:39 AM, Simon Marchi wrote:
> This is a follow-up to:
> 
>   https://sourceware.org/ml/gdb-patches/2019-12/msg00568.html
> 
> Again, the first patch is a fix and the rest is some c++ification.
> 
> I think I fixed all review comments from v1, the biggest change being
> the use of std::forward_list instead of std::vector.

Thanks, LGTM.

Pedro Alves

Re: [PATCH v2 0/5] Fix and cleanups in jit.c

[Simon Marchi]

On 2019-12-16 2:27 p.m., Pedro Alves wrote:
> On 12/16/19 3:39 AM, Simon Marchi wrote:
>> This is a follow-up to:
>>
>>   https://sourceware.org/ml/gdb-patches/2019-12/msg00568.html
>>
>> Again, the first patch is a fix and the rest is some c++ification.
>>
>> I think I fixed all review comments from v1, the biggest change being
>> the use of std::forward_list instead of std::vector.
> 
> Thanks, LGTM.
> 
> Pedro Alves

Thanks, I pushed the series.  Do you think I should push patch 1/5
to the stable branch?  It's not a new bug introduced in the last cycle,
it's been there forever, so I'm not sure what we usually do in these
cases.

Simon

Re: [PATCH v2 0/5] Fix and cleanups in jit.c

[Pedro Alves]

On 12/16/19 11:19 PM, Simon Marchi wrote:
> On 2019-12-16 2:27 p.m., Pedro Alves wrote:
>> On 12/16/19 3:39 AM, Simon Marchi wrote:
>>> This is a follow-up to:
>>>
>>>   https://sourceware.org/ml/gdb-patches/2019-12/msg00568.html
>>>
>>> Again, the first patch is a fix and the rest is some c++ification.
>>>
>>> I think I fixed all review comments from v1, the biggest change being
>>> the use of std::forward_list instead of std::vector.
>>
>> Thanks, LGTM.
>>
>> Pedro Alves
> 
> Thanks, I pushed the series.  Do you think I should push patch 1/5
> to the stable branch?  It's not a new bug introduced in the last cycle,
> it's been there forever, so I'm not sure what we usually do in these
> cases.
Sure, that's fine with me.  Generally it's fine to backport bugfixes
that are supposedly safe, and not invasive, such as this one.

Thanks,
Pedro Alves

Re: [PATCH v2 0/5] Fix and cleanups in jit.c

[Simon Marchi]

On 2019-12-17 2:10 p.m., Pedro Alves wrote:
> On 12/16/19 11:19 PM, Simon Marchi wrote:
>> On 2019-12-16 2:27 p.m., Pedro Alves wrote:
>>> On 12/16/19 3:39 AM, Simon Marchi wrote:
>>>> This is a follow-up to:
>>>>
>>>>   https://sourceware.org/ml/gdb-patches/2019-12/msg00568.html
>>>>
>>>> Again, the first patch is a fix and the rest is some c++ification.
>>>>
>>>> I think I fixed all review comments from v1, the biggest change being
>>>> the use of std::forward_list instead of std::vector.
>>>
>>> Thanks, LGTM.
>>>
>>> Pedro Alves
>>
>> Thanks, I pushed the series.  Do you think I should push patch 1/5
>> to the stable branch?  It's not a new bug introduced in the last cycle,
>> it's been there forever, so I'm not sure what we usually do in these
>> cases.
> Sure, that's fine with me.  Generally it's fine to backport bugfixes
> that are supposedly safe, and not invasive, such as this one.

Ok, thanks, I pushed just that patch to the gdb-9-branch branch.

Simon