From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 84897384A87E for ; Sun, 7 Jun 2020 03:41:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 84897384A87E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id D13F71E5F9; Sat, 6 Jun 2020 23:41:55 -0400 (EDT) From: Simon Marchi Subject: Re: [PATCH 2/2] jit: remove bp locations when unregistering jit code To: "Strasuns, Mihails" , "gdb-patches@sourceware.org" References: <20200512113308.9502-1-mihails.strasuns@intel.com> <20200512113308.9502-3-mihails.strasuns@intel.com> <7f117c0d-5741-28e7-c1c3-5d034f7a6ab1@simark.ca> Message-ID: Date: Sat, 6 Jun 2020 23:41:55 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 07 Jun 2020 03:41:58 -0000 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 >> Sent: Sunday, May 24, 2020 5:02 PM >> To: Strasuns, Mihails ; 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 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 solib_loaded; been unloaded yet, and thus are still available. */ extern observable solib_unloaded; +extern observable 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