[PATCH 0/3] Fix PR gdb/19858: GDB doesn't register the JIT libraries on attach

[PATCH 0/3] Fix PR gdb/19858: GDB doesn't register the JIT libraries on attach

[Pedro Alves]

As discussed at https://sourceware.org/ml/gdb/2016-03/msg00023.html,
GDB currently fails to fetch the list of already-registered JIT
modules on attach.  This series fixes gdb, and adds a regression test.

I'm aiming at pushing it to both master and the 7.11 branch.

Tested on x86_64 Fedora 23.

Pedro Alves (2):
  Make gdb.base/jit.exp binaries unique
  Add regression test for PR gdb/19858 (JIT code registration on attach)

Yichao Yu (1):
  Fix PR gdb/19858: GDB doesn't register the JIT libraries on attach

 gdb/jit.c                         | 13 +++++-
 gdb/testsuite/gdb.base/jit-main.c | 21 +++++++++-
 gdb/testsuite/gdb.base/jit.exp    | 88 +++++++++++++++++++++++++++++++--------
 3 files changed, 101 insertions(+), 21 deletions(-)

-- 
2.5.5

[PATCH 1/3] Fix PR gdb/19858: GDB doesn't register the JIT libraries on attach

[Pedro Alves]

From: Yichao Yu <yyc1992@gmail.com>

Ref: https://sourceware.org/ml/gdb/2016-03/msg00023.html

GDB currently fails to fetch the list of already-registered JIT
modules on attach.

Nothing is calling jit_inferior_init, which is what is responsible for
walking the JIT object list at init time.

Despite the misleading naming, jit_inferior_created_hook ->
jit_inferior_init is only called when the inferior execs.

This regressed with the fix for PR gdb/13431 (03bef283c2d3):
 https://sourceware.org/ml/gdb-patches/2012-02/msg00023.html which
removed the inferior_created (jit_inferior_created_observer)
observer.

Adding an inferior_created observer back fixes the issue.

In turn, this exposes a bug in jit_breakpoint_re_set_internal as well,
which is returning the wrong result when we already have the
breakpoint at the right address.

gdb/ChangeLog:
2016-03-23  Yichao Yu  <yyc1992@gmail.com>

	PR gdb/19858
	* jit.c (jit_breakpoint_re_set_internal): Return 0 if we already
	got the breakpoint at the right address.
	(jit_inferior_created): New function.
	(_initialize_jit): Install jit_inferior_created as
	inferior_created observer.

Signed-off-by: Pedro Alves <palves@redhat.com>
---
 gdb/jit.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index afc1c51..9fd5ae6 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -1026,7 +1026,7 @@ jit_breakpoint_deleted (struct breakpoint *b)
 }
 
 /* (Re-)Initialize the jit breakpoint if necessary.
-   Return 0 on success.  */
+   Return 0 if the jit breakpoint has been successfully initialized.  */
 
 static int
 jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
@@ -1070,7 +1070,7 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
 			paddress (gdbarch, addr));
 
   if (ps_data->cached_code_address == addr)
-    return 1;
+    return 0;
 
   /* Delete the old breakpoint.  */
   if (ps_data->jit_breakpoint != NULL)
@@ -1367,6 +1367,14 @@ jit_inferior_init (struct gdbarch *gdbarch)
     }
 }
 
+/* inferior_created observer.  */
+
+static void
+jit_inferior_created (struct target_ops *ops, int from_tty)
+{
+  jit_inferior_created_hook ();
+}
+
 /* Exported routine to call when an inferior has been created.  */
 
 void
@@ -1496,6 +1504,7 @@ _initialize_jit (void)
 			     show_jit_debug,
 			     &setdebuglist, &showdebuglist);
 
+  observer_attach_inferior_created (jit_inferior_created);
   observer_attach_inferior_exit (jit_inferior_exit_hook);
   observer_attach_breakpoint_deleted (jit_breakpoint_deleted);
 
-- 
2.5.5

Re: [PATCH 1/3] Fix PR gdb/19858: GDB doesn't register the JIT libraries on attach

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> gdb/ChangeLog:
> 2016-03-23  Yichao Yu  <yyc1992@gmail.com>
>
> 	PR gdb/19858
> 	* jit.c (jit_breakpoint_re_set_internal): Return 0 if we already
> 	got the breakpoint at the right address.
> 	(jit_inferior_created): New function.
> 	(_initialize_jit): Install jit_inferior_created as
> 	inferior_created observer.

Patch is good to me.

-- 
Yao (齐尧)

[PATCH 2/3] Make gdb.base/jit.exp binaries unique

[Pedro Alves]

This testcase compiles the same program and library differently
multiple times using the same file names.  Make them unique, to make
it easier to debug test problems.

gdb/testsuite/ChangeLog:
2016-03-23  Pedro Alves  <palves@redhat.com>

	PR gdb/19858
	* gdb.base/jit.exp (compile_jit_test): Add intro comment.  Add
	BINSUFFIX parameter, and handle it.
	(top level): Adjust calls compile_jit_test.
---
 gdb/testsuite/gdb.base/jit.exp | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/gdb/testsuite/gdb.base/jit.exp b/gdb/testsuite/gdb.base/jit.exp
index b51b878..3e12301 100644
--- a/gdb/testsuite/gdb.base/jit.exp
+++ b/gdb/testsuite/gdb.base/jit.exp
@@ -24,18 +24,19 @@ if {[get_compiler_info]} {
     return 1
 }
 
-#
-# test running programs
-#
+# Compile the testcase program and library.  BINSUFFIX is the suffix
+# to append to the program and library filenames, to make them unique
+# between invocations.  OPTIONS is passed to gdb_compile when
+# compiling the program.
 
-proc compile_jit_test {testname options} {
+proc compile_jit_test {testname binsuffix options} {
     global testfile srcfile binfile srcdir subdir
     global solib_testfile solib_srcfile solib_binfile solib_binfile_test_msg
     global solib_binfile_target
 
     set testfile jit-main
     set srcfile ${testfile}.c
-    set binfile [standard_output_file $testfile]
+    set binfile [standard_output_file $testfile$binsuffix]
     if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
 	      executable [concat debug $options]] != "" } {
 	untested $testname
@@ -44,8 +45,8 @@ proc compile_jit_test {testname options} {
 
     set solib_testfile "jit-solib"
     set solib_srcfile "${srcdir}/${subdir}/${solib_testfile}.c"
-    set solib_binfile [standard_output_file ${solib_testfile}.so]
-    set solib_binfile_test_msg "SHLIBDIR/${solib_testfile}.so"
+    set solib_binfile [standard_output_file ${solib_testfile}$binsuffix.so]
+    set solib_binfile_test_msg "SHLIBDIR/${solib_testfile}$binsuffix.so"
 
     # Note: compiling without debug info: the library goes through
     # symbol renaming by munging on its symbol table, and that
@@ -109,7 +110,7 @@ proc one_jit_test {count match_str} {
     }
 }
 
-if {[compile_jit_test jit.exp {}] < 0} {
+if {[compile_jit_test jit.exp "" {}] < 0} {
     return
 }
 one_jit_test 1 "${hex}  jit_function_0000"
@@ -117,7 +118,7 @@ one_jit_test 2 "${hex}  jit_function_0000\[\r\n\]+${hex}  jit_function_0001"
 
 with_test_prefix PIE {
     if {[compile_jit_test "jit.exp PIE tests" \
-	     {additional_flags=-fPIE ldflags=-pie}] < 0} {
+	     "-pie" {additional_flags=-fPIE ldflags=-pie}] < 0} {
 	return
     }
 
-- 
2.5.5

Re: [PATCH 2/3] Make gdb.base/jit.exp binaries unique

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> gdb/testsuite/ChangeLog:
> 2016-03-23  Pedro Alves  <palves@redhat.com>
>
> 	PR gdb/19858
> 	* gdb.base/jit.exp (compile_jit_test): Add intro comment.  Add
> 	BINSUFFIX parameter, and handle it.
> 	(top level): Adjust calls compile_jit_test.

Patch is good to me.

-- 
Yao (齐尧)

[testsuite bugreport] gdb.base/jit.exp has false PASSes, probably [Re: [PATCH 2/3] Make gdb.base/jit.exp binaries unique]

[Jan Kratochvil]

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

On Wed, 23 Mar 2016 14:29:51 +0100, Pedro Alves wrote:
> This testcase compiles the same program and library differently
> multiple times using the same file names.  Make them unique, to make
> it easier to debug test problems.
> 
> gdb/testsuite/ChangeLog:
> 2016-03-23  Pedro Alves  <palves@redhat.com>
> 
> 	PR gdb/19858
> 	* gdb.base/jit.exp (compile_jit_test): Add intro comment.  Add
> 	BINSUFFIX parameter, and handle it.
> 	(top level): Adjust calls compile_jit_test.

commit 40dea8cbf6b40f159bdfab4f3d8ec9010c293e84
Author: Pedro Alves <palves@redhat.com>
Date:   Thu Mar 31 19:28:47 2016 +0100
    Make gdb.base/jit.exp binaries unique
Message-Id: <1458739792-9864-3-git-send-email-palves@redhat.com>

This change is wrong as it sets variable $binfile to the new filename but the
variable $binfile is never used.  Therefore the testcase still uses the
unsuffixed binary even for testcases where a suffixed binary should be used.

The proper fix - to use the new $binfile value - is OK for this commit (it has
no effect on the testfile results).  But the later patch

commit 64cdf930d9ed85e93ae55adbc20b0f9848ef863b
Author: Pedro Alves <palves@redhat.com>
Date:   Thu Mar 31 19:28:47 2016 +0100
    Add regression test for PR gdb/19858 (JIT code registration on attach)

depends on this testfile deficiency and so with the proper use of suffixed
$binfile it starts to FAIL.


Jan

[-- Attachment #2: jit-binfile-fix-breaks-later.patch --]
[-- Type: text/plain, Size: 619 bytes --]

diff --git a/gdb/testsuite/gdb.base/jit.exp b/gdb/testsuite/gdb.base/jit.exp
index 3e12301..d2c4605 100644
--- a/gdb/testsuite/gdb.base/jit.exp
+++ b/gdb/testsuite/gdb.base/jit.exp
@@ -68,9 +68,9 @@ proc compile_jit_test {testname binsuffix options} {
 
 proc one_jit_test {count match_str} {
     with_test_prefix "one_jit_test-$count" {
-	global verbose testfile solib_binfile_target solib_binfile_test_msg
+	global verbose binfile solib_binfile_target solib_binfile_test_msg
 
-	clean_restart $testfile
+	clean_restart [file tail $binfile]
 
 	# This is just to help debugging when things fail
 	if {$verbose > 0} {

[-- Attachment #3: jit-binfile-not-used.patch --]
[-- Type: text/plain, Size: 429 bytes --]

diff --git a/gdb/testsuite/gdb.base/jit.exp b/gdb/testsuite/gdb.base/jit.exp
index 3e12301..568fb50 100644
--- a/gdb/testsuite/gdb.base/jit.exp
+++ b/gdb/testsuite/gdb.base/jit.exp
@@ -42,6 +42,7 @@ proc compile_jit_test {testname binsuffix options} {
 	untested $testname
 	return -1
     }
+    set binfile foo${binfile}bar
 
     set solib_testfile "jit-solib"
     set solib_srcfile "${srcdir}/${subdir}/${solib_testfile}.c"

[PATCH 3/3] Add regression test for PR gdb/19858 (JIT code registration on attach)

[Pedro Alves]

This test would fail without the previous gdb/jit.c fix:

  (gdb) attach 23031
  Attaching to program: .../build/gdb/testsuite/outputs/gdb.base/jit/jit-main, process 23031
  [...]
  207           WAIT_FOR_GDB; i = 0;  /* gdb break here 1 */
  (gdb) PASS: gdb.base/jit.exp: attach: one_jit_test-2: attach
  set var wait_for_gdb = 0
  (gdb) PASS: gdb.base/jit.exp: attach: one_jit_test-2: set var wait_for_gdb = 0
  info function ^jit_function
  All functions matching regular expression "^jit_function":
  (gdb) FAIL: gdb.base/jit.exp: attach: one_jit_test-2: info function ^jit_function

gdb/testsuite/ChangeLog:
2016-03-23  Pedro Alves  <palves@redhat.com>

	PR gdb/19858
	* gdb.base/jit-main.c: Include unistd.h.
	(ATTACH): Define to 0 if not already defined.
	(wait_for_gdb, mypid): New globals.
	(WAIT_FOR_GDB): New macro.
	(MAIN): Set an alarm.  Store the process's pid.  Wait for GDB at
	some breakpoint locations.
	* gdb.base/jit.exp (clean_reattach, continue_to_test_location):
	New procedures.
	(one_jit_test): Add REATTACH parameter, and handle it.  Use
	continue_to_test_location.
	(top level): Test attach, and adjusts calls to one_jit_test.
---
 gdb/testsuite/gdb.base/jit-main.c | 21 ++++++++++--
 gdb/testsuite/gdb.base/jit.exp    | 69 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 80 insertions(+), 10 deletions(-)

diff --git a/gdb/testsuite/gdb.base/jit-main.c b/gdb/testsuite/gdb.base/jit-main.c
index 2f0707c..63dd1a1 100644
--- a/gdb/testsuite/gdb.base/jit-main.c
+++ b/gdb/testsuite/gdb.base/jit-main.c
@@ -27,6 +27,7 @@
 #include <string.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
+#include <unistd.h>
 
 /* ElfW is coming from linux. On other platforms it does not exist.
    Let us define it here. */
@@ -116,10 +117,22 @@ update_locations (const void *const addr, int idx)
     }
 }
 
+/* Defined by the .exp file if testing attach.  */
+#ifndef ATTACH
+#define ATTACH 0
+#endif
+
 #ifndef MAIN
 #define MAIN main
 #endif
 
+/* Used to spin waiting for GDB.  */
+volatile int wait_for_gdb = ATTACH;
+#define WAIT_FOR_GDB while (wait_for_gdb)
+
+/* The current process's PID.  GDB retrieves this.  */
+int mypid;
+
 int
 MAIN (int argc, char *argv[])
 {
@@ -127,6 +140,10 @@ MAIN (int argc, char *argv[])
   const char *libname = NULL;
   int count = 0;
 
+  alarm (300);
+
+  mypid = getpid ();
+
   count = count;  /* gdb break here 0  */
 
   if (argc < 2)
@@ -190,7 +207,7 @@ MAIN (int argc, char *argv[])
           __jit_debug_register_code ();
         }
 
-      i = 0;  /* gdb break here 1 */
+      WAIT_FOR_GDB; i = 0;  /* gdb break here 1 */
 
       /* Now unregister them all in reverse order.  */
       while (__jit_debug_descriptor.relevant_entry != NULL)
@@ -215,5 +232,5 @@ MAIN (int argc, char *argv[])
           free (entry);
         }
     }
-  return 0;  /* gdb break here 2  */
+  WAIT_FOR_GDB; return 0;  /* gdb break here 2  */
 }
diff --git a/gdb/testsuite/gdb.base/jit.exp b/gdb/testsuite/gdb.base/jit.exp
index 3e12301..d1cb41a 100644
--- a/gdb/testsuite/gdb.base/jit.exp
+++ b/gdb/testsuite/gdb.base/jit.exp
@@ -66,7 +66,47 @@ proc compile_jit_test {testname binsuffix options} {
     return 0
 }
 
-proc one_jit_test {count match_str} {
+# Detach, restart GDB, and re-attach to the program.
+
+proc clean_reattach {} {
+    global decimal gdb_prompt srcfile testfile
+
+    # Get PID of test program.
+    set testpid -1
+    set test "get inferior process ID"
+    gdb_test_multiple "p mypid" $test {
+	-re ".* = ($decimal).*$gdb_prompt $" {
+	    set testpid $expect_out(1,string)
+	    pass $test
+	}
+    }
+
+    gdb_test_no_output "set var wait_for_gdb = 1"
+    gdb_test "detach" ".*"
+
+    clean_restart $testfile
+
+    set test "attach"
+    gdb_test_multiple "attach $testpid" "$test" {
+	-re "Attaching to program.*.*main.*at .*$srcfile:.*$gdb_prompt $" {
+	    pass "$test"
+	}
+    }
+
+    gdb_test_no_output "set var wait_for_gdb = 0"
+}
+
+# Continue to LOCATION in the program.  If REATTACH, detach and
+# re-attach to the program from scratch.
+proc continue_to_test_location {location reattach} {
+    gdb_breakpoint [gdb_get_line_number $location]
+    gdb_continue_to_breakpoint $location
+    if {$reattach} {
+	clean_reattach
+    }
+}
+
+proc one_jit_test {count match_str reattach} {
     with_test_prefix "one_jit_test-$count" {
 	global verbose testfile solib_binfile_target solib_binfile_test_msg
 
@@ -91,8 +131,7 @@ proc one_jit_test {count match_str} {
 	gdb_test_no_output "set var libname = \"$solib_binfile_target\"" "set var libname = \"$solib_binfile_test_msg\""
 	gdb_test_no_output "set var count = $count"
 
-	gdb_breakpoint [gdb_get_line_number "break here 1"]
-	gdb_continue_to_breakpoint "break here 1"
+	continue_to_test_location "break here 1" $reattach
 
 	gdb_test "info function ^jit_function" "$match_str"
 
@@ -102,8 +141,8 @@ proc one_jit_test {count match_str} {
 	    gdb_test "maintenance info break"
 	}
 
-	gdb_breakpoint [gdb_get_line_number "break here 2"]
-	gdb_continue_to_breakpoint "break here 2"
+	continue_to_test_location "break here 2" $reattach
+
 	# All jit librares must have been unregistered
 	gdb_test "info function jit_function" \
 	    "All functions matching regular expression \"jit_function\":"
@@ -113,8 +152,22 @@ proc one_jit_test {count match_str} {
 if {[compile_jit_test jit.exp "" {}] < 0} {
     return
 }
-one_jit_test 1 "${hex}  jit_function_0000"
-one_jit_test 2 "${hex}  jit_function_0000\[\r\n\]+${hex}  jit_function_0001"
+one_jit_test 1 "${hex}  jit_function_0000" 0
+one_jit_test 2 "${hex}  jit_function_0000\[\r\n\]+${hex}  jit_function_0001" 0
+
+# Test attaching to an inferior with some JIT libraries already
+# registered.  We reuse the normal test, and detach/reattach at
+# specific interesting points.
+if {[can_spawn_for_attach]} {
+    if {[compile_jit_test "jit.exp attach tests" \
+	     "-attach" {additional_flags=-DATTACH=1}] < 0} {
+	return
+    }
+
+    with_test_prefix attach {
+	one_jit_test 2 "${hex}  jit_function_0000\[\r\n\]+${hex}  jit_function_0001" 1
+    }
+}
 
 with_test_prefix PIE {
     if {[compile_jit_test "jit.exp PIE tests" \
@@ -122,5 +175,5 @@ with_test_prefix PIE {
 	return
     }
 
-    one_jit_test 1 "${hex}  jit_function_0000"
+    one_jit_test 1 "${hex}  jit_function_0000" 0
 }
-- 
2.5.5

Re: [PATCH 3/3] Add regression test for PR gdb/19858 (JIT code registration on attach)

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> gdb/testsuite/ChangeLog:
> 2016-03-23  Pedro Alves  <palves@redhat.com>
>
> 	PR gdb/19858
> 	* gdb.base/jit-main.c: Include unistd.h.
> 	(ATTACH): Define to 0 if not already defined.
> 	(wait_for_gdb, mypid): New globals.
> 	(WAIT_FOR_GDB): New macro.
> 	(MAIN): Set an alarm.  Store the process's pid.  Wait for GDB at
> 	some breakpoint locations.
> 	* gdb.base/jit.exp (clean_reattach, continue_to_test_location):
> 	New procedures.
> 	(one_jit_test): Add REATTACH parameter, and handle it.  Use
> 	continue_to_test_location.
> 	(top level): Test attach, and adjusts calls to one_jit_test.

Patch is good to me.

-- 
Yao (齐尧)

Re: [PATCH 0/3] Fix PR gdb/19858: GDB doesn't register the JIT libraries on attach

[Pedro Alves]

On 03/23/2016 01:29 PM, Pedro Alves wrote:
> As discussed at https://sourceware.org/ml/gdb/2016-03/msg00023.html,
> GDB currently fails to fetch the list of already-registered JIT
> modules on attach.  This series fixes gdb, and adds a regression test.
> 
> I'm aiming at pushing it to both master and the 7.11 branch.

Now pushed, both master and 7.11 branch.

Thanks,
Pedro Alves

> Tested on x86_64 Fedora 23.
> 
> Pedro Alves (2):
>   Make gdb.base/jit.exp binaries unique
>   Add regression test for PR gdb/19858 (JIT code registration on attach)
> 
> Yichao Yu (1):
>   Fix PR gdb/19858: GDB doesn't register the JIT libraries on attach
> 
>  gdb/jit.c                         | 13 +++++-
>  gdb/testsuite/gdb.base/jit-main.c | 21 +++++++++-
>  gdb/testsuite/gdb.base/jit.exp    | 88 +++++++++++++++++++++++++++++++--------
>  3 files changed, 101 insertions(+), 21 deletions(-)
>