[review] jit: remove bp locations when unregistering jit code

[review] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................

jit: remove bp locations when unregistering jit code

Fixes the rare problem when identical JIT code is registered and
unregistered several times and its symbols happen to be relocated to the
exactly the same addresses as previous time.  In such situation gdb would
still think that old breakpoint locations are still inserted (because
`inserted` flag is set and objfile/address is the same) despite the fact
that code memory was overwritten and does not contain a breakpoint trap
anymore.

This solution removes such breakpoint location from the respective
breakpoint location list.  That way next time
`update_breakpoint_locations` is called, it will re-create a new
bp_location instance if that JIT code was loaded again, and won't try
reusing the previous one.  It is a conservative solution which only
applies to object files coming from JIT.

Added test case tries to reproduce the problem using the opencl test
suite (because current jit test suite doesn't actually execute the
code).  Without this patch the breakpoint would only trigger during the first
kernel run and get completely ignored second run.

gdb/ChangeLog:
2019-11-14  Mihails Strasuns  <mihails.strasuns@intel.com>
	* breakpoint.h, breakpoint.c (forget_breakpoint_locations_obj): new
	function to forget all breakpoint locations for a given objfile.
	* jit.c (jit_unregister_code): call `forget_breakpoint_locations_obj`.

Signed-off-by: Mihails Strasuns <mihails.strasuns@intel.com>
Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
---
M gdb/breakpoint.c
M gdb/breakpoint.h
M gdb/jit.c
A gdb/testsuite/gdb.opencl/multirun.exp
M gdb/testsuite/lib/opencl_hostapp.c
5 files changed, 149 insertions(+), 2 deletions(-)



diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 583f46d..a184e46 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3063,6 +3063,48 @@
   }
 }
 
+/* Tries to find `bp` in the linked list of `b` locations
+   and if found, removes it.  In practice that means that breakpoint
+   will forget about this location and won't try to re-insert it.  */
+
+static bool
+remove_location_from_bp (breakpoint *b, bp_location *bp)
+{
+  if (b->loc == bp)
+    {
+      b->loc = nullptr;
+      return true;
+    }
+
+  bp_location *i;
+  for (i = b->loc; i != nullptr && i->next != bp; i = i->next)
+    ;
+  if (i == nullptr)
+    return false;
+  i->next = i->next->next;
+  return true;
+}
+
+/* See breakpoint.h.  */
+
+int
+forget_breakpoint_locations_obj (objfile *obj)
+{
+  struct bp_location **blp, *bl;
+
+  int count = 0;
+
+  ALL_BP_LOCATIONS (bl, blp)
+    if (bl->symtab != NULL && SYMTAB_OBJFILE (bl->symtab) == obj)
+      {
+	bool ret = remove_location_from_bp (bl->owner, bl);
+	gdb_assert (ret);
+	++count;
+      }
+
+  return count;
+}
+
 static int internal_breakpoint_number = -1;
 
 /* Set the breakpoint number of B, depending on the value of INTERNAL.
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 5c8f17c..5fa99e9 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1405,6 +1405,13 @@
 
 extern void remove_breakpoints_inf (inferior *inf);
 
+/* Forgets all breakpoint locations that are inserted
+   for a given objfile by removing them from the owning breakpoint
+   linked list.  Will not affect actual breakpoints and
+   if the object file remains these may be re-created during next
+   `update_breakpoint_locations` call.  */
+extern int forget_breakpoint_locations_obj (objfile* obj);
+
 /* This function can be used to update the breakpoint package's state
    after an exec() system call has been executed.
 
diff --git a/gdb/jit.c b/gdb/jit.c
index 5014dfd..9c5a92c 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -960,6 +960,11 @@
 {
   if (jit_debug)
     fprintf_unfiltered (gdb_stdlog, "jit_unregister_code (%p)\n", objfile);
+  int count = forget_breakpoint_locations_obj (objfile);
+  if (jit_debug > 1 && count > 0)
+    fprintf_unfiltered (
+	gdb_stdlog,
+	"ignoring %d breakpoint locations in the unregistered code\n", count);
   delete objfile;
 }
 
diff --git a/gdb/testsuite/gdb.opencl/multirun.exp b/gdb/testsuite/gdb.opencl/multirun.exp
new file mode 100644
index 0000000..48e270c
--- /dev/null
+++ b/gdb/testsuite/gdb.opencl/multirun.exp
@@ -0,0 +1,83 @@
+# Copyright 2019 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/>.  */
+#
+# Tests global symbol lookup in case a symbol's name in the kernel
+# coincides with another in the main executable.
+
+load_lib opencl.exp
+
+if { [skip_opencl_tests] } {
+    return 0
+}
+
+set name "opencl_kernel"
+set app [remote_download target ${srcdir}/lib/${name}.cl]
+if { [gdb_compile_opencl_hostapp "${app}" "${name}" "additional_flags=-DRUN_COUNT=2" ] != "" } {
+    untested "failed to compile"
+    return -1
+}
+
+clean_restart ${name}
+
+# Get to the kernel first
+gdb_breakpoint "testkernel" {allow-pending} {temporary}
+gdb_test "set debug jit 2"
+gdb_test "start"
+gdb_continue_to_breakpoint "testkernel"
+
+# First run
+gdb_breakpoint "testkernel"
+set count_first 1
+while 1 {
+   gdb_test_multiple "continue" "first run" {
+      -re "Breakpoint" {
+	  incr count_first +1
+	  pass "expected breakpount"
+      }
+      -re "jit_unregister_code" {
+	pass "end of first run"
+	break 
+      }
+      -re "exited" {
+	fail "unexpected temrination"
+	break
+      }
+   }
+}
+
+# Second run
+set count_second 0
+while 1 {
+   gdb_test_multiple "continue" "second run" {
+      -re "Breakpoint" {
+	  incr count_second +1
+	  pass "expected breakpount"
+      }
+      -re "jit_unregister_code" {
+	pass "end of second run"
+	break 
+      }
+      -re "exited" {
+	fail "unexpected temrination"
+	break
+      }
+   }
+}
+
+if [expr $count_first==$count_second] then {
+    pass "same count of breakpoint hits between two runs"
+} else {
+    fail "${count_first} != ${count_second}"
+}
\ No newline at end of file
diff --git a/gdb/testsuite/lib/opencl_hostapp.c b/gdb/testsuite/lib/opencl_hostapp.c
index 9d6ce0e..7d63d09 100644
--- a/gdb/testsuite/lib/opencl_hostapp.c
+++ b/gdb/testsuite/lib/opencl_hostapp.c
@@ -32,13 +32,17 @@
 #error "Please specify the OpenCL source file using the CL_SOURCE define"
 #endif
 
+#ifndef RUN_COUNT
+#define RUN_COUNT 1
+#endif
+
 #define STRINGIFY(S) _STRINGIFY(S)
 #define _STRINGIFY(S) #S
 
 #define SIZE 16
 
-int
-main ()
+static void
+run_once ()
 {
   int err, i;
   cl_platform_id platform;
@@ -163,6 +167,12 @@
   CHK (clReleaseCommandQueue (queue));
   CHK (clReleaseContext (context));
   free (data);
+}
 
+int
+main ()
+{
+  for (int i = 0; i < RUN_COUNT; ++i)
+    run_once();
   return 0;
 }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 1
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-MessageType: newchange

[review] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 1:

(1 comment)

| --- gdb/breakpoint.c
| +++ gdb/breakpoint.c
| @@ -3066,2 +3091,19 @@ int
| +forget_breakpoint_locations_obj (objfile *obj)
| +{
| +  struct bp_location **blp, *bl;
| +
| +  int count = 0;
| +
| +  ALL_BP_LOCATIONS (bl, blp)
| +    if (bl->symtab != NULL && SYMTAB_OBJFILE (bl->symtab) == obj)
| +      {
| +	bool ret = remove_location_from_bp (bl->owner, bl);

PS1, Line 3100:

There is also an option of reusing existing `shlib_disabled` flag here
instead - jit code is not a shared library but underlying logic should
be quite similar. I don't fully understand possible side effects of
doing so though.

| +	gdb_assert (ret);
| +	++count;
| +      }
| +
| +  return count;
| +}
| +
|  static int internal_breakpoint_number = -1;
|  

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 1
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Comment-Date: Mon, 25 Nov 2019 12:58:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review] jit: remove bp locations when unregistering jit code

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 1:

Just trying to understand the problem better.  From what I understand, when the breakpoint locations must be updated following a code change event (e.g. solib getting loaded or unloaded), we call breakpoint_re_set, which goes through all breakpoint locations and sees if they must be updated.  Have you tryied calling this from jit_unregister_code?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 1
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Mon, 25 Nov 2019 14:40:02 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 1:

> Patch Set 1:
> 
> Just trying to understand the problem better.  From what I understand, when the breakpoint locations must be updated following a code change event (e.g. solib getting loaded or unloaded), we call breakpoint_re_set, which goes through all breakpoint locations and sees if they must be updated.  Have you tryied calling this from jit_unregister_code?

Have tried it now, just in case - it doesn't work. As far as I understand the problem here is that from the gdb PoV breakpoint locations with the same address are the same and `locations_are_equal (existing_locations, b->loc)` condition is true. Because of that updating breakpoint locations won't actually install breakpoint traps - gdb will still think that it is already installed. However in JIT case that instruction memory was overwritten and doesn't have a trap anymore.

Sadly I don't have enough knowledge about gdb architecture to reason if the same problem can possibly manifest with non-jit object files.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 1
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Tue, 26 Nov 2019 11:01:59 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................

jit: remove bp locations when unregistering jit code

Fixes the rare problem when identical JIT code is registered and
unregistered several times and its symbols happen to be relocated to the
exactly the same addresses as previous time.  In such situation gdb would
still think that old breakpoint locations are still inserted (because
`inserted` flag is set and objfile/address is the same) despite the fact
that code memory was overwritten and does not contain a breakpoint trap
anymore.

This solution removes such breakpoint location from the respective
breakpoint location list.  That way next time
`update_breakpoint_locations` is called, it will re-create a new
bp_location instance if that JIT code was loaded again, and won't try
reusing the previous one.  It is a conservative solution which only
applies to object files coming from JIT.

Added test case tries to reproduce the problem using the opencl test
suite (because current jit test suite doesn't actually execute the
code).  Without this patch the breakpoint would only trigger during the first
kernel run and get completely ignored second run.

gdb/ChangeLog:
2019-11-14  Mihails Strasuns  <mihails.strasuns@intel.com>
	* breakpoint.h, breakpoint.c (forget_breakpoint_locations_obj): new
	function to forget all breakpoint locations for a given objfile.
	* jit.c (jit_unregister_code): call `forget_breakpoint_locations_obj`.

Signed-off-by: Mihails Strasuns <mihails.strasuns@intel.com>
Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
---
M gdb/breakpoint.c
M gdb/breakpoint.h
M gdb/jit.c
A gdb/testsuite/gdb.opencl/multirun.exp
M gdb/testsuite/lib/opencl_hostapp.c
5 files changed, 137 insertions(+), 2 deletions(-)



diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 583f46d..a184e46 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3063,6 +3063,48 @@
   }
 }
 
+/* Tries to find `bp` in the linked list of `b` locations
+   and if found, removes it.  In practice that means that breakpoint
+   will forget about this location and won't try to re-insert it.  */
+
+static bool
+remove_location_from_bp (breakpoint *b, bp_location *bp)
+{
+  if (b->loc == bp)
+    {
+      b->loc = nullptr;
+      return true;
+    }
+
+  bp_location *i;
+  for (i = b->loc; i != nullptr && i->next != bp; i = i->next)
+    ;
+  if (i == nullptr)
+    return false;
+  i->next = i->next->next;
+  return true;
+}
+
+/* See breakpoint.h.  */
+
+int
+forget_breakpoint_locations_obj (objfile *obj)
+{
+  struct bp_location **blp, *bl;
+
+  int count = 0;
+
+  ALL_BP_LOCATIONS (bl, blp)
+    if (bl->symtab != NULL && SYMTAB_OBJFILE (bl->symtab) == obj)
+      {
+	bool ret = remove_location_from_bp (bl->owner, bl);
+	gdb_assert (ret);
+	++count;
+      }
+
+  return count;
+}
+
 static int internal_breakpoint_number = -1;
 
 /* Set the breakpoint number of B, depending on the value of INTERNAL.
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 5c8f17c..5fa99e9 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1405,6 +1405,13 @@
 
 extern void remove_breakpoints_inf (inferior *inf);
 
+/* Forgets all breakpoint locations that are inserted
+   for a given objfile by removing them from the owning breakpoint
+   linked list.  Will not affect actual breakpoints and
+   if the object file remains these may be re-created during next
+   `update_breakpoint_locations` call.  */
+extern int forget_breakpoint_locations_obj (objfile* obj);
+
 /* This function can be used to update the breakpoint package's state
    after an exec() system call has been executed.
 
diff --git a/gdb/jit.c b/gdb/jit.c
index 480b459..a897345 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -961,6 +961,11 @@
   if (jit_debug)
     fprintf_unfiltered (gdb_stdlog, "jit_unregister_code (%s)\n",
 			host_address_to_string (objfile));
+  int count = forget_breakpoint_locations_obj (objfile);
+  if (jit_debug > 1 && count > 0)
+    fprintf_unfiltered (
+	gdb_stdlog,
+	"ignoring %d breakpoint locations in the unregistered code\n", count);
   delete objfile;
 }
 
diff --git a/gdb/testsuite/gdb.opencl/multirun.exp b/gdb/testsuite/gdb.opencl/multirun.exp
new file mode 100644
index 0000000..e0dcbcc
--- /dev/null
+++ b/gdb/testsuite/gdb.opencl/multirun.exp
@@ -0,0 +1,71 @@
+# Copyright 2019 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/>.  */
+#
+# Tests global symbol lookup in case a symbol's name in the kernel
+# coincides with another in the main executable.
+
+load_lib opencl.exp
+
+if { [skip_opencl_tests] } {
+    return 0
+}
+
+set name "opencl_kernel"
+set app [remote_download target ${srcdir}/lib/${name}.cl]
+if { [gdb_compile_opencl_hostapp "${app}" "${name}" "additional_flags=-DRUN_COUNT=2" ] != "" } {
+    untested "failed to compile"
+    return -1
+}
+
+clean_restart ${name}
+
+# Get to the kernel first
+gdb_breakpoint "testkernel" {allow-pending} {temporary}
+gdb_test "set debug jit 2"
+gdb_test "start"
+gdb_continue_to_breakpoint "testkernel"
+
+
+set counts(0) 0
+set counts(1) 0
+set i 0
+
+gdb_breakpoint "testkernel"
+send_gdb "continue\n"
+
+while 1 {
+   gdb_expect {
+      -re "Breakpoint" {
+	  incr counts($i)
+	  send_gdb "continue\n"
+      }
+      -re "jit_unregister_code" {
+	  incr i
+      }
+      -re "exited" {
+	  break
+      }
+      timeout {
+	  perror "test hang while executing continue"
+	  return -1
+      }
+   }
+}
+
+if [expr $counts(0)==$counts(1)] then {
+    pass "same count of breakpoint hits between two runs"
+} else {
+    fail "${counts(0)} != ${counts(1)}"
+}
\ No newline at end of file
diff --git a/gdb/testsuite/lib/opencl_hostapp.c b/gdb/testsuite/lib/opencl_hostapp.c
index 9d6ce0e..7d63d09 100644
--- a/gdb/testsuite/lib/opencl_hostapp.c
+++ b/gdb/testsuite/lib/opencl_hostapp.c
@@ -32,13 +32,17 @@
 #error "Please specify the OpenCL source file using the CL_SOURCE define"
 #endif
 
+#ifndef RUN_COUNT
+#define RUN_COUNT 1
+#endif
+
 #define STRINGIFY(S) _STRINGIFY(S)
 #define _STRINGIFY(S) #S
 
 #define SIZE 16
 
-int
-main ()
+static void
+run_once ()
 {
   int err, i;
   cl_platform_id platform;
@@ -163,6 +167,12 @@
   CHK (clReleaseCommandQueue (queue));
   CHK (clReleaseContext (context));
   free (data);
+}
 
+int
+main ()
+{
+  for (int i = 0; i < RUN_COUNT; ++i)
+    run_once();
   return 0;
 }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: newpatchset

[review v2] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

Update:
- improved the test case


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Tue, 26 Nov 2019 11:29:28 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

> Patch Set 1:
> 
> > Patch Set 1:
> > 
> > Just trying to understand the problem better.  From what I understand, when the breakpoint locations must be updated following a code change event (e.g. solib getting loaded or unloaded), we call breakpoint_re_set, which goes through all breakpoint locations and sees if they must be updated.  Have you tryied calling this from jit_unregister_code?
> 
> Have tried it now, just in case - it doesn't work. As far as I understand the problem here is that from the gdb PoV breakpoint locations with the same address are the same and `locations_are_equal (existing_locations, b->loc)` condition is true. Because of that updating breakpoint locations won't actually install breakpoint traps - gdb will still think that it is already installed. However in JIT case that instruction memory was overwritten and doesn't have a trap anymore.
> 
> Sadly I don't have enough knowledge about gdb architecture to reason if the same problem can possibly manifest with non-jit object files.

There's still something that doesn't connect in my mind.  I may have a wrong picture of what's happening, so please correct me.

From my understanding of the JIT interface (https://sourceware.org/gdb/current/onlinedocs/gdb/JIT-Interface.html), unregistering code is made by the process calling __jit_debug_register_code (on which GDB has a special breakpoint) with action == JIT_UNREGISTER.  Registering code is made by the process calling __jit_debug_register_code with action == JIT_REGISTER.

So to unregister a jit region and register a new one (that would happen to have the exact same code at the exact same address as the previous one), the process would need to call __jit_debug_register_code twice, executing between the two events.  After unregistration, when the execution resumes, shouldn't there be something that deletes the breakpoint locations related to that objfile that was removed?  And then when __jit_debug_register_code for registering the new object, we would re-create brand new breakpoint locations?

Or is it that somehow, the unregistration and re-registration is made during the same stop?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Tue, 26 Nov 2019 16:58:51 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

> So to unregister a jit region and register a new one (that would happen to have the exact same code at the exact same address as the previous one), the process would need to call __jit_debug_register_code twice, executing between the two events.

Correct.

> After unregistration, when the execution resumes, shouldn't there be something that deletes the breakpoint locations related to that objfile that was removed?  And then when __jit_debug_register_code for registering the new object, we would re-create brand new breakpoint locations?

To be honest I hoped someone here will be able to explain this to me :)

But no, this is not how gdb works right now - removal of object files does not cause existing breakpoint locations to be removed, it only clears matching symtab reference (see `breakpoint_free_objfile`). Normally it doesn't cause any problems because next time the same object file appears again, a new breakpoint location gets created/inserted with the new address.

The only possible explanation I could think of is that it is somehow possible for objfile instance to be deleted with the matching instruction memory still being accessible - and that such breakpoint location is still expected to hit. But some sort of historical quirk is always possible of course.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Tue, 26 Nov 2019 17:07:46 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Luis Machado (Code Review)]

Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

Technically, when we're deleting an objfile during JIT_UNREGISTER handling, we also call "breakpoint_free_objfile" as part of the objfile destructor.

That function should take care of removing said locations, no? Is something off with that particular function that is preventing GDB from updating the list of locations?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 27 Nov 2019 01:38:19 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

> That function should take care of removing said locations, no? Is something off with that particular function that is preventing GDB from updating the list of locations?

It doesn't currently remove or disable locations in any way, only clears symtab references:

~~~
void
breakpoint_free_objfile (struct objfile *objfile)
{
  struct bp_location **locp, *loc;

  ALL_BP_LOCATIONS (loc, locp)
    if (loc->symtab != NULL && SYMTAB_OBJFILE (loc->symtab) == objfile)
      loc->symtab = NULL;
}
~~~


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 27 Nov 2019 08:22:48 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Luis Machado (Code Review)]

Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

> Patch Set 2:
> 
> > That function should take care of removing said locations, no? Is something off with that particular function that is preventing GDB from updating the list of locations?
> 
> It doesn't currently remove or disable locations in any way, only clears symtab references:

Should it do so then? I'm not too familiar with the whole jit scheme in GDB, but it sounds like the breakpoint locations should go away when we free the objfile?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 27 Nov 2019 12:44:35 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

> Should it do so then? I'm not too familiar with the whole jit scheme in GDB, but it sounds like the breakpoint locations should go away when we free the objfile?

See my response to Simon above - I was hoping that someone who knows it more can chime in and clarify this bit. The current behavior looks wrong indeed but I am hesitant to change generic objfile behavior without any confirmation from someone who knows this better.

The fact that there is currently a dedicated flag for a similar shared library case makes me suspect it may be intentional (otherwise it could just use generic objfile behavior).

The `breakpoint_free_objfile` was introduced in 2f202fde0a4586f88b98439b436e7b0bb1336b26 long time ago and I don't see any rationale mentioned there.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 27 Nov 2019 12:58:57 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Luis Machado (Code Review)]

Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

I think Pedro and/or Tom may have a better idea of whether we can drop the breakpoint locations associated with the to-be-unregistered JIT objfile.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 27 Nov 2019 13:40:26 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

> See my response to Simon above - I was hoping that someone who knows it more can chime in and clarify this bit. The current behavior looks wrong indeed but I am hesitant to change generic objfile behavior without any confirmation from someone who knows this better.
> 
> The fact that there is currently a dedicated flag for a similar shared library case makes me suspect it may be intentional (otherwise it could just use generic objfile behavior).
> 
> The `breakpoint_free_objfile` was introduced in 2f202fde0a4586f88b98439b436e7b0bb1336b26 long time ago and I don't see any rationale mentioned there.

It would really help to have a reproducer that runs on standard x86-64, so we can play with it and experiment.

You said that the test suite doesn't execute jit-ed code, but it seems like we do execute some jit-ed code in the jitreader.exp test (the code is jit-ed and executed in jithost.c).  It might not be too hard to augment it to reproduce the bug (even if it's just hackish at first)?

Otherwise, jit-main.c (for jit.exp) takes the approach of copying an ELF file into memory (and adjusting some addresses).  It might be possible to lookup a function symbol in this mapped ELF file and execute it?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Thu, 28 Nov 2019 05:09:53 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

Sorry for the delayed response, had switch to other issues.

> You said that the test suite doesn't execute jit-ed code, but it seems like we do execute some jit-ed code in the jitreader.exp test (the code is jit-ed and executed in jithost.c).  It might not be too hard to augment it to reproduce the bug (even if it's just hackish at first)?
> 
> Otherwise, jit-main.c (for jit.exp) takes the approach of copying an ELF file into memory (and adjusting some addresses).  It might be possible to lookup a function symbol in this mapped ELF file and execute it?

I was able to tweak jit-main.c to adjust address for jit_function_XXXX and execute it but to reproduce the bug I'd also need to adjust addresses in debug sections too (so that `break jit_function_XXXX` resolves to the correct address) and that seems much more effort.

Same applies to jithost.c - its jit reader does not populate debug symbols right now and it is not possible to put a breakpoint in jit function (which is why it uses hard-coded trap).

It is still something I'd really like to make work because currently there is no easy testing facility for more complex jit tests in gdb. Maybe reusing dwarf processing bits from gdb itself can make it feasible, will have to give it a try.

Note though that you can run my current opencl test case on a regular x86_64, it is intended to work with CPU opencl runtime (for example https://software.intel.com/en-us/articles/opencl-drivers).


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Tue, 10 Dec 2019 15:22:12 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

> Sorry for the delayed response, had switch to other issues.
> 
> > You said that the test suite doesn't execute jit-ed code, but it seems like we do execute some jit-ed code in the jitreader.exp test (the code is jit-ed and executed in jithost.c).  It might not be too hard to augment it to reproduce the bug (even if it's just hackish at first)?
> > 
> > Otherwise, jit-main.c (for jit.exp) takes the approach of copying an ELF file into memory (and adjusting some addresses).  It might be possible to lookup a function symbol in this mapped ELF file and execute it?
> 
> I was able to tweak jit-main.c to adjust address for jit_function_XXXX and execute it but to reproduce the bug I'd also need to adjust addresses in debug sections too (so that `break jit_function_XXXX` resolves to the correct address) and that seems much more effort.

Hmm indeed I see the problem, relocating the DWARF info would be tricky.

> Same applies to jithost.c - its jit reader does not populate debug symbols right now and it is not possible to put a breakpoint in jit function (which is why it uses hard-coded trap).

Ok, that's odd.  I would expect that when defining a block with a name (like jitreader.c does with "jit_function_00"), it would define a symbol on which you can put a breakpoint.  But it apparently does not.

I'm a bit lost in how all this interacts with opencl though.  Can you explain how the jit comes in play with opencl?  Is there a jit reader for opencl?  In the tests, how can we put a breakpoint on "testkernel", where does that symbol come from?
 
> It is still something I'd really like to make work because currently there is no easy testing facility for more complex jit tests in gdb. Maybe reusing dwarf processing bits from gdb itself can make it feasible, will have to give it a try.

I agree, the more advanced use cases need to be tested.

> Note though that you can run my current opencl test case on a regular x86_64, it is intended to work with CPU opencl runtime (for example https://software.intel.com/en-us/articles/opencl-drivers).

I was able to build and run a simple "hello world" opencl example, but unfortunately I'm unable to run any of the opencl tests in GDB.  I get:

 128 (gdb) tbreak testkernel^M
 129 Function "testkernel" not defined.^M
 130 Make breakpoint pending on future shared library load? (y or [n]) y^M
 131 Temporary breakpoint 1 (testkernel) pending.^M

and the breakpoint is never resolved and hit, the inferior just exits.

I'm not against adding opencl tests, but I think if we can reproduce and test this issue with a plain x86-64 test, it's much more valuable, as nearly everybody will run this test out of the box, so we have more changes of catching any regression.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 11 Dec 2019 05:51:30 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

> I'm a bit lost in how all this interacts with opencl though.  Can you explain how the jit comes in play with opencl?  Is there a jit reader for opencl?  In the tests, how can we put a breakpoint on "testkernel", where does that symbol come from?

I am only using it as an easy way to get already relocated elf binary with the debug info present. The way it works with the CPU runtime I am using locally is that compiling opencl kernel generates elf which is loaded into memory and passed via __jit_debug_register_code - pretty much the same behavior you would get with hypothetically adjusted jit-main.c

But indeed this is an implementation detail and cannot be relied on for a general purpose test case :/

> > It is still something I'd really like to make work because currently there is no easy testing facility for more complex jit tests in gdb. Maybe reusing dwarf processing bits from gdb itself can make it feasible, will have to give it a try.
> 
> I agree, the more advanced use cases need to be tested.

I will try to come up with something but can't give any ETA right now.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 11 Dec 2019 09:24:22 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

> Patch Set 2:
> 
> > I'm a bit lost in how all this interacts with opencl though.  Can you explain how the jit comes in play with opencl?  Is there a jit reader for opencl?  In the tests, how can we put a breakpoint on "testkernel", where does that symbol come from?
> 
> I am only using it as an easy way to get already relocated elf binary with the debug info present. The way it works with the CPU runtime I am using locally is that compiling opencl kernel generates elf which is loaded into memory and passed via __jit_debug_register_code - pretty much the same behavior you would get with hypothetically adjusted jit-main.c

Ok, so IIUC, you are using a special OpenCL runtime that happens to use the GDB JIT interface?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 11 Dec 2019 16:19:33 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

> Patch Set 2:
> Ok, so IIUC, you are using a special OpenCL runtime that happens to use the GDB JIT interface?

Yes. I think it uses LLVM JIT infrastructure internally which is the one responsible __jit_debug_register_code call.

It should be quite similar with the one I have linked though. Have just noticed that I had another old local patch to enable debug info for opencl:

diff --git a/gdb/testsuite/lib/opencl_hostapp.c b/gdb/testsuite/lib/opencl_hostapp.c
index 9d6ce0eacb..23da3ad1f6 100644
--- a/gdb/testsuite/lib/opencl_hostapp.c
+++ b/gdb/testsuite/lib/opencl_hostapp.c
@@ -86,7 +86,7 @@ main ()
   device_extensions = (char *) malloc (len);
   CHK (clGetDeviceInfo (device, CL_DEVICE_EXTENSIONS, len, device_extensions,
                        NULL));
-  strcpy (kernel_build_opts, "-Werror -cl-opt-disable");
+  strcpy (kernel_build_opts, "-Werror -g -cl-opt-disable");
   if (strstr (device_extensions, "cl_khr_fp64") != NULL)
     strcpy (kernel_build_opts + strlen (kernel_build_opts),
            " -D HAVE_cl_khr_fp64");

With this change you may be able to trigger a pending breakpoint on the kernel function.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Thu, 12 Dec 2019 09:04:58 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

> Patch Set 2:
> 
> > Patch Set 2:
> > Ok, so IIUC, you are using a special OpenCL runtime that happens to use the GDB JIT interface?
> 
> Yes. I think it uses LLVM JIT infrastructure internally which is the one responsible __jit_debug_register_code call.
> 
> It should be quite similar with the one I have linked though. Have just noticed that I had another old local patch to enable debug info for opencl:

Hmm sorry, what did you link to?

> 
> diff --git a/gdb/testsuite/lib/opencl_hostapp.c b/gdb/testsuite/lib/opencl_hostapp.c
> index 9d6ce0eacb..23da3ad1f6 100644
> --- a/gdb/testsuite/lib/opencl_hostapp.c
> +++ b/gdb/testsuite/lib/opencl_hostapp.c
> @@ -86,7 +86,7 @@ main ()
>    device_extensions = (char *) malloc (len);
>    CHK (clGetDeviceInfo (device, CL_DEVICE_EXTENSIONS, len, device_extensions,
>                         NULL));
> -  strcpy (kernel_build_opts, "-Werror -cl-opt-disable");
> +  strcpy (kernel_build_opts, "-Werror -g -cl-opt-disable");
>    if (strstr (device_extensions, "cl_khr_fp64") != NULL)
>      strcpy (kernel_build_opts + strlen (kernel_build_opts),
>             " -D HAVE_cl_khr_fp64");
> 
> With this change you may be able to trigger a pending breakpoint on the kernel function.

No chance, I still see the breakpoint not getting resolved.  I suppose that this is very dependent on the opencl implemention one is using?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Sun, 15 Dec 2019 02:41:07 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

FYI: I came up with a stupid enough hack to make dwarf adjustment work but after rebasing on latest master I see a rather different behavior for breakpoints. Will investigate that and hopefully update the patch in a few days.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 18 Dec 2019 16:36:59 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] jit: remove bp locations when unregistering jit code

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 2:

> Patch Set 2:
> 
> FYI: I came up with a stupid enough hack to make dwarf adjustment work but after rebasing on latest master I see a rather different behavior for breakpoints. Will investigate that and hopefully update the patch in a few days.

Ok, and now that I'm a bit more familiar with the JIT interface/subsystem, it is also on my todo list to try to reproduce the issue using plain x86-64 code.  When using the JIT debug reader interface, it's possible to give symtabs names (e.g. "foo.c"), add line <-> mappings, and then place breakpoints by line (e.g. "foo.c:4").  I intend to try it like this first.

I also intend to look into why it's not possible to place breakpoints on the symbols defined by the JIT debug reader interface.  I find it surprising, and think it would make sense / be helpful to be able to do so.  If we make it possible, then we'll have a second way to try to reproduce this bug, by placing breakpoints by function name rather than line.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 2
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 18 Dec 2019 17:32:51 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v3] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................

jit: remove bp locations when unregistering jit code

Fixes the rare problem when identical JIT code is registered and
unregistered several times and its symbols happen to be relocated to the
exactly the same addresses as previous time.  In such situation gdb would
still think that old breakpoint locations are still inserted (because
`inserted` flag is set and objfile/address is the same) despite the fact
that code memory was overwritten and does not contain a breakpoint trap
anymore.

This solution removes such breakpoint location from the respective
breakpoint location list.  That way next time
`update_breakpoint_locations` is called, it will re-create a new
bp_location instance if that JIT code was loaded again, and won't try
reusing the previous one.  It is a conservative solution which only
applies to object files coming from JIT.

Added test case tries to reproduce the problem using the opencl test
suite (because current jit test suite doesn't actually execute the
code).  Without this patch the breakpoint would only trigger during the first
kernel run and get completely ignored second run.

gdb/ChangeLog:
2019-11-14  Mihails Strasuns  <mihails.strasuns@intel.com>
	* breakpoint.h, breakpoint.c (forget_breakpoint_locations_obj): new
	  function to forget all breakpoint locations for a given objfile.
	* jit.c (jit_unregister_code): call `forget_breakpoint_locations_obj`.

gdb/testsuite/ChangeLog:
2019-11-14  Mihails Strasuns  <mihails.strasuns@intel.com>
	* jit-reregister.exp, jit-reregister.c: new test case to verify that
	  breakpoint is hit even when JIT code is unregistered and registered
	  again.

Signed-off-by: Mihails Strasuns <mihails.strasuns@intel.com>
Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
---
M gdb/breakpoint.c
M gdb/breakpoint.h
M gdb/jit.c
A gdb/testsuite/gdb.base/jit-reregister.c
A gdb/testsuite/gdb.base/jit-reregister.exp
5 files changed, 181 insertions(+), 9 deletions(-)



diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 904abda..ab1139d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3066,6 +3066,50 @@
   }
 }
 
+/* Tries to find `bp` in the linked list of `b` locations
+   and if found, removes it.  In practice that means that breakpoint
+   will forget about this location and won't try to re-insert it.  */
+
+static bool
+remove_location_from_bp (breakpoint *b, bp_location *bp)
+{
+  if (b->loc == bp)
+    {
+      b->loc = nullptr;
+      return true;
+    }
+
+  bp_location *i;
+  for (i = b->loc; i != nullptr && i->next != bp; i = i->next)
+    ;
+  if (i == nullptr)
+    return false;
+  i->next = i->next->next;
+  return true;
+}
+
+/* See breakpoint.h.  */
+
+int
+forget_breakpoint_locations_obj (objfile *obj)
+{
+  struct bp_location **blp, *bl;
+
+  int count = 0;
+
+  ALL_BP_LOCATIONS (bl, blp)
+  {
+    if (is_addr_in_objfile (bl->address, obj))
+      {
+	bool ret = remove_location_from_bp (bl->owner, bl);
+	gdb_assert (ret);
+	++count;
+      }
+  }
+
+  return count;
+}
+
 static int internal_breakpoint_number = -1;
 
 /* Set the breakpoint number of B, depending on the value of INTERNAL.
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 3197854..9105970 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1411,6 +1411,13 @@
 
 extern void remove_breakpoints_inf (inferior *inf);
 
+/* Forgets all breakpoint locations that are inserted
+   for a given objfile by removing them from the owning breakpoint
+   linked list.  Will not affect actual breakpoints and
+   if the object file remains these may be re-created during next
+   `update_breakpoint_locations` call.  */
+extern int forget_breakpoint_locations_obj (objfile* obj);
+
 /* This function can be used to update the breakpoint package's state
    after an exec() system call has been executed.
 
diff --git a/gdb/jit.c b/gdb/jit.c
index 0dd11e1..6ed5f51 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -923,6 +923,32 @@
   return NULL;
 }
 
+/* This function unregisters JITed code and does the necessary cleanup.  */
+
+static void
+jit_unregister_code (struct gdbarch *gdbarch, CORE_ADDR entry_addr)
+{
+  struct objfile *objfile = nullptr;
+  if (jit_debug)
+    fprintf_unfiltered (gdb_stdlog, "jit_unregister_code (%s)\n",
+			host_address_to_string (objfile));
+  objfile = jit_find_objf_with_entry_addr (entry_addr);
+  if (objfile == NULL)
+    printf_unfiltered (_ ("Unable to find JITed code "
+			  "entry at address: %s\n"),
+		       paddress (gdbarch, entry_addr));
+  else
+    {
+      int count = forget_breakpoint_locations_obj (objfile);
+      if (jit_debug && count > 0)
+	fprintf_unfiltered (gdb_stdlog,
+			    "jit_unregister_code, ignoring %d breakpoint"
+			    "locations in the unregistered code\n",
+			    count);
+      objfile->unlink ();
+    }
+}
+
 /* This is called when a breakpoint is deleted.  It updates the
    inferior's cache, if needed.  */
 
@@ -1333,7 +1359,6 @@
   struct jit_descriptor descriptor;
   struct jit_code_entry code_entry;
   CORE_ADDR entry_addr;
-  struct objfile *objf;
 
   /* Read the descriptor from remote memory.  */
   if (!jit_read_descriptor (gdbarch, &descriptor,
@@ -1351,14 +1376,7 @@
       jit_register_code (gdbarch, entry_addr, &code_entry);
       break;
     case JIT_UNREGISTER:
-      objf = jit_find_objf_with_entry_addr (entry_addr);
-      if (objf == NULL)
-	printf_unfiltered (_("Unable to find JITed code "
-			     "entry at address: %s\n"),
-			   paddress (gdbarch, entry_addr));
-      else
-	objf->unlink ();
-
+      jit_unregister_code (gdbarch, entry_addr);
       break;
     default:
       error (_("Unknown action_flag value in JIT descriptor!"));
diff --git a/gdb/testsuite/gdb.base/jit-reregister.c b/gdb/testsuite/gdb.base/jit-reregister.c
new file mode 100644
index 0000000..3d63f14
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-reregister.c
@@ -0,0 +1,58 @@
+/* This test program is part of GDB, the GNU debugger.
+
+   Copyright 2019 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 "jit-defs.h"
+#include "jit-elf.h"
+
+int
+main (int argc, char *argv[])
+{
+  size_t obj_size;
+  ElfW (Addr) addr = load_elf (argv[1], &obj_size, 0);
+  update_locations (addr, -1);
+  int (*jit_function) ()
+      = (int (*) ()) load_symbol (addr, "jit_function_XXXX");
+
+  struct jit_code_entry *const entry
+      = (struct jit_code_entry *) calloc (1, sizeof (*entry));
+
+  entry->symfile_addr = (const char *) addr;
+  entry->symfile_size = obj_size;
+  entry->prev_entry = __jit_debug_descriptor.relevant_entry;
+  __jit_debug_descriptor.relevant_entry = entry;
+  __jit_debug_descriptor.first_entry = entry;
+  __jit_debug_descriptor.action_flag = JIT_REGISTER_FN;
+  __jit_debug_register_code ();
+
+  jit_function (); /* first-call */
+
+  __jit_debug_descriptor.action_flag = JIT_UNREGISTER_FN;
+  __jit_debug_register_code ();
+
+  addr = load_elf (argv[1], &obj_size, addr);
+  update_locations (addr, -1);
+  jit_function = (int (*) ()) load_symbol (addr, "jit_function_XXXX");
+
+  entry->symfile_addr = (const char *) addr;
+  entry->symfile_size = obj_size;
+  __jit_debug_descriptor.relevant_entry = entry;
+  __jit_debug_descriptor.first_entry = entry;
+  __jit_debug_descriptor.action_flag = JIT_REGISTER_FN;
+  __jit_debug_register_code ();
+
+  jit_function ();
+}
diff --git a/gdb/testsuite/gdb.base/jit-reregister.exp b/gdb/testsuite/gdb.base/jit-reregister.exp
new file mode 100644
index 0000000..23de587
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-reregister.exp
@@ -0,0 +1,45 @@
+# Copyright 2019 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/>.  */
+#
+# Tests global symbol lookup in case a symbol's name in the kernel
+# coincides with another in the main executable.
+
+standard_testfile .c
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+set solib "jit-solib"
+set solib_src "${srcdir}/${subdir}/${solib}.c"
+set solib_bin [standard_output_file ${solib}.so]
+
+if { [gdb_compile_shlib "${solib_src}" "${solib_bin}" {debug}] != "" } {
+    untested "failed to compile jit shared library"
+    return -1
+}
+
+set solib_bin_target [gdb_remote_download target ${solib_bin}]
+
+clean_restart ${binfile}
+
+gdb_test "start ${solib_bin_target}"
+gdb_breakpoint [gdb_get_line_number "first-call"] {temporary}
+gdb_test "set debug jit 1"
+gdb_continue_to_breakpoint "first-call"
+
+gdb_breakpoint "jit_function_XXXX"
+gdb_continue_to_breakpoint "jit_function_XXXX"
+gdb_continue_to_breakpoint "jit_function_XXXX"
\ No newline at end of file

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 3
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: newpatchset

[review v3] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 3:

> Ok, and now that I'm a bit more familiar with the JIT interface/subsystem, it is also on my todo list to try to reproduce the issue using plain x86-64 code.  When using the JIT debug reader interface, it's possible to give symtabs names (e.g. "foo.c"), add line <-> mappings, and then place breakpoints by line (e.g. "foo.c:4").  I intend to try it like this first.

I have pursued elf adjustment approach in the meanwhile - please check another patch in the chain for jit-main.c enhancement and refactoring. It is a somewhat stupid approach for blindly updating all mentions of the original symbol address in the loaded binary (so for example line info remain broken) but it is sufficient to break on functions and reproduce the problem.

The added jit-reregister.exp test case should fail if the call to `forget_breakpoint_locations_obj` is commented out. Hope that will help.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 3
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Thu, 19 Dec 2019 10:33:51 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v3] jit: remove bp locations when unregistering jit code

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704
......................................................................


Patch Set 3:

Ping


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1
Gerrit-Change-Number: 704
Gerrit-PatchSet: 3
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Mon, 13 Jan 2020 08:39:33 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment