Re: [PATCH 3/3] Add a test case for the jit-reader interface.

[Sanjoy Das <sanjoy@playingwithpointers.com>]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

> I do not understand "Do.".  Either "Likewise." or also "New file.".

Fixed.

> For GDB repository it is only 2012.

Fixed.

> It executes:
>         run {set debug jit 1}
> Use:
>         gdb_test_no_output "set debug jit 1"

Fixed.

> It executes: (gdb) run run
>
> Besides that it does not wait on the SIGTRAP output it produces, so it is
> racy.  use:
>         gdb_run_cmd
>         gdb_test "" "Program received signal SIGTRAP, .*" "expect SIGTRAP"

Fixed.

> I do not understand why you do not use gdb/jit.h here?  This file is
> a duplicate.  Sorry if it was already discussed during initial JIT review.

I don't think this has been discussed, but my justification is that
we'd want the JIT tests to break if we make ABI incompatible changes
to the structures in jit.h (which, loosely speaking, are part of GDB's
"interface").


As an aside, something seems to have gone wrong with my SSH keys and I
was wondering if someone could commit my work (patches attached) for
me, if that is permissible.

--
Sanjoy Das
http://playingwithpointers.com

[-- Attachment #2: 0001-Fix-segfault-when-unwinding-JIT-frames-using-a-custo.patch --]
[-- Type: application/octet-stream, Size: 1612 bytes --]

From 1cb3dd86455645c89bd51a46fda6bebd18882229 Mon Sep 17 00:00:00 2001
From: Sanjoy Das <sanjoy@playingwithpointers.com>
Date: Thu, 6 Sep 2012 04:52:28 +0530
Subject: [PATCH 1/3] Fix segfault when unwinding JIT frames using a custom
 reader.

Issue http://sourceware.org/bugzilla/show_bug.cgi?id=14550
---
 gdb/ChangeLog |    7 +++++++
 gdb/jit.c     |   14 ++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7f9ca8f..bd74eeb 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2012-12-07  Sanjoy Das <sanjoy@playingwithpointers.com>
+
+	PR gdb/14550
+
+	* jit.c (finalize_symtab): Ensure that only the global block has a
+	NULL superblock.
+
 2012-12-06  Pedro Alves  <palves@redhat.com>
 	    Tom Tromey  <tromey@redhat.com>
 
diff --git a/gdb/jit.c b/gdb/jit.c
index 1afbee6..9817d04 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -724,8 +724,18 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
        gdb_block_iter = gdb_block_iter->next)
     {
       if (gdb_block_iter->parent != NULL)
-        BLOCK_SUPERBLOCK (gdb_block_iter->real_block) =
-          gdb_block_iter->parent->real_block;
+	{
+	  /* If the plugin specifically mentioned a parent block, we
+	     use that.  */
+	  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) =
+	    BLOCKVECTOR_BLOCK (symtab->blockvector, STATIC_BLOCK);
+	}
     }
 
   /* Free memory.  */
-- 
1.7.10.4


[-- Attachment #3: 0002-Make-jit-reader-load-accept-absolute-paths-to-reader.patch --]
[-- Type: application/octet-stream, Size: 3733 bytes --]

From b9d68aa9b1e782c2b7393bbe10cdaf5f83a783e5 Mon Sep 17 00:00:00 2001
From: Sanjoy Das <sanjoy@playingwithpointers.com>
Date: Tue, 18 Sep 2012 08:51:53 +0530
Subject: [PATCH 2/3] Make jit-reader-load accept absolute paths to reader
 shared objects.

---
 gdb/ChangeLog       |    5 +++++
 gdb/doc/ChangeLog   |    4 ++++
 gdb/doc/gdb.texinfo |   22 +++++++++++++---------
 gdb/jit.c           |    6 +++++-
 4 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index bd74eeb..8ecb055 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2012-12-07  Sanjoy Das <sanjoy@playingwithpointers.com>
 
+	* jit.c (jit_reader_load_command): Interpret the jit reader name
+	as an absolute path if it begins with a forward slash.
+
+2012-11-04  Sanjoy Das <sanjoy@playingwithpointers.com>
+
 	PR gdb/14550
 
 	* jit.c (finalize_symtab): Ensure that only the global block has a
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 1f4aa55..762a42e 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,7 @@
+2012-12-07  Sanjoy Das <sanjoy@playingwithpointers.com>
+	* gdb.texinfo (Using JIT Debug Info Readers): Change documentation
+	to reflect that jit-reader-load now supports absolute file-names.
+
 2012-11-29  Tom Tromey  <tromey@redhat.com>
 
 	* gdb.texinfo (SVR4 Process Information): Document missing
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9ffdb77..b950ebd 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33763,15 +33763,19 @@ Readers can be loaded and unloaded using the @code{jit-reader-load}
 and @code{jit-reader-unload} commands.
 
 @table @code
-@item jit-reader-load @var{reader-name}
-Load the JIT reader named @var{reader-name}.  On a UNIX system, this
-will usually load @file{@var{libdir}/gdb/@var{reader-name}}, where
-@var{libdir} is the system library directory, usually
-@file{/usr/local/lib}.  Only one reader can be active at a time;
-trying to load a second reader when one is already loaded will result
-in @value{GDBN} reporting an error.  A new JIT reader can be loaded by
-first unloading the current one using @code{jit-reader-load} and then
-invoking @code{jit-reader-load}.
+@item jit-reader-load @var{reader}
+Load the JIT reader named @var{reader}.  @var{reader} is a shared
+object specified as either an absolute or a relative file name.  In
+the latter case, @value{GDBN} will try to load the reader from a
+pre-configured directory, usually @file{@var{libdir}/gdb/} on a UNIX
+system (here @var{libdir} is the system library directory, often
+@file{/usr/local/lib}).
+
+Only one reader can be active at a time; trying to load a second
+reader when one is already loaded will result in @value{GDBN}
+reporting an error.  A new JIT reader can be loaded by first unloading
+the current one using @code{jit-reader-unload} and then invoking
+@code{jit-reader-load}.
 
 @item jit-reader-unload
 Unload the currently loaded JIT reader.
diff --git a/gdb/jit.c b/gdb/jit.c
index 9817d04..1e5f847 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -25,6 +25,7 @@
 #include "breakpoint.h"
 #include "command.h"
 #include "dictionary.h"
+#include "filenames.h"
 #include "frame-unwind.h"
 #include "gdbcmd.h"
 #include "gdbcore.h"
@@ -208,7 +209,10 @@ jit_reader_load_command (char *args, int from_tty)
   if (loaded_jit_reader != NULL)
     error (_("JIT reader already loaded.  Run jit-reader-unload first."));
 
-  so_name = xstrprintf ("%s/%s", jit_reader_dir, args);
+  if (IS_ABSOLUTE_PATH (args))
+    so_name = xstrdup (args);
+  else
+    so_name = xstrprintf ("%s/%s", jit_reader_dir, args);
   prev_cleanup = make_cleanup (xfree, so_name);
 
   loaded_jit_reader = jit_reader_load (so_name);
-- 
1.7.10.4


[-- Attachment #4: 0003-Add-a-test-case-for-the-jit-reader-interface.patch --]
[-- Type: application/octet-stream, Size: 12186 bytes --]

From 8c81255525d116d49c79eb7c76bba0deb9c1eefb Mon Sep 17 00:00:00 2001
From: Sanjoy Das <sanjoy@playingwithpointers.com>
Date: Tue, 18 Sep 2012 08:52:07 +0530
Subject: [PATCH 3/3] Add a test case for the jit-reader interface.

---
 gdb/testsuite/ChangeLog               |    8 ++
 gdb/testsuite/gdb.base/jit-reader.exp |   80 +++++++++++++++++
 gdb/testsuite/gdb.base/jithost.c      |   84 ++++++++++++++++++
 gdb/testsuite/gdb.base/jithost.h      |   27 ++++++
 gdb/testsuite/gdb.base/jitreader.c    |  156 +++++++++++++++++++++++++++++++++
 5 files changed, 355 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/jit-reader.exp
 create mode 100644 gdb/testsuite/gdb.base/jithost.c
 create mode 100644 gdb/testsuite/gdb.base/jithost.h
 create mode 100644 gdb/testsuite/gdb.base/jitreader.c

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 335ecff..567174f 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,11 @@
+2012-12-07  Sanjoy Das <sanjoy@playingwithpointers.com>
+
+	* gdb.base/jit-reader.exp: New file. Test case for the jit-reader
+	interface.
+	* gdb.base/jithost.c: New file.
+	* gdb.base/jithost.h: New file.
+	* gdb.base/jitreader.c : New file.
+
 2012-12-06  Pedro Alves  <palves@redhat.com>
 	    Tom Tromey  <tromey@redhat.com>
 
diff --git a/gdb/testsuite/gdb.base/jit-reader.exp b/gdb/testsuite/gdb.base/jit-reader.exp
new file mode 100644
index 0000000..5034c3a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-reader.exp
@@ -0,0 +1,80 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile jithost.c
+
+if { (![istarget x86_64-*-*] && ![istarget i?86-*-*]) || ![is_lp64_target] } {
+    return -1;
+}
+
+if {[skip_shlib_tests]} {
+    return -1
+}
+
+if { ![isnative] } {
+    return -1
+}
+
+if {[get_compiler_info]} {
+    untested "could not get compiler info"
+    return 1
+}
+
+set jit_host_src ${srcfile}
+set jit_host_bin ${binfile}
+
+# We inject the complete path to jit-reader.h into the source file
+# lest we end up (incorrectly) building against a system-installed
+# version.
+set jit_reader_header [standard_output_file "../../../gdb/jit-reader.h"]
+set jit_reader_flag "-DJIT_READER_H=\"$jit_reader_header\""
+
+if  { [gdb_compile "${srcdir}/${subdir}/${jit_host_src}" "${jit_host_bin}" \
+       executable  [list debug additional_flags=$jit_reader_flag]] != "" } {
+    untested jit-reader.exp
+    return -1
+}
+
+set jit_reader jitreader
+set jit_reader_src ${jit_reader}.c
+set jit_reader_bin [standard_output_file ${jit_reader}.so]
+
+if { [gdb_compile_shlib "${srcdir}/${subdir}/${jit_reader_src}" "${jit_reader_bin}" \
+	  [list debug additional_flags=$jit_reader_flag]] != "" } {
+    untested jit-reader.exp
+    return -1
+}
+
+gdb_load_shlibs "${jit_reader_bin}"
+
+proc jit_reader_test {} {
+    global jit_host_bin
+    global jit_reader_bin
+    global verbose
+
+    clean_restart $jit_host_bin
+
+    if {$verbose > 0} {
+	gdb_test_no_output "set debug jit 1"
+    }
+
+    gdb_test_no_output "jit-reader-load ${jit_reader_bin}"
+    gdb_run_cmd
+    gdb_test "" "Program received signal SIGTRAP, .*" "expect SIGTRAP"
+
+    gdb_test "bt" "jit_function_00.*"
+}
+
+jit_reader_test
diff --git a/gdb/testsuite/gdb.base/jithost.c b/gdb/testsuite/gdb.base/jithost.c
new file mode 100644
index 0000000..31adb25
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jithost.c
@@ -0,0 +1,84 @@
+/* Copyright (C) 2009-2012 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <sys/mman.h>
+
+#include JIT_READER_H  /* Please see jit-reader.exp for an explanation.  */
+#include "jithost.h"
+
+typedef enum
+{
+  JIT_NOACTION = 0,
+  JIT_REGISTER_FN,
+  JIT_UNREGISTER_FN
+} jit_actions_t;
+
+struct jit_code_entry
+{
+  struct jit_code_entry *next_entry;
+  struct jit_code_entry *prev_entry;
+  void *symfile_addr;
+  uint64_t symfile_size;
+};
+
+struct jit_descriptor
+{
+  uint32_t version;
+  uint32_t action_flag;
+  struct jit_code_entry *relevant_entry;
+  struct jit_code_entry *first_entry;
+};
+
+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) ();
+
+int main (int argc, char **argv)
+{
+  char *code = mmap (NULL, getpagesize (), PROT_WRITE | PROT_EXEC,
+		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  jit_function_t *function = (jit_function_t *) code;
+
+  code[0] = 0xcc; /* SIGTRAP  */
+  code[1] = 0xc3; /* RET  */
+
+  struct jithost_abi *symfile = malloc (sizeof (struct jithost_abi));
+  symfile->begin = code;
+  symfile->end = code + 2;
+
+  only_entry.symfile_addr = symfile;
+  only_entry.symfile_size = sizeof (struct jithost_abi);
+
+  __jit_debug_descriptor.first_entry = &only_entry;
+  __jit_debug_descriptor.relevant_entry = &only_entry;
+  __jit_debug_descriptor.action_flag = JIT_REGISTER_FN;
+  __jit_debug_descriptor.version = 1;
+  __jit_debug_register_code ();
+
+  function ();
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/jithost.h b/gdb/testsuite/gdb.base/jithost.h
new file mode 100644
index 0000000..52ca87a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jithost.h
@@ -0,0 +1,27 @@
+/* Copyright (C) 2009-2012 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef JITHOST_H
+#define JITHOST_H
+
+struct jithost_abi
+{
+  const char *begin;
+  const char *end;
+};
+
+#endif /* JITHOST_H */
diff --git a/gdb/testsuite/gdb.base/jitreader.c b/gdb/testsuite/gdb.base/jitreader.c
new file mode 100644
index 0000000..0c935c4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jitreader.c
@@ -0,0 +1,156 @@
+/* Copyright (C) 2009-2012 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include JIT_READER_H  /* Please see jit-reader.exp for an explanation.  */
+#include "jithost.h"
+
+GDB_DECLARE_GPL_COMPATIBLE_READER;
+
+enum register_mapping
+{
+  AMD64_RA = 16,
+  AMD64_RSP = 7,
+};
+
+struct reader_state
+{
+  uintptr_t code_begin;
+  uintptr_t code_end;
+};
+
+static enum gdb_status
+read_debug_info (struct gdb_reader_funcs *self,
+		 struct gdb_symbol_callbacks *cbs,
+                 void *memory, long memory_sz)
+{
+  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;
+
+  cbs->block_open (cbs, symtab, NULL, begin, end, "jit_function_00");
+
+  cbs->symtab_close (cbs, symtab);
+  cbs->object_close (cbs, object);
+  return GDB_SUCCESS;
+}
+
+static void
+free_reg_value (struct gdb_reg_value *value)
+{
+  free (value);
+}
+
+static void
+write_register (struct gdb_unwind_callbacks *callbacks, int dw_reg,
+                uintptr_t value)
+{
+  const int size = sizeof (uintptr_t);
+  struct gdb_reg_value *reg_val =
+    malloc (sizeof (struct gdb_reg_value) + size - 1);
+  reg_val->defined = 1;
+  reg_val->free = free_reg_value;
+
+  memcpy (reg_val->value, &value, size);
+  callbacks->reg_set (callbacks, dw_reg, reg_val);
+}
+
+static int
+read_register (struct gdb_unwind_callbacks *callbacks, int dw_reg,
+	       uintptr_t *value)
+{
+  const int size = sizeof (uintptr_t);
+  struct gdb_reg_value *reg_val = callbacks->reg_get (callbacks, dw_reg);
+  if (reg_val->size != size || !reg_val->defined)
+    {
+      reg_val->free (reg_val);
+      return 0;
+    }
+  memcpy (value, reg_val->value, size);
+  reg_val->free (reg_val);
+  return 1;
+}
+
+static enum gdb_status
+unwind_frame (struct gdb_reader_funcs *self, struct gdb_unwind_callbacks *cbs)
+{
+  const int word_size = sizeof (uintptr_t);
+  uintptr_t this_sp, this_ip, prev_ip, prev_sp;
+  struct reader_state *state = (struct reader_state *) self->priv_data;
+
+  if (!read_register (cbs, AMD64_RA, &this_ip))
+    return GDB_FAIL;
+
+  if (this_ip >= state->code_end || this_ip < state->code_begin)
+    return GDB_FAIL;
+
+  if (!read_register (cbs, AMD64_RSP, &this_sp))
+    return GDB_FAIL;
+
+  if (cbs->target_read (this_sp, &prev_ip, word_size) == GDB_FAIL)
+    return GDB_FAIL;
+
+  prev_sp = this_sp + word_size;
+  write_register (cbs, AMD64_RA, prev_ip);
+  write_register (cbs, AMD64_RSP, prev_sp);
+  return GDB_SUCCESS;
+}
+
+static struct gdb_frame_id
+get_frame_id (struct gdb_reader_funcs *self, struct gdb_unwind_callbacks *cbs)
+{
+  struct reader_state *state = (struct reader_state *) self->priv_data;
+  struct gdb_frame_id frame_id;
+
+  uintptr_t sp;
+  read_register (cbs, AMD64_RSP, &sp);
+
+  frame_id.code_address = (GDB_CORE_ADDR) state->code_begin;
+  frame_id.stack_address = (GDB_CORE_ADDR) sp;
+
+  return frame_id;
+}
+
+static void
+destroy_reader (struct gdb_reader_funcs *self)
+{
+  free (self->priv_data);
+  free (self);
+}
+
+extern struct gdb_reader_funcs *
+gdb_init_reader (void)
+{
+  struct reader_state *state = malloc (sizeof (struct reader_state));
+  struct gdb_reader_funcs *reader_funcs =
+    malloc (sizeof (struct gdb_reader_funcs));
+
+  reader_funcs->reader_version = GDB_READER_INTERFACE_VERSION;
+  reader_funcs->priv_data = state;
+  reader_funcs->read = read_debug_info;
+  reader_funcs->unwind = unwind_frame;
+  reader_funcs->get_frame_id = get_frame_id;
+  reader_funcs->destroy = destroy_reader;
+
+  return reader_funcs;
+}
-- 
1.7.10.4