public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Simon Marchi <simark@simark.ca>
To: "Strasuns, Mihails" <mihails.strasuns@intel.com>,
	"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: [PATCH 2/2] jit: remove bp locations when unregistering jit code
Date: Sat, 6 Jun 2020 23:41:55 -0400	[thread overview]
Message-ID: <ffc6137d-6ea6-c7a2-ae04-a23ecf97ff3d@simark.ca> (raw)
In-Reply-To: <MWHPR11MB004873DF470129FE2025931D95B30@MWHPR11MB0048.namprd11.prod.outlook.com>

On 2020-05-25 6:07 a.m., Strasuns, Mihails via Gdb-patches wrote:
> Hello,
> 
> I will focus on the design related question for now:
> 
>> -----Original Message-----
>> From: Simon Marchi <simark@simark.ca>
>> Sent: Sunday, May 24, 2020 5:02 PM
>> To: Strasuns, Mihails <mihails.strasuns@intel.com>; gdb-
>> patches@sourceware.org
>> Subject: Re: [PATCH 2/2] jit: remove bp locations when unregistering jit code
>>
>> I'm not very familiar with how breakpoint locations are computed and
>> updated (I just know it's complicated). But I confirm that the test shows the
>> problem without the fix applied and passes with the fix applied.  I'd like if
>> some other maintainer more familiar with this area could review this part.
>>
>> I'd be really curious to understand how GDB handles this when it comes to
>> shared libraries.  Let's imagine the same case, but using shared libraries.
>> When a shared library is unloaded, the breakpoint locations that were in that
>> library must be forgotten somehow?  If so through which hook / mechanism?
> 
> This is something I have initially looked into myself didn't find a clear answer in GDB sources :-)
> 
> bp_location defines this field:
> 
> ---
>   /* This location's address is in an unloaded solib, and so this
>      location should not be inserted.  It will be automatically
>      enabled when that solib is loaded.  */
>   bool shlib_disabled = false;
> ---
> 
> What I don't understand though is that once set to true/1, this field is never reset back to 0. My guess is that this is intended to work in a similar way to my fix, i.e. that a new breakpoint location gets created for the new load rather than the old one being recreated. However I am not entirely sure.

I really don't know why, when a shared library containing a location gets
unloaded, we just mark the location as "disabled" and leave it there.  I
really don't see the point of leaving it there.  Maybe there's a comment
somewhere that explains why we do this instead of removing it, but I didn't
find any.

I also find it odd that when a breakpoint location gets disabled, we print
the message "Temporarily disabling breakpoints for $solib".  Why "Temporarily"?
If the same shared lib gets re-loaded, it might or might not be at the same
address, so it's not like the breakpoint location is going to be reused anyway.

I think that's something we can investigate separately from the current patch,
maybe for now we can just disable the locations instead of removing them, to
keep it consistent with shared libs.

> There is a fair amount of differences between semantics of jit and shared libraries, so I decided to start with an independent fix and ask in this mail list for someone who knows better.
> 
> Two important notes:
> 
> - there is a `disable_breakpoints_in_freed_objfile` hook in breakpoint.c which looks like it should have covered this case but somehow it is not enough.

Indeed, that sounds like a promising function that sounds like it should
do what we want (and it does already get called when a jit object gets
unloaded).  But it returns early because of this:

  /* OBJF_SHARED|OBJF_USERLOADED objfiles are dynamic modules manually
     managed by the user with add-symbol-file/remove-symbol-file.
     Similarly to how breakpoints in shared libraries are handled in
     response to "nosharedlibrary", mark breakpoints in such modules
     shlib_disabled so they end up uninserted on the next global
     location list update.  Shared libraries not loaded by the user
     aren't handled here -- they're already handled in
     disable_breakpoints_in_unloaded_shlib, called by solib.c's
     solib_unloaded observer.  We skip objfiles that are not
     OBJF_SHARED as those aren't considered dynamic objects (e.g. the
     main objfile).  */
  if ((objfile->flags & OBJF_SHARED) == 0
      || (objfile->flags & OBJF_USERLOADED) == 0)
    return;

So it sounds like this function considers that because shared libraries
where already taken care of in disable_breakpoints_in_unloaded_shlib, all
that remains is user-loaded objfiles.  But that's not true, there are
jit-loaded objfiles, which are neither user-loaded nor shared libraries.

Looking at disable_breakpoints_in_unloaded_shlib and
disable_breakpoints_in_freed_objfile, I noticed they do make sure that the
loc's pspace is the same as the unloaded shlib / freed objfile's pspace, which
you don't do in your patch.  I just tried with your patch:

- Start two inferiors with one jit object each
- Create a breakpoint with two locations (one in each jit object)
- Step one inferior so that it unloads its jit object

It ended up deleting the breakpoint locations for the two inferiors, whereas
it should have left the location for the inferior that still has its jit object
loaded.  It would be good to enhance the new test case to test that case.

To avoid trying to re-invent the wheel and making the same subtle mistakes that
are (hopefully) already solved for the shared library case, I think we should
try to re-use the same code path for disabling breakpoint locations when either
a shared library or jit object is unloaded.

What about something like this, which makes unloaded jit objects go through
the same breakpoint location disabling function as the unloaded shared libs?


From 6e3a377dc8181ec1ce430711d48037f513b153be Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Sat, 6 Jun 2020 22:47:31 -0400
Subject: [PATCH] Disable breakpoint locations in unloaded jit objects

Change-Id: I06ff4f3f02b3c348165ed451339e6effba6e3e97
---
 gdb/breakpoint.c | 29 +++++++++++++++++++++--------
 gdb/jit.c        | 31 ++++++++++++++++++++++---------
 gdb/observable.c |  1 +
 gdb/observable.h |  2 ++
 4 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index aead882acd88..cc15c4730364 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7487,12 +7487,8 @@ disable_breakpoints_in_shlibs (void)
   }
 }

-/* Disable any breakpoints and tracepoints that are in SOLIB upon
-   notification of unloaded_shlib.  Only apply to enabled breakpoints,
-   disabled ones can just stay disabled.  */
-
 static void
-disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
+disable_breakpoints_in_unloaded_objfile (objfile *objfile)
 {
   struct bp_location *loc, **locp_tmp;
   int disabled_shlib_breaks = 0;
@@ -7502,7 +7498,7 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
     /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.  */
     struct breakpoint *b = loc->owner;

-    if (solib->pspace == loc->pspace
+    if (objfile->pspace == loc->pspace
 	&& !loc->shlib_disabled
 	&& (((b->type == bp_breakpoint
 	      || b->type == bp_jit_event
@@ -7510,7 +7506,7 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
 	     && (loc->loc_type == bp_loc_hardware_breakpoint
 		 || loc->loc_type == bp_loc_software_breakpoint))
 	    || is_tracepoint (b))
-	&& solib_contains_address_p (solib, loc->address))
+	&& is_addr_in_objfile (loc->address, objfile))
       {
 	loc->shlib_disabled = 1;
 	/* At this point, we cannot rely on remove_breakpoint
@@ -7526,13 +7522,29 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
 	    target_terminal::ours_for_output ();
 	    warning (_("Temporarily disabling breakpoints "
 		       "for unloaded shared library \"%s\""),
-		     solib->so_name);
+		     objfile->original_name);
 	  }
 	disabled_shlib_breaks = 1;
       }
   }
 }

+/* Disable any breakpoints and tracepoints that are in SOLIB upon
+   notification of unloaded_shlib.  Only apply to enabled breakpoints,
+   disabled ones can just stay disabled.  */
+
+static void
+disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
+{
+  disable_breakpoints_in_unloaded_objfile (solib->objfile);
+}
+
+static void
+disable_breakpoints_in_unloaded_jit_object (objfile *objfile)
+{
+  disable_breakpoints_in_unloaded_objfile (objfile);
+}
+
 /* Disable any breakpoints and tracepoints in OBJFILE upon
    notification of free_objfile.  Only apply to enabled breakpoints,
    disabled ones can just stay disabled.  */
@@ -15435,6 +15447,7 @@ _initialize_breakpoint ()
   initialize_breakpoint_ops ();

   gdb::observers::solib_unloaded.attach (disable_breakpoints_in_unloaded_shlib);
+  gdb::observers::jit_object_unloaded.attach (disable_breakpoints_in_unloaded_jit_object);
   gdb::observers::free_objfile.attach (disable_breakpoints_in_freed_objfile);
   gdb::observers::memory_changed.attach (invalidate_bp_value_on_memory_change);

diff --git a/gdb/jit.c b/gdb/jit.c
index 1b5ef46469e0..16e38078bbd0 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -925,6 +925,27 @@ jit_find_objf_with_entry_addr (CORE_ADDR entry_addr)
   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
+    {
+      gdb::observers::jit_object_unloaded.notify (objfile);
+      objfile->unlink ();
+    }
+}
+
 /* This is called when a breakpoint is deleted.  It updates the
    inferior's cache, if needed.  */

@@ -1335,7 +1356,6 @@ jit_event_handler (struct gdbarch *gdbarch)
   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,
@@ -1353,14 +1373,7 @@ jit_event_handler (struct gdbarch *gdbarch)
       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/observable.c b/gdb/observable.c
index 81aa392cc21f..92354c64a366 100644
--- a/gdb/observable.c
+++ b/gdb/observable.c
@@ -46,6 +46,7 @@ DEFINE_OBSERVABLE (inferior_created);
 DEFINE_OBSERVABLE (record_changed);
 DEFINE_OBSERVABLE (solib_loaded);
 DEFINE_OBSERVABLE (solib_unloaded);
+DEFINE_OBSERVABLE (jit_object_unloaded);
 DEFINE_OBSERVABLE (new_objfile);
 DEFINE_OBSERVABLE (free_objfile);
 DEFINE_OBSERVABLE (new_thread);
diff --git a/gdb/observable.h b/gdb/observable.h
index 070cf0f18e51..b60ee7f3efe6 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -111,6 +111,8 @@ extern observable<struct so_list */* solib */> solib_loaded;
    been unloaded yet, and thus are still available.  */
 extern observable<struct so_list */* solib */> solib_unloaded;

+extern observable<objfile */* objfile */> jit_object_unloaded;
+
 /* The symbol file specified by OBJFILE has been loaded.  Called
    with OBJFILE equal to NULL to indicate previously loaded symbol
    table data has now been invalidated.  */
-- 
2.27.0


  parent reply	other threads:[~2020-06-07  3:41 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-12 11:33 [PATCH 0/2] fix missed breakpoint upon jit re-registration Mihails Strasuns
2020-05-12 11:33 ` [PATCH 1/2] [gdb/testsuite] optional flags for compile_and_download_n_jit_so Mihails Strasuns
2020-05-24 14:34   ` Simon Marchi
2020-05-12 11:33 ` [PATCH 2/2] jit: remove bp locations when unregistering jit code Mihails Strasuns
2020-05-24 15:01   ` Simon Marchi
2020-05-25 10:07     ` Strasuns, Mihails
2020-06-05 12:07       ` Strasuns, Mihails
2020-06-07  3:41       ` Simon Marchi [this message]
2020-06-16 11:48         ` Strasuns, Mihails

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ffc6137d-6ea6-c7a2-ae04-a23ecf97ff3d@simark.ca \
    --to=simark@simark.ca \
    --cc=gdb-patches@sourceware.org \
    --cc=mihails.strasuns@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).