[patch,7.3] Fix JIT clang-lli gdb-7.3 regression Re: [gdb-7.3] Error in gdb-llvm integration: Unable to read JIT descriptor from remote memory!

[patch,7.3] Fix JIT clang-lli gdb-7.3 regression Re: [gdb-7.3] Error in gdb-llvm integration: Unable to read JIT descriptor from remote memory!

[Jan Kratochvil]

On Sun, 03 Jul 2011 01:36:45 +0200, Yuri wrote:
> I tried the upcoming gdb-7.3 and the message "Unable to read JIT descriptor
> from remote memory!" is back when llvm::JITEmitDebugInfo is set when
> libLLVM-3.0.so is loaded dynamically.
> gdb-7.1 was broken this way, gdb-7.2 works, now gdb-7.3 code is broken
> again.

739571cc3151651f49f7171cfd98275d983bfaaa is the first bad commit
commit 739571cc3151651f49f7171cfd98275d983bfaaa
Author: Paul Pluzhnikov <ppluzhnikov@google.com>
Date:   Mon Jan 31 21:37:00 2011 +0000
Re: [patch] Fix leak of bp_jit_event breakpoints
http://sourceware.org/ml/gdb-patches/2011-01/msg00556.html

I expect Paul had /usr/bin/lli linked statically; /usr/bin/lli uses
libLLVM-2.8.so at least on Fedora 15.

No regressions on {x86_64,x86_64-m32,i686}-fedora15-linux-gnu.

I have mostly reverted that part of the Paul's patch, there is no "reduced"
reinitialization on a new objfile load as it can be the first successful
initialization at all.  Unaware how it affects performance in more complicated
cases but the Paul's breakpoints multiplication should not be regressed.

I will be away tomorrow (+ maybe partially the day after).

THe testcase is far from perfect as it cannot run cross-arch.  I have only
native build of clang here, not sure if it complies with the same arch-prefix
rules as GCC so that dejagnu should be extended for clang as for an additional
compiler kind.


Thanks,
Jan


gdb/
2011-07-04  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix JIT registration on dynamically loaded JIT enginers.
	* jit.c (registering_code): New.
	(jit_register_code): Protect symbol_file_add_from_bfd by it.
	(jit_breakpoint_re_set_internal): Inline into ...
	(jit_inferior_init): ... here.  Display REGISTERING_CODE in the
	JIT_DEBUG message.  Return on REGISTERING_CODE.
	(jit_breakpoint_re_set): Call jit_inferior_init instead of
	jit_breakpoint_re_set_internal.

gdb/testsuite/
2011-07-04  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix JIT registration on dynamically loaded JIT enginers.
	* gdb.base/jit-clang.c: New.
	* gdb.base/jit-clang.exp: New.

--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -38,6 +38,15 @@ static const char *const jit_break_name = "__jit_debug_register_code";
 
 static const char *const jit_descriptor_name = "__jit_debug_descriptor";
 
+/* This is a boolean indicating whether we're currently registering code.  This
+   is used to avoid re-entering the registration code.  We want to check for
+   new JITed every time a new object file is loaded, but we want to avoid
+   checking for new code while we're registering object files for JITed code.
+   Therefore, we flip this variable to 1 before registering new object files,
+   and set it to 0 before returning.  */
+
+static int registering_code = 0;
+
 static const struct inferior_data *jit_inferior_data = NULL;
 
 /* Non-zero if we want to see trace of jit level stuff.  */
@@ -295,9 +304,18 @@ JITed symbol file is not an object file, ignoring it.\n"));
         ++i;
       }
 
+  /* Raise this flag while we register code so we won't trigger any
+     re-registration.  */
+  gdb_assert (registering_code == 0);
+  my_cleanups = make_cleanup_restore_integer (&registering_code);
+  registering_code = 1;
+
   /* This call takes ownership of sai.  */
   objfile = symbol_file_add_from_bfd (nbfd, 0, sai, OBJF_SHARED, NULL);
 
+  /* Clear the registering_code flag.  */
+  do_cleanups (my_cleanups);
+
   /* Remember a mapping from entry_addr to objfile.  */
   entry_addr_ptr = xmalloc (sizeof (CORE_ADDR));
   *entry_addr_ptr = entry_addr;
@@ -332,13 +350,29 @@ jit_find_objf_with_entry_addr (CORE_ADDR entry_addr)
   return NULL;
 }
 
-/* (Re-)Initialize the jit breakpoint if necessary.
-   Return 0 on success.  */
+/* Register any already created translations.  */
 
-static int
-jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
-				struct jit_inferior_data *inf_data)
+static void
+jit_inferior_init (struct gdbarch *gdbarch)
 {
+  struct jit_descriptor descriptor;
+  struct jit_code_entry cur_entry;
+  struct jit_inferior_data *inf_data;
+  CORE_ADDR cur_entry_addr;
+
+  if (jit_debug)
+    fprintf_unfiltered (gdb_stdlog,
+		       "jit_inferior_init, registering_code = %d\n",
+		       registering_code);
+
+  /* When we register code, GDB resets its breakpoints in case symbols have
+     changed.  That in turn calls this handler, which makes us look for new
+     code again.  To avoid being re-entered, we check this flag.  */
+  if (registering_code)
+    return;
+
+  inf_data = get_jit_inferior_data ();
+
   if (inf_data->breakpoint_addr == 0)
     {
       struct minimal_symbol *reg_symbol;
@@ -347,42 +381,19 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
 	 we are not attached to a JIT.  */
       reg_symbol = lookup_minimal_symbol (jit_break_name, NULL, NULL);
       if (reg_symbol == NULL)
-	return 1;
+	return;
       inf_data->breakpoint_addr = SYMBOL_VALUE_ADDRESS (reg_symbol);
       if (inf_data->breakpoint_addr == 0)
-	return 2;
-    }
-  else
-    return 0;
-
-  if (jit_debug)
-    fprintf_unfiltered (gdb_stdlog,
-			"jit_breakpoint_re_set_internal, "
-			"breakpoint_addr = %s\n",
-			paddress (gdbarch, inf_data->breakpoint_addr));
-
-  /* Put a breakpoint in the registration symbol.  */
-  create_jit_event_breakpoint (gdbarch, inf_data->breakpoint_addr);
-
-  return 0;
-}
-
-/* Register any already created translations.  */
-
-static void
-jit_inferior_init (struct gdbarch *gdbarch)
-{
-  struct jit_descriptor descriptor;
-  struct jit_code_entry cur_entry;
-  struct jit_inferior_data *inf_data;
-  CORE_ADDR cur_entry_addr;
+	return;
 
-  if (jit_debug)
-    fprintf_unfiltered (gdb_stdlog, "jit_inferior_init\n");
+      if (jit_debug)
+	fprintf_unfiltered (gdb_stdlog,
+			    "jit_inferior_init, breakpoint_addr = %s\n",
+			    paddress (gdbarch, inf_data->breakpoint_addr));
 
-  inf_data = get_jit_inferior_data ();
-  if (jit_breakpoint_re_set_internal (gdbarch, inf_data) != 0)
-    return;
+      /* Put a breakpoint in the registration symbol.  */
+      create_jit_event_breakpoint (gdbarch, inf_data->breakpoint_addr);
+    }
 
   if (inf_data->descriptor_addr == 0)
     {
@@ -443,8 +456,7 @@ jit_inferior_created_hook (void)
 void
 jit_breakpoint_re_set (void)
 {
-  jit_breakpoint_re_set_internal (target_gdbarch,
-				  get_jit_inferior_data ());
+  jit_inferior_init (target_gdbarch);
 }
 
 /* Reset inferior_data, so sybols will be looked up again, and jit_breakpoint
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-clang.c
@@ -0,0 +1,30 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 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/>.  */
+
+#include <stdlib.h>
+
+static void
+jit_clang_function_name (void)
+{
+  abort ();
+}
+
+int
+main (void)
+{
+  jit_clang_function_name ();
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-clang.exp
@@ -0,0 +1,52 @@
+# Copyright (C) 2011 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/>.
+
+# gdbserver configuration does not support passing arguments to `lli'.
+if { ![isnative] || [is_remote target] } {
+    return
+}
+
+set testfile "jit-clang"
+set srcfile ${testfile}.c
+set binfile $objdir/$subdir/${testfile}.bc
+
+if {[catch "system \"clang -fexceptions $srcdir/$subdir/$srcfile -c -emit-llvm -o ${binfile}\""] != 0} {
+    verbose -log "clang compilation failed"
+    untested ${testfile}.exp
+    return -1
+}
+if {[catch "system \"which lli &>/dev/null\""] != 0} {
+    verbose -log "lli not found"
+    untested ${testfile}.exp
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+if [gdb_load "lli"] {
+    untested ${testfile}.exp
+    return -1
+}
+
+gdb_test_no_output "set args -jit-emit-debug ${binfile}" "set args"
+gdb_run_cmd
+
+# The error was:
+# Unable to read JIT descriptor from remote memory!
+gdb_test "" "\r\nProgram received signal SIGABRT.*" "run"
+
+gdb_test "bt" " jit_clang_function_name .*" "jit registration succeeded"

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression Re: [gdb-7.3] Error in gdb-llvm integration: Unable to read JIT descriptor from remote memory!

[Paul Pluzhnikov]

On Mon, Jul 4, 2011 at 2:46 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Sun, 03 Jul 2011 01:36:45 +0200, Yuri wrote:
>> I tried the upcoming gdb-7.3 and the message "Unable to read JIT descriptor
>> from remote memory!" is back when llvm::JITEmitDebugInfo is set when
>> libLLVM-3.0.so is loaded dynamically.
>> gdb-7.1 was broken this way, gdb-7.2 works, now gdb-7.3 code is broken
>> again.

Sorry about the breakage ...

> 739571cc3151651f49f7171cfd98275d983bfaaa is the first bad commit
> commit 739571cc3151651f49f7171cfd98275d983bfaaa
> Author: Paul Pluzhnikov <ppluzhnikov@google.com>
> Date:   Mon Jan 31 21:37:00 2011 +0000
> Re: [patch] Fix leak of bp_jit_event breakpoints
> http://sourceware.org/ml/gdb-patches/2011-01/msg00556.html
>
> I expect Paul had /usr/bin/lli linked statically; /usr/bin/lli uses
> libLLVM-2.8.so at least on Fedora 15.

Rather, I didn't test with lli (I don't even know what it is).

> I have mostly reverted that part of the Paul's patch

FWIW, I don't believe reverting the patch is the right fix. Certainly
using a single global to guard against recursive invocation can not
possibly work correctly in multi-inferior GDB.

> there is no "reduced"
> reinitialization on a new objfile load as it can be the first successful
> initialization at all.

It's been a while since I looked at that code, but it sounds to me
that we should keep the jit_inferior_data per-objfile, and not
per-inferior. This might even allow GDB to work with multiple JITers
in the same inferior ;-)

I'll see if I can work up a patch tomorrow.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression

[Jan Kratochvil]

On Tue, 05 Jul 2011 00:20:08 +0200, Paul Pluzhnikov wrote:
> Certainly using a single global to guard against recursive invocation can
> not possibly work correctly in multi-inferior GDB.

I was coding it into inf_data first but then changed it as I do not see how it
could harm as a single global.  But I am not much multi-inferior experienced
so I could miss something.


> we should keep the jit_inferior_data per-objfile, and not
> per-inferior. This might even allow GDB to work with multiple JITers
> in the same inferior ;-)

That's true and fine for 7.4 but that is an extension and not a 7.3 late
release cycle regression/blocker.


Thanks,
Jan

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression Re: [gdb-7.3] Error in gdb-llvm integration: Unable to read JIT descriptor from remote memory!

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

>> I have mostly reverted that part of the Paul's patch

Paul> FWIW, I don't believe reverting the patch is the right fix. Certainly
Paul> using a single global to guard against recursive invocation can not
Paul> possibly work correctly in multi-inferior GDB.

I think it is ok in this particular instance, because the global is used
to guard just the call to symbol_file_add_from_bfd.

I don't have an opinion about the other parts of the discussion.

Tom

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression Re: [gdb-7.3] Error in gdb-llvm integration: Unable to read JIT descriptor from remote memory!

[Joel Brobecker]

Jan, Paul,

We're getting ready to produce the first gdb-7.3 candidate release,
and we're thinking of either:
  - going with Jan's patch for now, with a view of possibly
    making it better with a followup patch if necessary;
  - releasing 7.3 with JIT as a known problem, hopefully to be
    fixed for 7.3.1, depending on how safe the fix is perceived.

Either way, we'd like to issue the pre-release either tomorrow,
of Thu if Jan doesn't come back tomorrow.  And if all goes well,
and no blocking bug is found, the 7.3 official release would be
created 2 weeks later.

-- 
Joel

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression Re: [gdb-7.3] Error in gdb-llvm integration: Unable to read JIT descriptor from remote memory!

[Paul Pluzhnikov]

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

On Tue, Jul 5, 2011 at 10:07 AM, Joel Brobecker <brobecker@adacore.com> wrote:

> We're getting ready to produce the first gdb-7.3 candidate release,
> and we're thinking of either:
>  - going with Jan's patch for now, with a view of possibly
>    making it better with a followup patch if necessary;
>  - releasing 7.3 with JIT as a known problem, hopefully to be
>    fixed for 7.3.1, depending on how safe the fix is perceived.

Another option is to apply a better fix (provided it is deemed safe) ...

Certainly it's ok with me if you decide to go with Jan's patch for now.

It took me  much longer to create a test case (I don't have 'lli'), then
it did to fix the problem once I had the repro.

Thanks,
-- 
Paul Pluzhnikov


2011-07-05  Paul Pluzhnikov  <ppluzhnikov@google.com>

	jit.c (jit_breakpoint_re_set_internal): Call jit_inferior_init.


testsuite/ChangeLog:

2011-07-05  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* gdb.base/jit-so.exp: New test.
	* gdb.base/jit-dlmain.c: New file.
	* gdb.base/jit-main.c: Allow "main" to be elsewhere.

[-- Attachment #2: gdb-jit-20110705.txt --]
[-- Type: text/plain, Size: 6978 bytes --]

Index: jit.c
===================================================================
RCS file: /cvs/src/src/gdb/jit.c,v
retrieving revision 1.12
diff -u -p -n -p -r1.12 jit.c
*** jit.c	17 Apr 2011 18:38:45 -0000	1.12
--- jit.c	5 Jul 2011 20:58:12 -0000
*************** static const char *const jit_descriptor_
*** 40,45 ****
--- 40,48 ----
  
  static const struct inferior_data *jit_inferior_data = NULL;
  
+ static void
+ jit_inferior_init (struct gdbarch *gdbarch);
+ 
  /* Non-zero if we want to see trace of jit level stuff.  */
  
  static int jit_debug = 0;
*************** jit_breakpoint_re_set_internal (struct g
*** 351,356 ****
--- 354,364 ----
        inf_data->breakpoint_addr = SYMBOL_VALUE_ADDRESS (reg_symbol);
        if (inf_data->breakpoint_addr == 0)
  	return 2;
+ 
+       /* If we have not read the jit descriptor yet (e.g. because the JITer
+ 	 itself is in a shared library which just got loaded), do so now.  */
+       if (inf_data->descriptor_addr == 0)
+ 	jit_inferior_init (gdbarch);
      }
    else
      return 0;
Index: testsuite/gdb.base/jit-dlmain.c
===================================================================
RCS file: testsuite/gdb.base/jit-dlmain.c
diff -N testsuite/gdb.base/jit-dlmain.c
*** /dev/null	1 Jan 1970 00:00:00 -0000
--- testsuite/gdb.base/jit-dlmain.c	5 Jul 2011 20:58:12 -0000
***************
*** 0 ****
--- 1,16 ----
+ #include <dlfcn.h>
+ #include <stdio.h>
+ 
+ int main (int argc, char *argv[])
+ {
+   void *h = dlopen ("jit-dlmain-so.so", RTLD_LAZY);
+   int (*p_main) (int, char **);
+ 
+   if (h == NULL) return 1;
+ 
+   p_main = dlsym (h, "jit_dl_main");
+   if (p_main == NULL) return 2;
+ 
+   h = h;  /* break here after-dlopen */
+   return (*p_main) (argc, argv);
+ }
Index: testsuite/gdb.base/jit-main.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/jit-main.c,v
retrieving revision 1.2
diff -u -p -n -p -r1.2 jit-main.c
*** testsuite/gdb.base/jit-main.c	15 Mar 2011 21:03:44 -0000	1.2
--- testsuite/gdb.base/jit-main.c	5 Jul 2011 20:58:12 -0000
*************** update_locations (const void *const addr
*** 117,124 ****
      }
  }
  
  int
! main (int argc, char *argv[])
  {
    /* These variables are here so they can easily be set from jit.exp.  */
    const char *libname = NULL;
--- 117,128 ----
      }
  }
  
+ #ifndef MAIN
+ #define MAIN main
+ #endif
+ 
  int
! MAIN (int argc, char *argv[])
  {
    /* These variables are here so they can easily be set from jit.exp.  */
    const char *libname = NULL;
Index: testsuite/gdb.base/jit-so.exp
===================================================================
RCS file: testsuite/gdb.base/jit-so.exp
diff -N testsuite/gdb.base/jit-so.exp
*** /dev/null	1 Jan 1970 00:00:00 -0000
--- testsuite/gdb.base/jit-so.exp	5 Jul 2011 20:58:12 -0000
***************
*** 0 ****
--- 1,117 ----
+ # Copyright 2011 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/>.
+ 
+ # The same tests as in jit.exp, but loading JITer itself from a shared
+ # library.
+ 
+ if $tracelevel {
+     strace $tracelevel
+ }
+ 
+ if {[skip_shlib_tests]} {
+     untested jit-so.exp
+     return -1
+ }
+ 
+ if {[get_compiler_info not-used]} {
+     warning "Could not get compiler info"
+     untested jit-so.exp
+     return 1
+ }
+ 
+ #
+ # test running programs
+ #
+ 
+ set testfile jit-dlmain
+ set srcfile ${testfile}.c
+ set binfile ${objdir}/${subdir}/${testfile}
+ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug shlib_load}] != "" } {
+     untested jit-so.exp
+     return -1
+ }
+ 
+ set testfile2 jit-main
+ set srcfile2 ${testfile2}.c
+ set binfile2 ${objdir}/${subdir}/${testfile2}.so
+ if { [gdb_compile_shlib "${srcdir}/${subdir}/${srcfile2}" ${binfile2} {debug additional_flags="-DMAIN=jit_dl_main"}] != "" } {
+     untested jit.exp
+     return -1
+ }
+ 
+ set solib_testfile "jit-solib"
+ set solib_srcfile "${srcdir}/${subdir}/${solib_testfile}.c"
+ set solib_binfile "${objdir}/${subdir}/${solib_testfile}.so"
+ set solib_binfile_test_msg "OBJDIR/${subdir}/${solib_testfile}.so"
+ 
+ # Note: compiling without debug info: the library goes through symbol
+ # renaming by munging on its symbol table, and that wouldn't work for .debug
+ # sections.  Also, output for "info function" changes when debug info is resent.
+ if { [gdb_compile_shlib ${solib_srcfile} ${solib_binfile} {}] != "" } {
+     untested jit-so.exp
+     return -1
+ }
+ 
+ proc one_jit_test {count match_str} {
+     global verbose testfile srcfile2 solib_binfile solib_binfile_test_msg pf_prefix
+ 
+     set old_pf_prefix $pf_prefix
+     set pf_prefix "one_jit_test-$count"
+ 
+     clean_restart $testfile
+ 
+     # This is just to help debugging when things fail
+     if {$verbose > 0} {
+ 	gdb_test "set debug jit 1"
+     }
+ 
+     if { ![runto_main] } {
+ 	fail "Can't run to main"
+ 	return
+     }
+ 
+     gdb_breakpoint [gdb_get_line_number "break here after-dlopen" ]
+     gdb_continue_to_breakpoint "break here after-dlopen"
+ 
+     gdb_breakpoint "$srcfile2:[gdb_get_line_number {break here 0} $srcfile2]"
+     gdb_continue_to_breakpoint "break here 0"
+ 
+     # Poke desired values directly into inferior instead of using "set args"
+     # because "set args" does not work under gdbserver.
+     gdb_test_no_output "set var argc = 2"
+     gdb_test_no_output "set var libname = \"$solib_binfile\"" "set var libname = \"$solib_binfile_test_msg\""
+     gdb_test_no_output "set var count = $count"
+ 
+     gdb_breakpoint "$srcfile2:[gdb_get_line_number {break here 1} $srcfile2]"
+     gdb_continue_to_breakpoint "break here 1"
+ 
+     gdb_test "info function jit_function" "$match_str"
+ 
+     # This is just to help debugging when things fail
+     if {$verbose > 0} {
+ 	gdb_test "maintenance print objfiles"
+ 	gdb_test "maintenance info break"
+     }
+ 
+     gdb_breakpoint "$srcfile2:[gdb_get_line_number {break here 2} $srcfile2]"
+     gdb_continue_to_breakpoint "break here 2"
+     # All jit librares must have been unregistered
+     gdb_test "info function jit_function" \
+ 	"All functions matching regular expression \"jit_function\":" \
+     set pf_prefix $old_pf_prefix
+ }
+ 
+ one_jit_test 1 "${hex}  jit_function_0000"
+ one_jit_test 2 "${hex}  jit_function_0000\[\r\n\]+${hex}  jit_function_0001"

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression Re: [gdb-7.3] Error in gdb-llvm integration: Unable to read JIT descriptor from remote memory!

[Joel Brobecker]

> Another option is to apply a better fix (provided it is deemed safe) ...

Thanks for doing that :)

Your fix certainly looks a lot nicer than Jan's! After careful
reading, I think the patch makes sense, but having never looked
at this code before, it would be better if someone else approved.

Can Doug approve the patch, for instance? The patch is OK for 7.3
if OK for HEAD.

> 2011-07-05  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	jit.c (jit_breakpoint_re_set_internal): Call jit_inferior_init.

Should we say that you're also adding jit_inferior_init's declaration?

Note that some of us (myself included) prefer moving functions
rather than having to write a declaration which duplicates the
amount of work to do when the function profile changes.  But
others disagree, and would rather use these declarations to allow
them to order the functions in a certain order of their liking...
I think that for the 7.3 branch we'll definitely want your version,
to keep the patch as small and simple as possible, but you're free
to choose whichever style you might prefer.

> testsuite/ChangeLog:
> 
> 2011-07-05  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	* gdb.base/jit-so.exp: New test.
> 	* gdb.base/jit-dlmain.c: New file.
> 	* gdb.base/jit-main.c: Allow "main" to be elsewhere.

Cheers,
-- 
Joel

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression Re: [gdb-7.3] Error in gdb-llvm integration: Unable to read JIT descriptor from remote memory!

[Paul Pluzhnikov]

On Tue, Jul 5, 2011 at 3:02 PM, Joel Brobecker <brobecker@adacore.com> wrote:

> Should we say that you're also adding jit_inferior_init's declaration?

Sure.

> Note that some of us (myself included) prefer moving functions

I do as well. However, jit_inferior_init and jit_breakpoint_re_set_internal
call each other, so moving them around doesn't help (I think ;-)

Thanks,
-- 
Paul Pluzhnikov


2011-07-05  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* jit.c (jit_inferior_init): Forward declare.
	(jit_breakpoint_re_set_internal): Call jit_inferior_init.

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression

[Jan Kratochvil]

On Tue, 05 Jul 2011 23:06:47 +0200, Paul Pluzhnikov wrote:
> Another option is to apply a better fix (provided it is deemed safe) ...

It is still regressing against 739571cc3151651f49f7171cfd98275d983bfaaa^ with
the attached testcase:
	FAIL: gdb.base/jit-so.exp: one_jit_test-1 run-2 info function jit_function
	FAIL: gdb.base/jit-so.exp: one_jit_test-2 run-2 info function jit_function

It is also regressing on the former fix by me but it is no longer regressing
on the fix attached here.

That is on unload+reload of the JIT engine.  OTOH (a) this case is probably
not faced by normal users and (b) it needs more work for better performance
and (c) in this case maybe the overall multi-JIT rework would be better.

So I am fine with the Paul's fix for 7.3; just the testcase there does not
work for me, I had to change:
-  void *h = dlopen ("jit-dlmain-so.so", RTLD_LAZY);
+  void *h = dlopen ("gdb.base/jit-main.so", RTLD_LAZY);

And jit-dlmain.c was missing the GPL comment.


> It took me  much longer to create a test case (I don't have 'lli'), then
> it did to fix the problem once I had the repro.

I agree that testcase of yours is based on better principle.


Thanks,
Jan


--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -38,6 +38,15 @@ static const char *const jit_break_name = "__jit_debug_register_code";
 
 static const char *const jit_descriptor_name = "__jit_debug_descriptor";
 
+/* This is a boolean indicating whether we're currently registering code.  This
+   is used to avoid re-entering the registration code.  We want to check for
+   new JITed every time a new object file is loaded, but we want to avoid
+   checking for new code while we're registering object files for JITed code.
+   Therefore, we flip this variable to 1 before registering new object files,
+   and set it to 0 before returning.  */
+
+static int registering_code = 0;
+
 static const struct inferior_data *jit_inferior_data = NULL;
 
 /* Non-zero if we want to see trace of jit level stuff.  */
@@ -295,9 +304,18 @@ JITed symbol file is not an object file, ignoring it.\n"));
         ++i;
       }
 
+  /* Raise this flag while we register code so we won't trigger any
+     re-registration.  */
+  gdb_assert (registering_code == 0);
+  my_cleanups = make_cleanup_restore_integer (&registering_code);
+  registering_code = 1;
+
   /* This call takes ownership of sai.  */
   objfile = symbol_file_add_from_bfd (nbfd, 0, sai, OBJF_SHARED, NULL);
 
+  /* Clear the registering_code flag.  */
+  do_cleanups (my_cleanups);
+
   /* Remember a mapping from entry_addr to objfile.  */
   entry_addr_ptr = xmalloc (sizeof (CORE_ADDR));
   *entry_addr_ptr = entry_addr;
@@ -332,41 +350,6 @@ jit_find_objf_with_entry_addr (CORE_ADDR entry_addr)
   return NULL;
 }
 
-/* (Re-)Initialize the jit breakpoint if necessary.
-   Return 0 on success.  */
-
-static int
-jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
-				struct jit_inferior_data *inf_data)
-{
-  if (inf_data->breakpoint_addr == 0)
-    {
-      struct minimal_symbol *reg_symbol;
-
-      /* Lookup the registration symbol.  If it is missing, then we assume
-	 we are not attached to a JIT.  */
-      reg_symbol = lookup_minimal_symbol (jit_break_name, NULL, NULL);
-      if (reg_symbol == NULL)
-	return 1;
-      inf_data->breakpoint_addr = SYMBOL_VALUE_ADDRESS (reg_symbol);
-      if (inf_data->breakpoint_addr == 0)
-	return 2;
-    }
-  else
-    return 0;
-
-  if (jit_debug)
-    fprintf_unfiltered (gdb_stdlog,
-			"jit_breakpoint_re_set_internal, "
-			"breakpoint_addr = %s\n",
-			paddress (gdbarch, inf_data->breakpoint_addr));
-
-  /* Put a breakpoint in the registration symbol.  */
-  create_jit_event_breakpoint (gdbarch, inf_data->breakpoint_addr);
-
-  return 0;
-}
-
 /* Register any already created translations.  */
 
 static void
@@ -375,29 +358,54 @@ jit_inferior_init (struct gdbarch *gdbarch)
   struct jit_descriptor descriptor;
   struct jit_code_entry cur_entry;
   struct jit_inferior_data *inf_data;
-  CORE_ADDR cur_entry_addr;
+  CORE_ADDR cur_entry_addr, breakpoint_addr;
+  struct minimal_symbol *reg_symbol, *desc_symbol;
 
   if (jit_debug)
-    fprintf_unfiltered (gdb_stdlog, "jit_inferior_init\n");
+    fprintf_unfiltered (gdb_stdlog,
+		       "jit_inferior_init, registering_code = %d\n",
+		       registering_code);
 
-  inf_data = get_jit_inferior_data ();
-  if (jit_breakpoint_re_set_internal (gdbarch, inf_data) != 0)
+  /* When we register code, GDB resets its breakpoints in case symbols have
+     changed.  That in turn calls this handler, which makes us look for new
+     code again.  To avoid being re-entered, we check this flag.  */
+  if (registering_code)
     return;
 
-  if (inf_data->descriptor_addr == 0)
-    {
-      struct minimal_symbol *desc_symbol;
+  inf_data = get_jit_inferior_data ();
 
-      /* Lookup the descriptor symbol and cache the addr.  If it is
-	 missing, we assume we are not attached to a JIT and return early.  */
-      desc_symbol = lookup_minimal_symbol (jit_descriptor_name, NULL, NULL);
-      if (desc_symbol == NULL)
-	return;
+  /* Lookup the registration symbol.  If it is missing, then we assume
+     we are not attached to a JIT.  */
+  breakpoint_addr = 0;
+  reg_symbol = lookup_minimal_symbol (jit_break_name, NULL, NULL);
+  if (reg_symbol != NULL)
+    breakpoint_addr = SYMBOL_VALUE_ADDRESS (reg_symbol);
+
+  if (jit_debug)
+    fprintf_unfiltered (gdb_stdlog,
+			"jit_inferior_init, new breakpoint_addr = %s, "
+			"old breakpoint_addr = %s\n",
+			paddress (gdbarch, breakpoint_addr),
+			paddress (gdbarch, inf_data->breakpoint_addr));
 
-      inf_data->descriptor_addr = SYMBOL_VALUE_ADDRESS (desc_symbol);
-      if (inf_data->descriptor_addr == 0)
-	return;
+  remove_jit_event_breakpoints ();
+  if (breakpoint_addr != 0)
+    {
+      /* Put a breakpoint in the registration symbol.  */
+      create_jit_event_breakpoint (gdbarch, breakpoint_addr);
     }
+  inf_data->breakpoint_addr = breakpoint_addr;
+
+  /* Lookup the descriptor symbol and cache the addr.  If it is
+     missing, we assume we are not attached to a JIT and return early.  */
+  inf_data->descriptor_addr = 0;
+  desc_symbol = lookup_minimal_symbol (jit_descriptor_name, NULL, NULL);
+  if (desc_symbol == NULL)
+    return;
+
+  inf_data->descriptor_addr = SYMBOL_VALUE_ADDRESS (desc_symbol);
+  if (inf_data->descriptor_addr == 0)
+    return;
 
   if (jit_debug)
     fprintf_unfiltered (gdb_stdlog,
@@ -443,8 +451,7 @@ jit_inferior_created_hook (void)
 void
 jit_breakpoint_re_set (void)
 {
-  jit_breakpoint_re_set_internal (target_gdbarch,
-				  get_jit_inferior_data ());
+  jit_inferior_init (target_gdbarch);
 }
 
 /* Reset inferior_data, so sybols will be looked up again, and jit_breakpoint
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-clang.c
@@ -0,0 +1,30 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 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/>.  */
+
+#include <stdlib.h>
+
+static void
+jit_clang_function_name (void)
+{
+  abort ();
+}
+
+int
+main (void)
+{
+  jit_clang_function_name ();
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-clang.exp
@@ -0,0 +1,52 @@
+# Copyright (C) 2011 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/>.
+
+# gdbserver configuration does not support passing arguments to `lli'.
+if { ![isnative] || [is_remote target] } {
+    return
+}
+
+set testfile "jit-clang"
+set srcfile ${testfile}.c
+set binfile $objdir/$subdir/${testfile}.bc
+
+if {[catch "system \"clang -fexceptions $srcdir/$subdir/$srcfile -c -emit-llvm -o ${binfile}\""] != 0} {
+    verbose -log "clang compilation failed"
+    untested ${testfile}.exp
+    return -1
+}
+if {[catch "system \"which lli &>/dev/null\""] != 0} {
+    verbose -log "lli not found"
+    untested ${testfile}.exp
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+if [gdb_load "lli"] {
+    untested ${testfile}.exp
+    return -1
+}
+
+gdb_test_no_output "set args -jit-emit-debug ${binfile}" "set args"
+gdb_run_cmd
+
+# The error was:
+# Unable to read JIT descriptor from remote memory!
+gdb_test "" "\r\nProgram received signal SIGABRT.*" "run"
+
+gdb_test "bt" " jit_clang_function_name .*" "jit registration succeeded"
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-dlmain.c
@@ -0,0 +1,40 @@
+#include <dlfcn.h>
+#include <stdio.h>
+
+static int
+run (int argc, char *argv[])
+{
+  void *h = dlopen ("gdb.base/jit-main.so", RTLD_LAZY);
+  int (*p_main) (int, char **);
+  int rc;
+
+  if (h == NULL) return 1;
+
+  p_main = dlsym (h, "jit_dl_main");
+  if (p_main == NULL) return 2;
+
+  h = h;  /* break here after-dlopen */
+  rc = (*p_main) (argc, argv);
+  if (rc != 0)
+    return rc;
+
+  if (dlclose (h) != 0)
+    return 3;
+
+  return 0;
+}
+
+int main (int argc, char *argv[])
+{
+  int rc;
+
+  rc = run (argc, argv);
+  if (rc != 0)
+    return rc;
+
+  rc = run (argc, argv);
+  if (rc != 0)
+    return rc + 10;
+
+  return 0;
+}
--- a/gdb/testsuite/gdb.base/jit-main.c
+++ b/gdb/testsuite/gdb.base/jit-main.c
@@ -117,8 +117,12 @@ update_locations (const void *const addr, int idx)
     }
 }
 
+#ifndef MAIN
+#define MAIN main
+#endif
+
 int
-main (int argc, char *argv[])
+MAIN (int argc, char *argv[])
 {
   /* These variables are here so they can easily be set from jit.exp.  */
   const char *libname = NULL;
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-so.exp
@@ -0,0 +1,137 @@
+# Copyright 2011 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/>.
+
+# The same tests as in jit.exp, but loading JITer itself from a shared
+# library.
+
+if $tracelevel {
+    strace $tracelevel
+}
+
+if {[skip_shlib_tests]} {
+    untested jit-so.exp
+    return -1
+}
+
+if {[get_compiler_info not-used]} {
+    warning "Could not get compiler info"
+    untested jit-so.exp
+    return 1
+}
+
+#
+# test running programs
+#
+
+set testfile jit-dlmain
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug shlib_load}] != "" } {
+    untested jit-so.exp
+    return -1
+}
+
+set testfile2 jit-main
+set srcfile2 ${testfile2}.c
+set binfile2 ${objdir}/${subdir}/${testfile2}.so
+if { [gdb_compile_shlib "${srcdir}/${subdir}/${srcfile2}" ${binfile2} {debug additional_flags="-DMAIN=jit_dl_main"}] != "" } {
+    untested jit.exp
+    return -1
+}
+
+set solib_testfile "jit-solib"
+set solib_srcfile "${srcdir}/${subdir}/${solib_testfile}.c"
+set solib_binfile "${objdir}/${subdir}/${solib_testfile}.so"
+set solib_binfile_test_msg "OBJDIR/${subdir}/${solib_testfile}.so"
+
+# Note: compiling without debug info: the library goes through symbol
+# renaming by munging on its symbol table, and that wouldn't work for .debug
+# sections.  Also, output for "info function" changes when debug info is resent.
+if { [gdb_compile_shlib ${solib_srcfile} ${solib_binfile} {}] != "" } {
+    untested jit-so.exp
+    return -1
+}
+
+proc run {run count match_str} {
+    global verbose testfile srcfile2 solib_binfile solib_binfile_test_msg pf_prefix srcfile
+
+    set old_pf_prefix $pf_prefix
+    lappend pf_prefix "run-$run"
+
+    gdb_breakpoint $srcfile:[gdb_get_line_number "break here after-dlopen" ]
+    gdb_continue_to_breakpoint "break here after-dlopen"
+
+    gdb_breakpoint "$srcfile2:[gdb_get_line_number {break here 0} $srcfile2]"
+    gdb_continue_to_breakpoint "break here 0"
+
+    # Poke desired values directly into inferior instead of using "set args"
+    # because "set args" does not work under gdbserver.
+    gdb_test_no_output "set var argc = 2"
+    gdb_test_no_output "set var libname = \"$solib_binfile\"" "set var libname = \"$solib_binfile_test_msg\""
+    gdb_test_no_output "set var count = $count"
+
+    gdb_breakpoint "$srcfile2:[gdb_get_line_number {break here 1} $srcfile2]"
+    gdb_continue_to_breakpoint "break here 1"
+
+    gdb_test "info function jit_function" "$match_str"
+
+    # This is just to help debugging when things fail
+    if {$verbose > 0} {
+	gdb_test "maintenance print objfiles"
+	gdb_test "maintenance info break"
+    }
+
+    gdb_breakpoint "$srcfile2:[gdb_get_line_number {break here 2} $srcfile2]"
+    gdb_continue_to_breakpoint "break here 2"
+    # All jit librares must have been unregistered
+    gdb_test "info function jit_function" \
+	"All functions matching regular expression \"jit_function\":" \
+
+    set pf_prefix $old_pf_prefix
+}
+
+proc one_jit_test {count match_str} {
+    global verbose testfile pf_prefix
+
+    set old_pf_prefix $pf_prefix
+    lappend pf_prefix "one_jit_test-$count"
+
+    clean_restart $testfile
+
+    # Error is OK if not supported by the platform.
+    gdb_test "set disable-randomization off"
+
+    # This is just to help debugging when things fail
+    if {$verbose > 0} {
+	gdb_test "set debug jit 1"
+    }
+
+    if { ![runto_main] } {
+	fail "Can't run to main"
+	return
+    }
+
+    run 1 $count $match_str
+
+    # Second run with reloaded JITer.
+    gdb_test_no_output "delete breakpoints" "delete breakpoints" \
+		       {Delete all breakpoints\? \(y or n\) } "y"
+    run 2 $count $match_str
+
+    set pf_prefix $old_pf_prefix
+}
+
+one_jit_test 1 "${hex}  jit_function_0000"
+one_jit_test 2 "${hex}  jit_function_0000\[\r\n\]+${hex}  jit_function_0001"
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -923,6 +923,13 @@ proc gdb_test_no_output { args } {
 	set message $command
     }
 
+    if [llength $args]==4 {
+	set question_string [lindex $args 2];
+	set response_string [lindex $args 3];
+    } else {
+	set question_string "^FOOBAR$"
+    }
+
     set command_regex [string_to_regexp $command]
     gdb_test_multiple $command $message {
         -re "^$command_regex\r\n$gdb_prompt $" {
@@ -930,6 +937,16 @@ proc gdb_test_no_output { args } {
 		pass "$message"
             }
         }
+	-re "^$command_regex\r\n${question_string}$" {
+	    # $command_regex is already precompiled above.
+	    gdb_test_multiple $response_string $message {
+		-re "^[string_to_regexp $response_string]\r\n$gdb_prompt $" {
+		    if ![string match "" $message] then {
+			pass "$message"
+		    }
+		}
+	    }
+	}
     }
 }
 

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression

[Joel Brobecker]

> That is on unload+reload of the JIT engine.  OTOH (a) this case is probably
> not faced by normal users and (b) it needs more work for better performance
> and (c) in this case maybe the overall multi-JIT rework would be better.

So, would the following approach be acceptable?
  (1) HEAD: Apply Paul's patch, then look at solutions for
      the remaining failures/regressions...
  (2) BRANCH: Apply Paul's patch; I think we might want to apply
      the testcase part as well, even if we still need to figure out
      why it doesn't work for Jan...

Once the patch is applied, we can produce the first pre-release
tarball.

-- 
Joel

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression

[Paul Pluzhnikov]

On Wed, Jul 6, 2011 at 9:07 AM, Joel Brobecker <brobecker@adacore.com> wrote:

>  (2) BRANCH: Apply Paul's patch; I think we might want to apply
>      the testcase part as well, even if we still need to figure out
>      why it doesn't work for Jan...

Possibly due to differences in glibc and/or LD_LIBRARY_PATH.

It's quite trivial to revise the test so it dlopens jit-dlmain-so.so via
absolute path to eliminate such problems.

I'll send updated patch (on original thread) in a few minutes ...

-- 
Paul Pluzhnikov

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression

[Jan Kratochvil]

On Wed, 06 Jul 2011 18:07:04 +0200, Joel Brobecker wrote:
> > That is on unload+reload of the JIT engine.  OTOH (a) this case is probably
> > not faced by normal users and (b) it needs more work for better performance
> > and (c) in this case maybe the overall multi-JIT rework would be better.
> 
> So, would the following approach be acceptable?
>   (1) HEAD: Apply Paul's patch, then look at solutions for
>       the remaining failures/regressions...

If Paul is going to implement the multi-JITer one as he was talking about.
Otherwise I would be for update by me of the very last patch of mine which
PASSed unload + reload as 739571cc3151651f49f7171cfd98275d983bfaaa^ did.


>   (2) BRANCH: Apply Paul's patch; I think we might want to apply
>       the testcase part as well, even if we still need to figure out
>       why it doesn't work for Jan...

yes


Thanks,
Jan

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression

[Paul Pluzhnikov]

On Wed, Jul 6, 2011 at 9:32 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Wed, 06 Jul 2011 18:07:04 +0200, Joel Brobecker wrote:
>> > That is on unload+reload of the JIT engine.  OTOH (a) this case is probably
>> > not faced by normal users and (b) it needs more work for better performance
>> > and (c) in this case maybe the overall multi-JIT rework would be better.
>>
>> So, would the following approach be acceptable?
>>   (1) HEAD: Apply Paul's patch, then look at solutions for
>>       the remaining failures/regressions...
>
> If Paul is going to implement the multi-JITer one as he was talking about.

I am not sure there is an actual use case for that, and I was not planning
on working on that.

> Otherwise I would be for update by me of the very last patch of mine which
> PASSed unload + reload as 739571cc3151651f49f7171cfd98275d983bfaaa^ did.

Fixing the unload/reload case would (I expect) be fairly simple without
reverting to a global -- just need to clear jit_inferior_data on unload.

>>   (2) BRANCH: Apply Paul's patch; I think we might want to apply
>>       the testcase part as well, even if we still need to figure out
>>       why it doesn't work for Jan...

Updated patch should work for Jan...

Thanks,
-- 
Paul Pluzhnikov

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression

[Jan Kratochvil]

On Wed, 06 Jul 2011 19:03:13 +0200, Paul Pluzhnikov wrote:
> I am not sure there is an actual use case for that, and I was not planning
> on working on that.

OK, not going to work on that myself.


Thanks,
Jan

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression Re: [gdb-7.3] Error in gdb-llvm integration: Unable to read JIT descriptor from remote memory!

[Paul Pluzhnikov]

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

On Tue, Jul 5, 2011 at 2:06 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

Updated patch with comments from
http://sourceware.org/ml/gdb-patches/2011-07/msg00195.html

Thanks,
-- 
Paul Pluzhnikov


2011-07-06  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* jit.c (jit_inferior_init): Forward declare.
	(jit_breakpoint_re_set_internal): Call jit_inferior_init.


testsuite/ChangeLog:

2011-07-06  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* gdb.base/jit-so.exp: New test.
	* gdb.base/jit-dlmain.c: New file.
	* gdb.base/jit-main.c: Allow "main" to be elsewhere.

[-- Attachment #2: gdb-jit-20110706.txt --]
[-- Type: text/plain, Size: 7329 bytes --]

Index: jit.c
===================================================================
RCS file: /cvs/src/src/gdb/jit.c,v
retrieving revision 1.12
diff -u -p -n -p -r1.12 jit.c
*** jit.c	17 Apr 2011 18:38:45 -0000	1.12
--- jit.c	6 Jul 2011 16:35:02 -0000
*************** static const char *const jit_descriptor_
*** 40,45 ****
--- 40,48 ----
  
  static const struct inferior_data *jit_inferior_data = NULL;
  
+ static void
+ jit_inferior_init (struct gdbarch *gdbarch);
+ 
  /* Non-zero if we want to see trace of jit level stuff.  */
  
  static int jit_debug = 0;
*************** jit_breakpoint_re_set_internal (struct g
*** 351,356 ****
--- 354,364 ----
        inf_data->breakpoint_addr = SYMBOL_VALUE_ADDRESS (reg_symbol);
        if (inf_data->breakpoint_addr == 0)
  	return 2;
+ 
+       /* If we have not read the jit descriptor yet (e.g. because the JITer
+ 	 itself is in a shared library which just got loaded), do so now.  */
+       if (inf_data->descriptor_addr == 0)
+ 	jit_inferior_init (gdbarch);
      }
    else
      return 0;
Index: testsuite/gdb.base/jit-dlmain.c
===================================================================
RCS file: testsuite/gdb.base/jit-dlmain.c
diff -N testsuite/gdb.base/jit-dlmain.c
*** /dev/null	1 Jan 1970 00:00:00 -0000
--- testsuite/gdb.base/jit-dlmain.c	6 Jul 2011 16:35:02 -0000
***************
*** 0 ****
--- 1,20 ----
+ #include <dlfcn.h>
+ #include <stdio.h>
+ 
+ int main (int argc, char *argv[])
+ {
+   /* jit_libname is updated by jit-so.exp  */
+   const char *jit_libname = "jit-dlmain-so.so";
+   void *h;
+   int (*p_main) (int, char **);
+ 
+   h = NULL;  /* break here before-dlopen  */
+   h = dlopen (jit_libname, RTLD_LAZY);
+   if (h == NULL) return 1;
+ 
+   p_main = dlsym (h, "jit_dl_main");
+   if (p_main == NULL) return 2;
+ 
+   h = h;  /* break here after-dlopen */
+   return (*p_main) (argc, argv);
+ }
Index: testsuite/gdb.base/jit-main.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/jit-main.c,v
retrieving revision 1.2
diff -u -p -n -p -r1.2 jit-main.c
*** testsuite/gdb.base/jit-main.c	15 Mar 2011 21:03:44 -0000	1.2
--- testsuite/gdb.base/jit-main.c	6 Jul 2011 16:35:02 -0000
*************** update_locations (const void *const addr
*** 117,124 ****
      }
  }
  
  int
! main (int argc, char *argv[])
  {
    /* These variables are here so they can easily be set from jit.exp.  */
    const char *libname = NULL;
--- 117,128 ----
      }
  }
  
+ #ifndef MAIN
+ #define MAIN main
+ #endif
+ 
  int
! MAIN (int argc, char *argv[])
  {
    /* These variables are here so they can easily be set from jit.exp.  */
    const char *libname = NULL;
Index: testsuite/gdb.base/jit-so.exp
===================================================================
RCS file: testsuite/gdb.base/jit-so.exp
diff -N testsuite/gdb.base/jit-so.exp
*** /dev/null	1 Jan 1970 00:00:00 -0000
--- testsuite/gdb.base/jit-so.exp	6 Jul 2011 16:35:02 -0000
***************
*** 0 ****
--- 1,121 ----
+ # Copyright 2011 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/>.
+ 
+ # The same tests as in jit.exp, but loading JITer itself from a shared
+ # library.
+ 
+ if $tracelevel {
+     strace $tracelevel
+ }
+ 
+ if {[skip_shlib_tests]} {
+     untested jit-so.exp
+     return -1
+ }
+ 
+ if {[get_compiler_info not-used]} {
+     warning "Could not get compiler info"
+     untested jit-so.exp
+     return 1
+ }
+ 
+ #
+ # test running programs
+ #
+ 
+ set testfile jit-dlmain
+ set srcfile ${testfile}.c
+ set binfile ${objdir}/${subdir}/${testfile}
+ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug shlib_load}] != "" } {
+     untested jit-so.exp
+     return -1
+ }
+ 
+ set testfile2 jit-main
+ set srcfile2 ${testfile2}.c
+ set binfile2 ${objdir}/${subdir}/${testfile2}.so
+ if { [gdb_compile_shlib "${srcdir}/${subdir}/${srcfile2}" ${binfile2} {debug additional_flags="-DMAIN=jit_dl_main"}] != "" } {
+     untested jit.exp
+     return -1
+ }
+ 
+ set solib_testfile "jit-solib"
+ set solib_srcfile "${srcdir}/${subdir}/${solib_testfile}.c"
+ set solib_binfile "${objdir}/${subdir}/${solib_testfile}.so"
+ set solib_binfile_test_msg "OBJDIR/${subdir}/${solib_testfile}.so"
+ 
+ # Note: compiling without debug info: the library goes through symbol
+ # renaming by munging on its symbol table, and that wouldn't work for .debug
+ # sections.  Also, output for "info function" changes when debug info is resent.
+ if { [gdb_compile_shlib ${solib_srcfile} ${solib_binfile} {}] != "" } {
+     untested jit-so.exp
+     return -1
+ }
+ 
+ proc one_jit_test {count match_str} {
+     global verbose testfile srcfile2 binfile2 solib_binfile solib_binfile_test_msg pf_prefix
+ 
+     set old_pf_prefix $pf_prefix
+     set pf_prefix "one_jit_test-$count"
+ 
+     clean_restart $testfile
+ 
+     # This is just to help debugging when things fail
+     if {$verbose > 0} {
+ 	gdb_test "set debug jit 1"
+     }
+ 
+     if { ![runto_main] } {
+ 	fail "Can't run to main"
+ 	return
+     }
+ 
+     gdb_breakpoint [gdb_get_line_number "break here before-dlopen" ]
+     gdb_continue_to_breakpoint "break here before-dlopen"
+     # Poke desired values directly into inferior instead of using "set args"
+     # because "set args" does not work under gdbserver.
+     gdb_test_no_output "set var jit_libname = \"$binfile2\""
+ 
+     gdb_breakpoint [gdb_get_line_number "break here after-dlopen" ]
+     gdb_continue_to_breakpoint "break here after-dlopen"
+ 
+     gdb_breakpoint "$srcfile2:[gdb_get_line_number {break here 0} $srcfile2]"
+     gdb_continue_to_breakpoint "break here 0"
+ 
+     gdb_test_no_output "set var argc = 2"
+     gdb_test_no_output "set var libname = \"$solib_binfile\"" "set var libname = \"$solib_binfile_test_msg\""
+     gdb_test_no_output "set var count = $count"
+ 
+     gdb_breakpoint "$srcfile2:[gdb_get_line_number {break here 1} $srcfile2]"
+     gdb_continue_to_breakpoint "break here 1"
+ 
+     gdb_test "info function jit_function" "$match_str"
+ 
+     # This is just to help debugging when things fail
+     if {$verbose > 0} {
+ 	gdb_test "maintenance print objfiles"
+ 	gdb_test "maintenance info break"
+     }
+ 
+     gdb_breakpoint "$srcfile2:[gdb_get_line_number {break here 2} $srcfile2]"
+     gdb_continue_to_breakpoint "break here 2"
+     # All jit librares must have been unregistered
+     gdb_test "info function jit_function" \
+ 	"All functions matching regular expression \"jit_function\":" \
+     set pf_prefix $old_pf_prefix
+ }
+ 
+ one_jit_test 1 "${hex}  jit_function_0000"
+ one_jit_test 2 "${hex}  jit_function_0000\[\r\n\]+${hex}  jit_function_0001"

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression Re: [gdb-7.3] Error in gdb-llvm integration: Unable to read JIT descriptor from remote memory!

[Joel Brobecker]

> 2011-07-06  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	* jit.c (jit_inferior_init): Forward declare.
> 	(jit_breakpoint_re_set_internal): Call jit_inferior_init.
> 
> 
> testsuite/ChangeLog:
> 
> 2011-07-06  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	* gdb.base/jit-so.exp: New test.
> 	* gdb.base/jit-dlmain.c: New file.
> 	* gdb.base/jit-main.c: Allow "main" to be elsewhere.

Patch checked in HEAD + branch.

Thanks, Paul!
-- 
Joel

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression Re: [gdb-7.3] Error in gdb-llvm integration: Unable to read JIT descriptor from remote memory!

[Paul Pluzhnikov]

On Wed, Jul 6, 2011 at 3:06 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> 2011-07-06  Paul Pluzhnikov  <ppluzhnikov@google.com>
>>
>>       * jit.c (jit_inferior_init): Forward declare.
>>       (jit_breakpoint_re_set_internal): Call jit_inferior_init.
>>
>>
>> testsuite/ChangeLog:
>>
>> 2011-07-06  Paul Pluzhnikov  <ppluzhnikov@google.com>
>>
>>       * gdb.base/jit-so.exp: New test.
>>       * gdb.base/jit-dlmain.c: New file.
>>       * gdb.base/jit-main.c: Allow "main" to be elsewhere.
>
> Patch checked in HEAD + branch.

This then leaves the "unload/reload" case not handled.

I will extend the test case to show this failure, and then fix it (HEAD
only, as that is not a very important use case).

Thanks,
-- 
Paul Pluzhnikov

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression Re: [gdb-7.3] Error in gdb-llvm integration: Unable to read JIT descriptor from remote memory!

[Joel Brobecker]

> This then leaves the "unload/reload" case not handled.
> 
> I will extend the test case to show this failure, and then fix it (HEAD
> only, as that is not a very important use case).

Great! That was my understanding too.

Thanks, Paul.
-- 
Joel

Re: [patch,7.3] Fix JIT clang-lli gdb-7.3 regression Re: [gdb-7.3] Error in gdb-llvm integration: Unable to read JIT descriptor from remote memory!

[Jan Kratochvil]

On Thu, 07 Jul 2011 00:43:50 +0200, Paul Pluzhnikov wrote:
> This then leaves the "unload/reload" case not handled.

What I was trying to exploit with the countercases is that breakpoint_re_set
is designed so that if anything in the inferior changes everything gets
re-set.

Since the former Paul's change 739571cc3151651f49f7171cfd98275d983bfaaa some
information is kept even across breakpoint_re_set which I find against the
design of breakpoint_re_set.  It now keeps information about inferior even if
some unspecific inferior change has happened.

The problem is breakpoint_re_set is no longer performance sufficient.  This is
a part of the large task Gary Benson is working on, to make inferior solib
notififications local and specific to each solib.  But it needs to pass more
information what has changed than the global breakpoint_re_set event.  I do
not think that workarounding breakpoint_re_set problems at its callees is
right.  But I do not have any further counter testcases.


Thanks,
Jan