[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 <jan dot kratochvil at redhat dot com>]

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"