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

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

[Simon Marchi]

Patch 1 fixes a bug I found while playing with the jit debug info reader
interface.

The other patches apply some C++ magic to make the code (IMO) easier to
work with.

Simon Marchi (7):
  Fix double-free when creating more than one block in JIT debug info
    reader
  jit: make gdb_object::symtabs a vector
  jit: c++-ify gdb_symtab
  jit: make gdb_symtab::blocks a vector
  jit: make gdb_object::symtabs a vector of unique_ptr
  jit: c++-ify gdb_block
  jit: make gdb_symtab::blocks a vector of unique_ptr

 gdb/jit.c                             | 184 +++++++++-----------------
 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, 139 insertions(+), 153 deletions(-)

-- 
2.24.1

[PATCH 1/7] 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 59da4e0cee1d..f8b12443ba8d 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 3/7] jit: c++-ify gdb_symtab

[Simon Marchi]

This patch makes the gdb_symtab bit more c++y.  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): Call delete on stab.
---
 gdb/jit.c | 63 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 33 insertions(+), 30 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index 672a74044af3..eace83e583d3 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -450,18 +450,37 @@ struct gdb_block
 
 struct gdb_symtab
 {
+  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;
+  std::string file_name;
 };
 
 /* Proxy object for building an object.  */
@@ -511,14 +530,11 @@ 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 ("");
-  object->symtabs.push_back (ret);
-  return ret;
+  gdb_symtab *symtab = new gdb_symtab (file_name);
+  object->symtabs.push_back (symtab);
+  return symtab;
 }
 
 /* Returns true if the block corresponding to old should be placed
@@ -603,7 +619,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++)
     {
@@ -630,7 +646,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;
@@ -639,8 +655,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.  */
@@ -654,8 +670,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)
@@ -756,20 +772,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

Re: [PATCH 3/7] jit: c++-ify gdb_symtab

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> +  gdb_symtab (const char *file_name)
Simon> +    : file_name (file_name != nullptr ? file_name : "")
Simon> +  {}

This should probably be "explicit".

Tom

Re: [PATCH 3/7] jit: c++-ify gdb_symtab

[Simon Marchi]

On 2019-12-13 4:01 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> +  gdb_symtab (const char *file_name)
> Simon> +    : file_name (file_name != nullptr ? file_name : "")
> Simon> +  {}
> 
> This should probably be "explicit".

Good point, I made this change locally.

Simon

[PATCH 6/7] jit: c++-ify gdb_block

[Simon Marchi]

Add a constructor to gdb_block, change the name field to be a
gdb::unique_xmalloc_ptr.

gdb/ChangeLog:

	* jit.c (struct gdb_block): Add constructor, initialize
	real_block field.
	<name): Change type to gdb::unique_xmalloc_ptr.
	(struct gdb_symtab) <~gdb_symtab>: Free blocks with delete.
	(jit_block_open_impl): Use gdb_block constructor.
	(finalize_symtab): Adjust to gdb::unique_xmalloc_ptr.
---
 gdb/jit.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index 1f6de6796e10..56a51f04eb2f 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -428,20 +428,28 @@ 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)
+  {}
+
   /* 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
      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.  */
@@ -455,10 +463,7 @@ struct gdb_symtab
   ~gdb_symtab ()
   {
     for (gdb_block *gdb_block_iter : this->blocks)
-      {
-        xfree ((void *) gdb_block_iter->name);
-        xfree (gdb_block_iter);
-      }
+      delete gdb_block_iter;
   }
 
   /* The list of blocks in this symtab.  These will eventually be
@@ -534,16 +539,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);
-
-  block->begin = (CORE_ADDR) begin;
-  block->end = (CORE_ADDR) end;
-  block->name = name ? xstrdup (name) : NULL;
-  block->parent = parent;
-
   /* Place the block at the end of the vector, it will be sorted when the
      symtab is finalized.  */
-  symtab->blocks.push_back (block);
+  symtab->blocks.push_back (new gdb_block (parent, begin, end, name));
   return symtab->blocks.back ();
 }
 
@@ -665,7 +663,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 6/7] jit: c++-ify gdb_block

[Aktemur, Tankut Baris]

* On Friday, December 13, 2019 7:03 AM, Simon Marchi wrote:
> 
> Add a constructor to gdb_block, change the name field to be a
> gdb::unique_xmalloc_ptr.
> 
> gdb/ChangeLog:
> 
> 	* jit.c (struct gdb_block): Add constructor, initialize
> 	real_block field.
> 	<name): Change type to gdb::unique_xmalloc_ptr.

Minor typo: "<name" -> "<name>"

Regards,
-Baris

> 	(struct gdb_symtab) <~gdb_symtab>: Free blocks with delete.
> 	(jit_block_open_impl): Use gdb_block constructor.
> 	(finalize_symtab): Adjust to gdb::unique_xmalloc_ptr.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 6/7] jit: c++-ify gdb_block

[Simon Marchi]

On 2019-12-13 2:54 a.m., Aktemur, Tankut Baris wrote:
> * On Friday, December 13, 2019 7:03 AM, Simon Marchi wrote:
>>
>> Add a constructor to gdb_block, change the name field to be a
>> gdb::unique_xmalloc_ptr.
>>
>> gdb/ChangeLog:
>>
>> 	* jit.c (struct gdb_block): Add constructor, initialize
>> 	real_block field.
>> 	<name): Change type to gdb::unique_xmalloc_ptr.
> 
> Minor typo: "<name" -> "<name>"

Oh thanks!  I'll fix it locally, while I wait for more feedback.

Simon

Re: [PATCH 6/7] jit: c++-ify gdb_block

[Christian Biesinger via gdb-patches]

On Fri, Dec 13, 2019, 01:04 Simon Marchi <simon.marchi@polymtl.ca> wrote:

> Add a constructor to gdb_block, change the name field to be a
> gdb::unique_xmalloc_ptr.
>
> gdb/ChangeLog:
>
>         * jit.c (struct gdb_block): Add constructor, initialize
>         real_block field.
>         <name): Change type to gdb::unique_xmalloc_ptr.
>         (struct gdb_symtab) <~gdb_symtab>: Free blocks with delete.
>         (jit_block_open_impl): Use gdb_block constructor.
>         (finalize_symtab): Adjust to gdb::unique_xmalloc_ptr.
> ---
>  gdb/jit.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/gdb/jit.c b/gdb/jit.c
> index 1f6de6796e10..56a51f04eb2f 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -428,20 +428,28 @@ 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)
> +  {}
> +
>    /* 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
>       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.  */
> @@ -455,10 +463,7 @@ struct gdb_symtab
>    ~gdb_symtab ()
>    {
>      for (gdb_block *gdb_block_iter : this->blocks)
> -      {
> -        xfree ((void *) gdb_block_iter->name);
> -        xfree (gdb_block_iter);
> -      }
> +      delete gdb_block_iter;
>    }
>

Have you considered making this a vector of unique_ptrs, so you don't have
to manually delete them?


>    /* The list of blocks in this symtab.  These will eventually be
> @@ -534,16 +539,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);
> -
> -  block->begin = (CORE_ADDR) begin;
> -  block->end = (CORE_ADDR) end;
> -  block->name = name ? xstrdup (name) : NULL;
> -  block->parent = parent;
> -
>    /* Place the block at the end of the vector, it will be sorted when the
>       symtab is finalized.  */
> -  symtab->blocks.push_back (block);
> +  symtab->blocks.push_back (new gdb_block (parent, begin, end, name));
>    return symtab->blocks.back ();
>  }
>
> @@ -665,7 +663,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 6/7] jit: c++-ify gdb_block

[Simon Marchi]

On 2019-12-13 10:10 a.m., Christian Biesinger via gdb-patches wrote:
>> @@ -455,10 +463,7 @@ struct gdb_symtab
>>    ~gdb_symtab ()
>>    {
>>      for (gdb_block *gdb_block_iter : this->blocks)
>> -      {
>> -        xfree ((void *) gdb_block_iter->name);
>> -        xfree (gdb_block_iter);
>> -      }
>> +      delete gdb_block_iter;
>>    }
>>
> 
> Have you considered making this a vector of unique_ptrs, so you don't have
> to manually delete them?

Yes, it's done in the following patch.  I went perhaps a bit overboard with the
patch splitting, but I prefer to do these changes in many smaller patches, so that
if something goes wrong, it's easier to identify the culprit.

Simon

Re: [PATCH 6/7] jit: c++-ify gdb_block

[Pedro Alves]

On 12/13/19 3:18 PM, Simon Marchi wrote:
> On 2019-12-13 10:10 a.m., Christian Biesinger via gdb-patches wrote:
>>> @@ -455,10 +463,7 @@ struct gdb_symtab
>>>    ~gdb_symtab ()
>>>    {
>>>      for (gdb_block *gdb_block_iter : this->blocks)
>>> -      {
>>> -        xfree ((void *) gdb_block_iter->name);
>>> -        xfree (gdb_block_iter);
>>> -      }
>>> +      delete gdb_block_iter;
>>>    }
>>>
>>
>> Have you considered making this a vector of unique_ptrs, so you don't have
>> to manually delete them?
> 
> Yes, it's done in the following patch.  I went perhaps a bit overboard with the
> patch splitting, but I prefer to do these changes in many smaller patches, so that
> if something goes wrong, it's easier to identify the culprit.

I wonder whether it wouldn't be simpler to use std::list<object> for these cases.

Thanks,
Pedro Alves

Re: [PATCH 6/7] jit: c++-ify gdb_block

[Simon Marchi]

On 2019-12-13 3:57 p.m., Pedro Alves wrote:
> On 12/13/19 3:18 PM, Simon Marchi wrote:
>> On 2019-12-13 10:10 a.m., Christian Biesinger via gdb-patches wrote:
>>>> @@ -455,10 +463,7 @@ struct gdb_symtab
>>>>    ~gdb_symtab ()
>>>>    {
>>>>      for (gdb_block *gdb_block_iter : this->blocks)
>>>> -      {
>>>> -        xfree ((void *) gdb_block_iter->name);
>>>> -        xfree (gdb_block_iter);
>>>> -      }
>>>> +      delete gdb_block_iter;
>>>>    }
>>>>
>>>
>>> Have you considered making this a vector of unique_ptrs, so you don't have
>>> to manually delete them?
>>
>> Yes, it's done in the following patch.  I went perhaps a bit overboard with the
>> patch splitting, but I prefer to do these changes in many smaller patches, so that
>> if something goes wrong, it's easier to identify the culprit.
> 
> I wonder whether it wouldn't be simpler to use std::list<object> for these cases.

I don't think the code would be much different if use used a list, so I don't expect it
to be much simpler.  If there's a compelling argument for using a list, I can do the
change, but if it's equivalent, I'd rather stick with what is already done.

Simon

Re: [PATCH 6/7] jit: c++-ify gdb_block

[Pedro Alves]

On 12/13/19 9:01 PM, Simon Marchi wrote:
> On 2019-12-13 3:57 p.m., Pedro Alves wrote:
>> I wonder whether it wouldn't be simpler to use std::list<object> for these cases.
> 
> I don't think the code would be much different if use used a list, so I don't expect it
> to be much simpler.  If there's a compelling argument for using a list, I can do the
> change, but if it's equivalent, I'd rather stick with what is already done.
> 

It wouldn't be that much simpler, but simpler regardless, I believe.
Instead of a vector of heap-allocated pointers, with means an extra levels of
indirection and use of a unique_ptr to manage memory, the list would hold
the objects directly, and you'd get address stability encoded in the
data structure.  I don't think changing it would be anything remotely
complicated, since vector/list's apis are quite similar, if not the same
for the uses here.  Seems like a win to me.  But I'm not unhappy with
what you have.

Thanks,
Pedro Alves

Re: [PATCH 6/7] jit: c++-ify gdb_block

[Simon Marchi]

On 2019-12-13 5:20 p.m., Pedro Alves wrote:
> On 12/13/19 9:01 PM, Simon Marchi wrote:
>> On 2019-12-13 3:57 p.m., Pedro Alves wrote:
>>> I wonder whether it wouldn't be simpler to use std::list<object> for these cases.
>>
>> I don't think the code would be much different if use used a list, so I don't expect it
>> to be much simpler.  If there's a compelling argument for using a list, I can do the
>> change, but if it's equivalent, I'd rather stick with what is already done.
>>
> 
> It wouldn't be that much simpler, but simpler regardless, I believe.
> Instead of a vector of heap-allocated pointers, with means an extra levels of
> indirection and use of a unique_ptr to manage memory, the list would hold
> the objects directly, and you'd get address stability encoded in the
> data structure.  I don't think changing it would be anything remotely
> complicated, since vector/list's apis are quite similar, if not the same
> for the uses here.  Seems like a win to me.  But I'm not unhappy with
> what you have.

Ok, I can do a v2 with that.

Simon

[PATCH 7/7] jit: make gdb_symtab::blocks a vector of unique_ptr

[Simon Marchi]

This avoids the need for a destructor at all.

gdb/ChangeLog:

	* jit.c (struct gdb_symtab) <~gdb_symtab>: Remove.
	<blocks>: Change type to
	std::vector<std::unique_ptr<gdb_block>>.
	(jit_block_open_impl): Adjust.
	(finalize_symtab): Adjust.
---
 gdb/jit.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index 56a51f04eb2f..f08ad799a7ea 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -460,15 +460,9 @@ struct gdb_symtab
     : file_name (file_name != nullptr ? file_name : "")
   {}
 
-  ~gdb_symtab ()
-  {
-    for (gdb_block *gdb_block_iter : this->blocks)
-      delete gdb_block_iter;
-  }
-
   /* The list of blocks in this symtab.  These will eventually be
      converted to real blocks.  */
-  std::vector<gdb_block *> blocks;
+  std::vector<std::unique_ptr<gdb_block>> blocks;
 
   /* A mapping between line numbers to PC.  */
   gdb::unique_xmalloc_ptr<struct linetable> linetable;
@@ -541,8 +535,8 @@ jit_block_open_impl (struct gdb_symbol_callbacks *cb,
 {
   /* Place the block at the end of the vector, it will be sorted when the
      symtab is finalized.  */
-  symtab->blocks.push_back (new gdb_block (parent, begin, end, name));
-  return symtab->blocks.back ();
+  symtab->blocks.emplace_back (new gdb_block (parent, begin, end, name));
+  return symtab->blocks.back ().get ();
 }
 
 /* Readers call this to add a line mapping (from PC to line number) to
@@ -596,7 +590,8 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
 
   /* Sort the blocks in the order they should appear in the blockvector. */
   std::sort (stab->blocks.begin (), stab->blocks.end (),
-	     [] (const gdb_block *a, const gdb_block *b)
+	     [] (const std::unique_ptr<gdb_block> &a,
+		 const std::unique_ptr<gdb_block> &b)
 	       {
 		 if (a->begin != b->begin)
 		   return a->begin < b->begin;
@@ -640,7 +635,7 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
      object for each.  Simultaneously, keep setting the real_block
      fields.  */
   int block_idx = FIRST_LOCAL_BLOCK;
-  for (gdb_block *gdb_block_iter : stab->blocks)
+  for (const std::unique_ptr<gdb_block> &gdb_block_iter : stab->blocks)
     {
       struct block *new_block = allocate_block (&objfile->objfile_obstack);
       struct symbol *block_name = allocate_symbol (objfile);
@@ -703,7 +698,7 @@ 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 *gdb_block_iter : stab->blocks)
+  for (const std::unique_ptr<gdb_block> &gdb_block_iter : stab->blocks)
     {
       if (gdb_block_iter->parent != NULL)
 	{
-- 
2.24.1

[PATCH 2/7] jit: make gdb_object::symtabs a vector

[Simon Marchi]

Replace the manual linked list with a vector, simplifying the memory
management.  This requires allocating gdb_object with new and freeing it
with delete.

gdb/ChangeLog:

	* jit.c (struct gdb_symtab) <next>: Remove field.
	(struct gdb_object) <symtabs>: Change type to
	std::vector<gdb_symtab *>.
	(jit_object_open_impl): Allocate gdb_object with new.
	(jit_symtab_open_impl): Adjust to std::vector.
	(jit_object_close_impl):  Adjust to std::vector.
---
 gdb/jit.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index f8b12443ba8d..672a74044af3 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -462,14 +462,13 @@ struct gdb_symtab
 
   /* The source file for this symtab.  */
   const char *file_name;
-  struct gdb_symtab *next;
 };
 
 /* Proxy object for building an object.  */
 
 struct gdb_object
 {
-  struct gdb_symtab *symtabs;
+  std::vector<gdb_symtab *> symtabs;
 };
 
 /* The type of the `private' data passed around by the callback
@@ -501,7 +500,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,
@@ -518,8 +517,7 @@ jit_symtab_open_impl (struct gdb_symbol_callbacks *cb,
 
   ret = XCNEW (struct gdb_symtab);
   ret->file_name = file_name ? xstrdup (file_name) : xstrdup ("");
-  ret->next = object->symtabs;
-  object->symtabs = ret;
+  object->symtabs.push_back (ret);
   return ret;
 }
 
@@ -781,7 +779,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;
 
@@ -791,14 +788,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 5/7] jit: make gdb_object::symtabs a vector of unique_ptr

[Simon Marchi]

This makes the gdb_object hold everything.  When it gets destroyed, all
contained symtabs and contained blocks get destroyed.

gdb/ChangeLog:

	* jit.c (struct gdb_object) <symtabs>: Change type to
	std::vector<std::unique_ptr<gdb_symtab>>.
	(jit_symtab_open_impl): Adjust.
	(jit_object_close_impl): Adjust.
---
 gdb/jit.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index bb855e09f59b..1f6de6796e10 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -476,7 +476,7 @@ struct gdb_symtab
 
 struct gdb_object
 {
-  std::vector<gdb_symtab *> symtabs;
+  std::vector<std::unique_ptr<gdb_symtab>> symtabs;
 };
 
 /* The type of the `private' data passed around by the callback
@@ -521,9 +521,8 @@ jit_symtab_open_impl (struct gdb_symbol_callbacks *cb,
 {
   /* CB stays unused.  See comment in jit_object_open_impl.  */
 
-  gdb_symtab *symtab = new gdb_symtab (file_name);
-  object->symtabs.push_back (symtab);
-  return symtab;
+  object->symtabs.emplace_back (new gdb_symtab (file_name));
+  return object->symtabs.back ().get ();
 }
 
 /* Called by readers to open a new gdb_block.  This function also
@@ -722,8 +721,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
@@ -742,8 +739,8 @@ jit_object_close_impl (struct gdb_symbol_callbacks *cb,
 			   OBJF_NOT_FILENAME);
   objfile->per_bfd->gdbarch = target_gdbarch ();
 
-  for (gdb_symtab *symtab : obj->symtabs)
-    finalize_symtab (symtab, objfile);
+  for (const std::unique_ptr<gdb_symtab> &symtab : obj->symtabs)
+    finalize_symtab (symtab.get (), objfile);
 
   add_objfile_entry (objfile, *priv_data);
 
-- 
2.24.1

Re: [PATCH 5/7] jit: make gdb_object::symtabs a vector of unique_ptr

[Pedro Alves]

On 12/13/19 6:03 AM, Simon Marchi wrote:
>  struct gdb_object
>  {
> -  std::vector<gdb_symtab *> symtabs;
> +  std::vector<std::unique_ptr<gdb_symtab>> symtabs;
>  };

Could this be a vector or objects instead of a vector or pointers?

Like:

 std::vector<gdb_symtab> symtabs;

> +  object->symtabs.emplace_back (new gdb_symtab (file_name));
> +  return object->symtabs.back ().get ();
>  }

and:

 object->symtabs.emplace_back (file_name);
 return &object->symtabs.back ();

Thanks,
Pedro Alves

Re: [PATCH 5/7] jit: make gdb_object::symtabs a vector of unique_ptr

[Simon Marchi]

On 2019-12-13 12:54 p.m., Pedro Alves wrote:
> On 12/13/19 6:03 AM, Simon Marchi wrote:
>>  struct gdb_object
>>  {
>> -  std::vector<gdb_symtab *> symtabs;
>> +  std::vector<std::unique_ptr<gdb_symtab>> symtabs;
>>  };
> 
> Could this be a vector or objects instead of a vector or pointers?
> 
> Like:
> 
>  std::vector<gdb_symtab> symtabs;

I don't think so.  We return these pointers to the JIT debug readers, which then pass
it back to the block_open and symtab_close callbacks.  It's important that the objects
don't move during their lifetime.  If we had a vector of objects, the pointers we return
to the user would get invalidated the moment the vector is resized.

> 
>> +  object->symtabs.emplace_back (new gdb_symtab (file_name));
>> +  return object->symtabs.back ().get ();
>>  }
> 
> and:
> 
>  object->symtabs.emplace_back (file_name);
>  return &object->symtabs.back ();
> 
> Thanks,
> Pedro Alves
> 

Re: [PATCH 5/7] jit: make gdb_object::symtabs a vector of unique_ptr

[Simon Marchi]

On 2019-12-13 1:45 p.m., Simon Marchi wrote:
> I don't think so.  We return these pointers to the JIT debug readers, which then pass
> it back to the block_open and symtab_close callbacks.  It's important that the objects
> don't move during their lifetime.  If we had a vector of objects, the pointers we return
> to the user would get invalidated the moment the vector is resized.

Actually, it would be good to document this, so we don't change it to a vector of
objects, out of good intention.  I would add this to document the gdb_object::symtabs
field:

  /* Symtabs of this object.

     This is a vector of pointers, rather than a vector of objects, 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.  */

I would add a similar comment later in the series to document gdb_symtab::blocks.

Simon

Re: [PATCH 5/7] jit: make gdb_object::symtabs a vector of unique_ptr

[Pedro Alves]

On 12/13/19 6:51 PM, Simon Marchi wrote:
> On 2019-12-13 1:45 p.m., Simon Marchi wrote:
>> I don't think so.  We return these pointers to the JIT debug readers, which then pass
>> it back to the block_open and symtab_close callbacks.  It's important that the objects
>> don't move during their lifetime.  If we had a vector of objects, the pointers we return
>> to the user would get invalidated the moment the vector is resized.
> 
> Actually, it would be good to document this, so we don't change it to a vector of
> objects, out of good intention.  I would add this to document the gdb_object::symtabs
> field:
> 
>   /* Symtabs of this object.
> 
>      This is a vector of pointers, rather than a vector of objects, 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.  */
> 
> I would add a similar comment later in the series to document gdb_symtab::blocks.

That looks good to me.

Thanks,
Pedro Alves

[PATCH 4/7] jit: make gdb_symtab::blocks a vector

[Simon Marchi]

This patch changes the gdb_symtab::blocks linked list to be an
std::vector, simplifying memory management.

Currently, the list is sorted as blocks are created.  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.  It might happen to be true for some nodes, but it can't be true
in general, as this is a simple linked list containing all the created
blocks.

gdb/ChangeLog:

	* jit.c (struct gdb_block) <next>: Remove field.
	(struct gdb_symtab) <~gdb_symtab>: Adjust to std::vector.
	<blocks>: Change type to std::vector<gdb_block *>.
	<nblocks>: Remove.
	(compare_block): Remove.
	(jit_block_open_impl): Adjust to std::vector.  Place the new
	block at the end, don't mind about sorting.
	(finalize_symtab): Adjust to std::vector, sort the blocks vector
	before using it.
---
 gdb/jit.c | 111 +++++++++++++++---------------------------------------
 1 file changed, 31 insertions(+), 80 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index eace83e583d3..bb855e09f59b 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -428,10 +428,8 @@ jit_read_code_entry (struct gdbarch *gdbarch,
 
 struct gdb_block
 {
-  /* 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;
+  /* 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
@@ -456,14 +454,8 @@ struct gdb_symtab
 
   ~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)
+    for (gdb_block *gdb_block_iter : this->blocks)
       {
-        gdb_block_iter_tmp = gdb_block_iter->next;
         xfree ((void *) gdb_block_iter->name);
         xfree (gdb_block_iter);
       }
@@ -471,10 +463,7 @@ struct gdb_symtab
 
   /* The list of blocks in this symtab.  These will eventually be
      converted to real blocks.  */
-  struct gdb_block *blocks = nullptr;
-
-  /* The number of blocks inserted.  */
-  int nblocks = 0;
+  std::vector<gdb_block *> blocks;
 
   /* A mapping between line numbers to PC.  */
   gdb::unique_xmalloc_ptr<struct linetable> linetable;
@@ -537,28 +526,6 @@ jit_symtab_open_impl (struct gdb_symbol_callbacks *cb,
   return symtab;
 }
 
-/* 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.  */
@@ -570,37 +537,15 @@ jit_block_open_impl (struct gdb_symbol_callbacks *cb,
 {
   struct gdb_block *block = XCNEW (struct gdb_block);
 
-  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).  */
-  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;
-            }
-        }
-    }
-  symtab->nblocks++;
-
-  return block;
+  /* Place the block at the end of the vector, it will be sorted when the
+     symtab is finalized.  */
+  symtab->blocks.push_back (block);
+  return symtab->blocks.back ();
 }
 
 /* Readers call this to add a line mapping (from PC to line number) to
@@ -646,14 +591,21 @@ 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->blocks.size ();
+
+  /* Sort the blocks in the order they should appear in the blockvector. */
+  std::sort (stab->blocks.begin (), stab->blocks.end (),
+	     [] (const gdb_block *a, const gdb_block *b)
+	       {
+		 if (a->begin != b->begin)
+		   return a->begin < b->begin;
+
+		 return b->begin < a->begin;
+	       });
 
   cust = allocate_compunit_symtab (objfile, stab->file_name.c_str ());
   allocate_symtab (cust, stab->file_name.c_str ());
@@ -680,19 +632,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);
@@ -719,18 +670,20 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
 
       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;
+
+      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;
 
@@ -753,9 +706,7 @@ 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)
 	{
-- 
2.24.1

Re: [PATCH 4/7] jit: make gdb_symtab::blocks a vector

[Christian Biesinger via gdb-patches]

On Fri, Dec 13, 2019, 01:18 Simon Marchi <simon.marchi@polymtl.ca> wrote:

> This patch changes the gdb_symtab::blocks linked list to be an
> std::vector, simplifying memory management.
>
> Currently, the list is sorted as blocks are created.  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.  It might happen to be true for some nodes, but it can't be true
> in general, as this is a simple linked list containing all the created
> blocks.
>
> gdb/ChangeLog:
>
>         * jit.c (struct gdb_block) <next>: Remove field.
>         (struct gdb_symtab) <~gdb_symtab>: Adjust to std::vector.
>         <blocks>: Change type to std::vector<gdb_block *>.
>         <nblocks>: Remove.
>         (compare_block): Remove.
>         (jit_block_open_impl): Adjust to std::vector.  Place the new
>         block at the end, don't mind about sorting.
>         (finalize_symtab): Adjust to std::vector, sort the blocks vector
>         before using it.
> ---
>  gdb/jit.c | 111 +++++++++++++++---------------------------------------
>  1 file changed, 31 insertions(+), 80 deletions(-)
>
> diff --git a/gdb/jit.c b/gdb/jit.c
> index eace83e583d3..bb855e09f59b 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -428,10 +428,8 @@ jit_read_code_entry (struct gdbarch *gdbarch,
>
>  struct gdb_block
>  {
> -  /* 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;
> +  /* 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
> @@ -456,14 +454,8 @@ struct gdb_symtab
>
>    ~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)
> +    for (gdb_block *gdb_block_iter : this->blocks)
>        {
> -        gdb_block_iter_tmp = gdb_block_iter->next;
>          xfree ((void *) gdb_block_iter->name);
>          xfree (gdb_block_iter);
>        }
> @@ -471,10 +463,7 @@ struct gdb_symtab
>
>    /* The list of blocks in this symtab.  These will eventually be
>       converted to real blocks.  */
> -  struct gdb_block *blocks = nullptr;
> -
> -  /* The number of blocks inserted.  */
> -  int nblocks = 0;
> +  std::vector<gdb_block *> blocks;
>
>    /* A mapping between line numbers to PC.  */
>    gdb::unique_xmalloc_ptr<struct linetable> linetable;
> @@ -537,28 +526,6 @@ jit_symtab_open_impl (struct gdb_symbol_callbacks *cb,
>    return symtab;
>  }
>
> -/* 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.  */
> @@ -570,37 +537,15 @@ jit_block_open_impl (struct gdb_symbol_callbacks *cb,
>  {
>    struct gdb_block *block = XCNEW (struct gdb_block);
>
> -  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).  */
> -  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;
> -            }
> -        }
> -    }
> -  symtab->nblocks++;
> -
> -  return block;
> +  /* Place the block at the end of the vector, it will be sorted when the
> +     symtab is finalized.  */
> +  symtab->blocks.push_back (block);
> +  return symtab->blocks.back ();
>  }
>
>  /* Readers call this to add a line mapping (from PC to line number) to
> @@ -646,14 +591,21 @@ 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->blocks.size ();
> +
> +  /* Sort the blocks in the order they should appear in the blockvector.
> */
> +  std::sort (stab->blocks.begin (), stab->blocks.end (),
> +            [] (const gdb_block *a, const gdb_block *b)
> +              {
> +                if (a->begin != b->begin)
> +                  return a->begin < b->begin;
> +
> +                return b->begin < a->begin;
>

This doesn't look right? Should this look at end or something?

+              });


>    cust = allocate_compunit_symtab (objfile, stab->file_name.c_str ());
>    allocate_symtab (cust, stab->file_name.c_str ());
> @@ -680,19 +632,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);
> @@ -719,18 +670,20 @@ finalize_symtab (struct gdb_symtab *stab, struct
> objfile *objfile)
>
>        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;
> +
> +      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;
>
> @@ -753,9 +706,7 @@ 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)
>         {
> --
> 2.24.1
>
>

Re: [PATCH 4/7] jit: make gdb_symtab::blocks a vector

[Simon Marchi]

On 2019-12-13 10:16 a.m., Christian Biesinger via gdb-patches wrote:
> On Fri, Dec 13, 2019, 01:18 Simon Marchi <simon.marchi@polymtl.ca> wrote:
> 
>> This patch changes the gdb_symtab::blocks linked list to be an
>> std::vector, simplifying memory management.
>>
>> Currently, the list is sorted as blocks are created.  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.  It might happen to be true for some nodes, but it can't be true
>> in general, as this is a simple linked list containing all the created
>> blocks.
>>
>> gdb/ChangeLog:
>>
>>         * jit.c (struct gdb_block) <next>: Remove field.
>>         (struct gdb_symtab) <~gdb_symtab>: Adjust to std::vector.
>>         <blocks>: Change type to std::vector<gdb_block *>.
>>         <nblocks>: Remove.
>>         (compare_block): Remove.
>>         (jit_block_open_impl): Adjust to std::vector.  Place the new
>>         block at the end, don't mind about sorting.
>>         (finalize_symtab): Adjust to std::vector, sort the blocks vector
>>         before using it.
>> ---
>>  gdb/jit.c | 111 +++++++++++++++---------------------------------------
>>  1 file changed, 31 insertions(+), 80 deletions(-)
>>
>> diff --git a/gdb/jit.c b/gdb/jit.c
>> index eace83e583d3..bb855e09f59b 100644
>> --- a/gdb/jit.c
>> +++ b/gdb/jit.c
>> @@ -428,10 +428,8 @@ jit_read_code_entry (struct gdbarch *gdbarch,
>>
>>  struct gdb_block
>>  {
>> -  /* 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;
>> +  /* 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
>> @@ -456,14 +454,8 @@ struct gdb_symtab
>>
>>    ~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)
>> +    for (gdb_block *gdb_block_iter : this->blocks)
>>        {
>> -        gdb_block_iter_tmp = gdb_block_iter->next;
>>          xfree ((void *) gdb_block_iter->name);
>>          xfree (gdb_block_iter);
>>        }
>> @@ -471,10 +463,7 @@ struct gdb_symtab
>>
>>    /* The list of blocks in this symtab.  These will eventually be
>>       converted to real blocks.  */
>> -  struct gdb_block *blocks = nullptr;
>> -
>> -  /* The number of blocks inserted.  */
>> -  int nblocks = 0;
>> +  std::vector<gdb_block *> blocks;
>>
>>    /* A mapping between line numbers to PC.  */
>>    gdb::unique_xmalloc_ptr<struct linetable> linetable;
>> @@ -537,28 +526,6 @@ jit_symtab_open_impl (struct gdb_symbol_callbacks *cb,
>>    return symtab;
>>  }
>>
>> -/* 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.  */
>> @@ -570,37 +537,15 @@ jit_block_open_impl (struct gdb_symbol_callbacks *cb,
>>  {
>>    struct gdb_block *block = XCNEW (struct gdb_block);
>>
>> -  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).  */
>> -  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;
>> -            }
>> -        }
>> -    }
>> -  symtab->nblocks++;
>> -
>> -  return block;
>> +  /* Place the block at the end of the vector, it will be sorted when the
>> +     symtab is finalized.  */
>> +  symtab->blocks.push_back (block);
>> +  return symtab->blocks.back ();
>>  }
>>
>>  /* Readers call this to add a line mapping (from PC to line number) to
>> @@ -646,14 +591,21 @@ 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->blocks.size ();
>> +
>> +  /* Sort the blocks in the order they should appear in the blockvector.
>> */
>> +  std::sort (stab->blocks.begin (), stab->blocks.end (),
>> +            [] (const gdb_block *a, const gdb_block *b)
>> +              {
>> +                if (a->begin != b->begin)
>> +                  return a->begin < b->begin;
>> +
>> +                return b->begin < a->begin;
>>
> 
> This doesn't look right? Should this look at end or something?

Oh my, indeed, thank you so much for spotting this.  I meant this, of course:

  std::sort (stab->blocks.begin (), stab->blocks.end (),
	     [] (const gdb_block *a, const gdb_block *b)
	       {
		 if (a->begin != b->begin)
		   return a->begin < b->begin;

		 return b->end < a->end;
	       });

Or do you find it more readable this way below instead?  It's a bit subtle that "a"
and "b" are reversed, otherwise

  std::sort (stab->blocks.begin (), stab->blocks.end (),
	     [] (const gdb_block *a, const gdb_block *b)
	       {
		 if (a->begin != b->begin)
		   return a->begin < b->begin;

		 return a->end > b->end;
	       });

Simon

Re: [PATCH 4/7] jit: make gdb_symtab::blocks a vector

[Christian Biesinger via gdb-patches]

On Fri, Dec 13, 2019, 11:02 Simon Marchi <simon.marchi@polymtl.ca> wrote:

> On 2019-12-13 10:16 a.m., Christian Biesinger via gdb-patches wrote:
> > On Fri, Dec 13, 2019, 01:18 Simon Marchi <simon.marchi@polymtl.ca>
> wrote:
> >
> >> This patch changes the gdb_symtab::blocks linked list to be an
> >> std::vector, simplifying memory management.
> >>
> >> Currently, the list is sorted as blocks are created.  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.  It might happen to be true for some nodes, but it can't be true
> >> in general, as this is a simple linked list containing all the created
> >> blocks.
> >>
> >> gdb/ChangeLog:
> >>
> >>         * jit.c (struct gdb_block) <next>: Remove field.
> >>         (struct gdb_symtab) <~gdb_symtab>: Adjust to std::vector.
> >>         <blocks>: Change type to std::vector<gdb_block *>.
> >>         <nblocks>: Remove.
> >>         (compare_block): Remove.
> >>         (jit_block_open_impl): Adjust to std::vector.  Place the new
> >>         block at the end, don't mind about sorting.
> >>         (finalize_symtab): Adjust to std::vector, sort the blocks vector
> >>         before using it.
> >> ---
> >>  gdb/jit.c | 111 +++++++++++++++---------------------------------------
> >>  1 file changed, 31 insertions(+), 80 deletions(-)
> >>
> >> diff --git a/gdb/jit.c b/gdb/jit.c
> >> index eace83e583d3..bb855e09f59b 100644
> >> --- a/gdb/jit.c
> >> +++ b/gdb/jit.c
> >> @@ -428,10 +428,8 @@ jit_read_code_entry (struct gdbarch *gdbarch,
> >>
> >>  struct gdb_block
> >>  {
> >> -  /* 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;
> >> +  /* 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
> >> @@ -456,14 +454,8 @@ struct gdb_symtab
> >>
> >>    ~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)
> >> +    for (gdb_block *gdb_block_iter : this->blocks)
> >>        {
> >> -        gdb_block_iter_tmp = gdb_block_iter->next;
> >>          xfree ((void *) gdb_block_iter->name);
> >>          xfree (gdb_block_iter);
> >>        }
> >> @@ -471,10 +463,7 @@ struct gdb_symtab
> >>
> >>    /* The list of blocks in this symtab.  These will eventually be
> >>       converted to real blocks.  */
> >> -  struct gdb_block *blocks = nullptr;
> >> -
> >> -  /* The number of blocks inserted.  */
> >> -  int nblocks = 0;
> >> +  std::vector<gdb_block *> blocks;
> >>
> >>    /* A mapping between line numbers to PC.  */
> >>    gdb::unique_xmalloc_ptr<struct linetable> linetable;
> >> @@ -537,28 +526,6 @@ jit_symtab_open_impl (struct gdb_symbol_callbacks
> *cb,
> >>    return symtab;
> >>  }
> >>
> >> -/* 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.  */
> >> @@ -570,37 +537,15 @@ jit_block_open_impl (struct gdb_symbol_callbacks
> *cb,
> >>  {
> >>    struct gdb_block *block = XCNEW (struct gdb_block);
> >>
> >> -  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).  */
> >> -  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;
> >> -            }
> >> -        }
> >> -    }
> >> -  symtab->nblocks++;
> >> -
> >> -  return block;
> >> +  /* Place the block at the end of the vector, it will be sorted when
> the
> >> +     symtab is finalized.  */
> >> +  symtab->blocks.push_back (block);
> >> +  return symtab->blocks.back ();
> >>  }
> >>
> >>  /* Readers call this to add a line mapping (from PC to line number) to
> >> @@ -646,14 +591,21 @@ 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->blocks.size ();
> >> +
> >> +  /* Sort the blocks in the order they should appear in the
> blockvector.
> >> */
> >> +  std::sort (stab->blocks.begin (), stab->blocks.end (),
> >> +            [] (const gdb_block *a, const gdb_block *b)
> >> +              {
> >> +                if (a->begin != b->begin)
> >> +                  return a->begin < b->begin;
> >> +
> >> +                return b->begin < a->begin;
> >>
> >
> > This doesn't look right? Should this look at end or something?
>
> Oh my, indeed, thank you so much for spotting this.  I meant this, of
> course:
>
>   std::sort (stab->blocks.begin (), stab->blocks.end (),
>              [] (const gdb_block *a, const gdb_block *b)
>                {
>                  if (a->begin != b->begin)
>                    return a->begin < b->begin;
>
>                  return b->end < a->end;
>                });
>
> Or do you find it more readable this way below instead?  It's a bit subtle
> that "a"
> and "b" are reversed, otherwise
>
>   std::sort (stab->blocks.begin (), stab->blocks.end (),
>              [] (const gdb_block *a, const gdb_block *b)
>                {
>                  if (a->begin != b->begin)
>                    return a->begin < b->begin;
>
>                  return a->end > b->end;
>                });
>

I personally prefer this one. And maybe add a comment that the block that
encloses the other one should come first?


> Simon
>

Re: [PATCH 4/7] jit: make gdb_symtab::blocks a vector

[Simon Marchi]

On 2019-12-13 11:08 a.m., Christian Biesinger via gdb-patches wrote:
> I personally prefer this one. And maybe add a comment that the block that
> encloses the other one should come first?

The order in which blocks appear in a blockvector is already explained in the comment on
top of struct block, in block.h, so I didn't want to duplicate it.  But I can certainly
point to it explicitly in the comment above the std::sort call:

  /* Sort the blocks in the order they should appear in the blockvector.

     See the comment on top of struct block, in block.h, for more details.  */

Does that sound good?

Re: [PATCH 4/7] jit: make gdb_symtab::blocks a vector

[Christian Biesinger via gdb-patches]

On Fri, Dec 13, 2019, 11:14 Simon Marchi <simon.marchi@polymtl.ca> wrote:

> On 2019-12-13 11:08 a.m., Christian Biesinger via gdb-patches wrote:
> > I personally prefer this one. And maybe add a comment that the block that
> > encloses the other one should come first?
>
> The order in which blocks appear in a blockvector is already explained in
> the comment on
> top of struct block, in block.h, so I didn't want to duplicate it.  But I
> can certainly
> point to it explicitly in the comment above the std::sort call:
>
>   /* Sort the blocks in the order they should appear in the blockvector.
>
>      See the comment on top of struct block, in block.h, for more
> details.  */
>
> Does that sound good?
>

Sounds good to me!

>

Re: [PATCH 4/7] jit: make gdb_symtab::blocks a vector

[Pedro Alves]

On 12/13/19 6:03 AM, Simon Marchi wrote:
> I don't think it's true that "next" points to the next node at the same
> depth.  It might happen to be true for some nodes, but it can't be true
> in general, as this is a simple linked list containing all the created
> blocks.

Is this really true?  I mean, the comment you're removing talks about
a tree.  I see that jit_block_open_impl starts by doing:

  block->next = symtab->blocks;

but then the else branch writes to blocl->next again:

          /* Guaranteed to terminate, since compare_block (NULL, _)
             returns 1.  */
          if (compare_block (i->next, block))
            {
              block->next = i->next;

I don't pretend to understand this code, but it does sound like at
least the intention was to have a tree of blocks, which is not
a surprising data structure for blocks.

Thanks,
Pedro Alves

Re: [PATCH 4/7] jit: make gdb_symtab::blocks a vector

[Simon Marchi]

On 2019-12-13 5:14 p.m., Pedro Alves wrote:
> On 12/13/19 6:03 AM, Simon Marchi wrote:
>> I don't think it's true that "next" points to the next node at the same
>> depth.  It might happen to be true for some nodes, but it can't be true
>> in general, as this is a simple linked list containing all the created
>> blocks.
> 
> Is this really true?  I mean, the comment you're removing talks about
> a tree.  I see that jit_block_open_impl starts by doing:
> 
>   block->next = symtab->blocks;
> 
> but then the else branch writes to blocl->next again:
> 
>           /* Guaranteed to terminate, since compare_block (NULL, _)
>              returns 1.  */
>           if (compare_block (i->next, block))
>             {
>               block->next = i->next;
> 
> I don't pretend to understand this code, but it does sound like at
> least the intention was to have a tree of blocks, which is not
> a surprising data structure for blocks.

Well, the code builds a singly linked list and takes care of keeping it
sorted in the reverse order of the expected order for a blockvector
But that doesn't make it a tree.

If the JIT generates this code, conceptually:

{ // A
  { // B
  }
  { // C
  }
}

It could be represented by this tree:

   A
  / \
 B   C

The created linked list will be:

C -> B -> A

Node B points to node A, which doesn't match the comment "Next points to the
next node at the same depth as this block".

Maybe the original intention of the author was to build an actual tree, where
each node has a singly linked list of its children, in which case it would have
been true, but then changed the implementation to use a single linked list in
reversed blockvector order.

Simon

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

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Patch 1 fixes a bug I found while playing with the jit debug info reader
Simon> interface.

Simon> The other patches apply some C++ magic to make the code (IMO) easier to
Simon> work with.

I read through these.  I just had one comment but I also agree with the
comments from the other reviews -- I just didn't have anything to add there.

thanks,
Tom