[PATCH 0/3] Address review, "fix recent breakage in the JIT reader interface"

[PATCH 0/3] Address review, "fix recent breakage in the JIT reader interface"

[Sanjoy Das]

Hi,

Sorry for hiatus, got a little busy with university exams.  Here are
the fixed patches.

Changes from the last series:

Fix segfault when unwinding JIT frames using a custom reader.
<None>

Make jit-reader-load accept absolute paths to reader shared objects.
 * Fixed nit in IS_ABSOLUTE_PATH invocation.

Add a test case for the jit-reader interface.
 * jit_host_src and jit_host_bin use standard_testfile
 * jit_reader_bin uses standard_output_file instead of a raw ${objdir}

Thanks!
--
Sanjoy

[PATCH 2/3] Make jit-reader-load accept absolute paths to reader shared objects.

[Sanjoy Das]

---
 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 e57e5c1..01ee95d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2012-10-08  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-09-18  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 05ac406..8cdca61 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,7 @@
+2012-10-05  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-09-26  Siddhesh Poyarekar  <siddhesh@redhat.com>
 
 	* observer.texi (memory_changed): Expand parameter LEN to ssize_t.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 5fcbada..c28b803 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33436,15 +33436,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, gdb 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 eff2ed6..fb6658f 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

Re: [PATCH 2/3] Make jit-reader-load accept absolute paths to reader shared objects.

[Eli Zaretskii]

> From: Sanjoy Das <sanjoy@playingwithpointers.com>
> Cc: Sanjoy Das <sanjoy@playingwithpointers.com>
> Date: Mon,  8 Oct 2012 17:17:28 +0530
> 
> +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, gdb will try to load the reader from a pre-configured
                    ^^^
@value{GDB}

The patch to the manual is OK with this change.  Thanks.

Re: [PATCH 2/3] Make jit-reader-load accept absolute paths to reader shared objects.

[Tom Tromey]

>>>>> "Sanjoy" == Sanjoy Das <sanjoy@playingwithpointers.com> writes:

Sanjoy>  2012-10-08  Sanjoy Das <sanjoy@playingwithpointers.com>
 
Sanjoy> +	* jit.c (jit_reader_load_command): Interpret the jit reader name
Sanjoy> +	as an absolute path if it begins with a forward slash.

This is ok, but I think it should wait until the whole series is approved.

Tom

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

[Sanjoy Das]

---
 gdb/testsuite/ChangeLog               |    8 ++
 gdb/testsuite/gdb.base/jit-reader.exp |   72 +++++++++++++++++
 gdb/testsuite/gdb.base/jithost.c      |   67 ++++++++++++++++
 gdb/testsuite/gdb.base/jithost.h      |    9 +++
 gdb/testsuite/gdb.base/jitreader.c    |  136 +++++++++++++++++++++++++++++++++
 5 files changed, 292 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 4eb19bd..02f909e 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,11 @@
+2012-10-08  Sanjoy Das <sanjoy@playingwithpointers.com>
+
+	* gdb.base/jit-reader.exp: New file. Test case for the jit-reader
+	interface.
+	* gdb.base/jithost.c: Do.
+	* gdb.base/jithost.h: Do.
+	* gdb.base/jitreader.c : Do.
+
 2012-10-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	Fix crash during stepping on ppc32.
diff --git a/gdb/testsuite/gdb.base/jit-reader.exp b/gdb/testsuite/gdb.base/jit-reader.exp
new file mode 100644
index 0000000..737d091
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-reader.exp
@@ -0,0 +1,72 @@
+# Copyright 2011-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-*-linux-gnu*] then {
+    untested jit-reader.exp
+    return -1;
+}
+
+if {[skip_shlib_tests]} {
+    untested jit-reader.exp
+    return -1
+}
+
+if {[get_compiler_info]} {
+    warning "Could not get compiler info"
+    untested jit-reader.exp
+    return 1
+}
+
+set jit_host_src ${srcfile}
+set jit_host_bin ${binfile}
+
+set include_dir ${objdir}/../../
+
+if  { [gdb_compile "${srcdir}/${subdir}/${jit_host_src}" "${jit_host_bin}" \
+       executable  [list debug incdir=${include_dir}]] != "" } {
+    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 incdir=${include_dir}]] != "" } {
+    untested jit-reader.exp
+    return -1
+}
+
+proc jit_reader_test {} {
+    global jit_host_bin
+    global jit_reader_bin
+    global verbose
+
+    clean_restart $jit_host_bin
+
+    if {$verbose > 0} {
+	gdb_run_cmd "set debug jit 1"
+    }
+
+    gdb_test_no_output "jit-reader-load ${jit_reader_bin}"
+    gdb_run_cmd "run"
+
+    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..967245c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jithost.c
@@ -0,0 +1,67 @@
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <sys/mman.h>
+
+#include "gdb/jit-reader.h"
+#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; // Generate a 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 =
+    __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..37cf042
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jithost.h
@@ -0,0 +1,9 @@
+#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..2438b0d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jitreader.c
@@ -0,0 +1,136 @@
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "gdb/jit-reader.h"
+#include "jithost.h"
+
+GDB_DECLARE_GPL_COMPATIBLE_READER;
+
+enum RegisterMapping {
+    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;
+}
+
+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;
+
+  printf("Unwinding %p\n", (void *) this_ip);
+
+  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;
+}
+
+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;
+}
+
+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

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

[Tom Tromey]

>>>>> "Sanjoy" == Sanjoy Das <sanjoy@playingwithpointers.com> writes:

Sanjoy> +2012-10-08  Sanjoy Das <sanjoy@playingwithpointers.com>
Sanjoy> +
Sanjoy> +	* gdb.base/jit-reader.exp: New file. Test case for the jit-reader
Sanjoy> +	interface.
Sanjoy> +	* gdb.base/jithost.c: Do.
Sanjoy> +	* gdb.base/jithost.h: Do.
Sanjoy> +	* gdb.base/jitreader.c : Do.

Thanks very much for doing this.

I was concerned that this test should be native-only, but then I noticed
that skip_shlib_tests checks isnative.  So, no worries there.

Sanjoy> +set include_dir ${objdir}/../../

I think this should use standard_output_file.

This patch is ok with this change.

thanks,
Tom

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

[Pedro Alves]

On 10/16/2012 09:15 PM, Tom Tromey wrote:
>>>>>> "Sanjoy" == Sanjoy Das <sanjoy@playingwithpointers.com> writes:
> 
> Sanjoy> +2012-10-08  Sanjoy Das <sanjoy@playingwithpointers.com>
> Sanjoy> +
> Sanjoy> +	* gdb.base/jit-reader.exp: New file. Test case for the jit-reader
> Sanjoy> +	interface.
> Sanjoy> +	* gdb.base/jithost.c: Do.
> Sanjoy> +	* gdb.base/jithost.h: Do.
> Sanjoy> +	* gdb.base/jitreader.c : Do.
> 
> Thanks very much for doing this.
> 
> I was concerned that this test should be native-only, but then I noticed
> that skip_shlib_tests checks isnative.  So, no worries there.

Hmm, confused.  :-)

# Return a 1 if we should skip shared library tests.

proc skip_shlib_tests {} {
    # Run the shared library tests on native systems.
    if {[isnative]} {
        return 0
    }

This means that shlib tests are never skipped on native, but
it doesn't prevent it from running elsewhere.

Should the test be native-only?  If so, why?  Doesn't the jit interface work
when debugging with remote targets (e.g., gdbserver)?

> 
> Sanjoy> +set include_dir ${objdir}/../../
> 
> I think this should use standard_output_file.
> 
> This patch is ok with this change.

-- 
Pedro Alves

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> This means that shlib tests are never skipped on native, but
Pedro> it doesn't prevent it from running elsewhere.

Oh, duh.
Sanjoy, your test has to check isnative.

Pedro> Should the test be native-only?  If so, why?  Doesn't the jit
Pedro> interface work when debugging with remote targets (e.g.,
Pedro> gdbserver)?

It does, but this tests the 'jit-reader-load' functionality, which means
it must build a .so that gdb itself loads.  'isnative' isn't maybe
exactly the right test -- you might have a build-x-host toolchain -- but
it seems simplest.

Tom

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

[Pedro Alves]

On 10/17/2012 05:27 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> This means that shlib tests are never skipped on native, but
> Pedro> it doesn't prevent it from running elsewhere.
> 
> Oh, duh.
> Sanjoy, your test has to check isnative.
> 
> Pedro> Should the test be native-only?  If so, why?  Doesn't the jit
> Pedro> interface work when debugging with remote targets (e.g.,
> Pedro> gdbserver)?
> 
> It does, but this tests the 'jit-reader-load' functionality, which means
> it must build a .so that gdb itself loads.  'isnative' isn't maybe
> exactly the right test -- you might have a build-x-host toolchain -- but
> it seems simplest.

Ah, I see.  We're talking about different "native's".  Dejagnu's isnative
only compares host vs target triplets.  Indeed that sounds simplest.

Bailing if !isnative will still run the tests if testing with a gdbserver
of the triplet as the host (of which native gdbserver testing is a special
case), so I'm happy with that.

(We should still add a gdb_load_shlibs call.)

-- 
Pedro Alves

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

[Pedro Alves]

I've looked at the patch and noticed a few minor issues.

On 10/08/2012 12:47 PM, Sanjoy Das wrote:

> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/jit-reader.exp
> @@ -0,0 +1,72 @@
> +# Copyright 2011-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-*-linux-gnu*] then {

We should move aways from strict istarget checks on x86_64/i686,
due to -m32 or -mx32, or even i686 with -m64.  One possibility is:

if { (![istarget x86_64-*-*] && ![istarget i?86-*-*]) || ![is_lp64_target] } {

But I'm not sure whether the test should run on x32.

> +    untested jit-reader.exp
> +    return -1;
> +}
> +
> +if {[skip_shlib_tests]} {
> +    untested jit-reader.exp

These should be silent.  IOW, remove the untested calls.

> +    return -1
> +}
> +
> +if {[get_compiler_info]} {
> +    warning "Could not get compiler info"
> +    untested jit-reader.exp

From <http://sourceware.org/gdb/wiki/GDBTestcaseCookbook>:

  In untested calls, please spell out the reason the test ends up untested, instead of
  just writing the test name, as with the latter we just end up with the test name
  duplicated in the gdb.sum output. For example:"

> +    return 1
> +}
> +
> +set jit_host_src ${srcfile}
> +set jit_host_bin ${binfile}
> +
> +set include_dir ${objdir}/../../
> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${jit_host_src}" "${jit_host_bin}" \
> +       executable  [list debug incdir=${include_dir}]] != "" } {
> +    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 incdir=${include_dir}]] != "" } {
> +    untested jit-reader.exp
> +    return -1
> +}

Add a call to gdb_load_shlibs, so the .so is uploaded to remote targets.
(unless the test really only should be run on native targets.)

> +
> +proc jit_reader_test {} {
> +    global jit_host_bin
> +    global jit_reader_bin
> +    global verbose
> +
> +    clean_restart $jit_host_bin
> +
> +    if {$verbose > 0} {
> +	gdb_run_cmd "set debug jit 1"
> +    }
> +
> +    gdb_test_no_output "jit-reader-load ${jit_reader_bin}"
> +    gdb_run_cmd "run"
> +
> +    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..967245c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/jithost.c

The test's sources miss copyright headers.

> diff --git a/gdb/testsuite/gdb.base/jitreader.c b/gdb/testsuite/gdb.base/jitreader.c
> new file mode 100644
> index 0000000..2438b0d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/jitreader.c
> @@ -0,0 +1,136 @@
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include "gdb/jit-reader.h"

Is this sure to pick up the build dir's jit-reader.h, or could it trip
on the system's installed copy from the system's gdb version?

I notice that the test's formatting is a bit inconsistent.  It's almost GNU
formatting, but at places it misses the space before parens.  It's not a
strict requirement for tests to follow the GNU conventions, but I think consistency
would be nice.  Issues I noticed (several instances of each):

> +struct jithost_abi {

{ does on next line, indented two spaces.

> +enum gdb_status
> +unwind_frame(struct gdb_reader_funcs* self, struct gdb_unwind_callbacks* cbs)

Missing space before parens.

> +extern struct gdb_reader_funcs* gdb_init_reader(void) {

{ does on next line.

"gdb_reader_funcs *gdb_init_reader"

-- 
Pedro Alves

[PATCH 1/3] Fix segfault when unwinding JIT frames using a custom reader.

[Sanjoy Das]

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 8fe5e27..e57e5c1 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2012-10-08  Sanjoy Das <sanjoy@playingwithpointers.com>
+
+	PR gdb/14550
+
+	* jit.c (finalize_symtab): Ensure that only the global block has a
+	NULL superblock.
+
 2012-10-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	Fix crash during stepping on ppc32.
diff --git a/gdb/jit.c b/gdb/jit.c
index 9e8f295..eff2ed6 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

Re: [PATCH 1/3] Fix segfault when unwinding JIT frames using a custom reader.

[Tom Tromey]

>>>>> "Sanjoy" == Sanjoy Das <sanjoy@playingwithpointers.com> writes:

Sanjoy> +2012-10-08  Sanjoy Das <sanjoy@playingwithpointers.com>
Sanjoy> +
Sanjoy> +	PR gdb/14550
Sanjoy> +
Sanjoy> +	* jit.c (finalize_symtab): Ensure that only the global block has a
Sanjoy> +	NULL superblock.

This is ok, but I think it should wait until the whole series is approved.

Tom

[PATCH 0/3] Address review, "fix recent breakage in the JIT reader interface"

[Sanjoy Das]

Changes from last series:

  Fix segfault when unwinding JIT frames using a custom reader.
    <None>

  Make jit-reader-load accept absolute paths to reader shared objects.
    Fix ${GDBN} mistake pointed out by Eli

  Add a test case for the jit-reader interface.
    Fix issues pointed out by Pedro.

 gdb/ChangeLog                         |   12 +++
 gdb/doc/ChangeLog                     |    4 +
 gdb/doc/gdb.texinfo                   |   22 +++--
 gdb/jit.c                             |   20 ++++-
 gdb/testsuite/ChangeLog               |    8 ++
 gdb/testsuite/gdb.base/jit-reader.exp |   73 +++++++++++++++
 gdb/testsuite/gdb.base/jithost.c      |   84 ++++++++++++++++++
 gdb/testsuite/gdb.base/jithost.h      |   27 ++++++
 gdb/testsuite/gdb.base/jitreader.c    |  156 +++++++++++++++++++++++++++++++++
 9 files changed, 394 insertions(+), 12 deletions(-)
 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

-- 
1.7.10.4

[PATCH 1/3] Fix segfault when unwinding JIT frames using a custom reader.

[Sanjoy Das]

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 f7808a4..0999b30 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2012-11-04  Sanjoy Das <sanjoy@playingwithpointers.com>
+
+	PR gdb/14550
+
+	* jit.c (finalize_symtab): Ensure that only the global block has a
+	NULL superblock.
+
 2012-11-03  Yao Qi  <yao@codesourcery.com>
 
 	Fix PR gdb/14617.
diff --git a/gdb/jit.c b/gdb/jit.c
index 9e8f295..eff2ed6 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

Re: [PATCH 1/3] Fix segfault when unwinding JIT frames using a custom reader.

[Jan Kratochvil]

On Sun, 04 Nov 2012 18:03:49 +0100, Sanjoy Das wrote:
> Issue http://sourceware.org/bugzilla/show_bug.cgi?id=14550
[...]
> --- 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);
> +	}

While I find this patch correct:

(1) The interface for JIT readers was AFAIK designed to be easier than what
    GDB provides internally.  Therefore I think gdb_block->parent should not
    exist, it can be rebuilt from gdb_block->begin and gdb_block->end,
    together with some assumption about gdb_block->next ordering.

(2) Otherwise to keep it ABI compatible one should at least sanity check and
    error if gdb_block->begin and gdb_block->end do not match the ordering via
    gdb_block->parent.



Thanks,
Jan