Message ID | 20190320140846.13031-1-tromey@adacore.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 40604 invoked by alias); 20 Mar 2019 14:08:53 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 40596 invoked by uid 89); 20 Mar 2019 14:08:53 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_NUMSUBJECT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=ecs, HX-Languages-Length:1999, HContent-Transfer-Encoding:8bit X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 20 Mar 2019 14:08:52 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 66D6F560C2; Wed, 20 Mar 2019 10:08:50 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id pRHtlHQ0NrTZ; Wed, 20 Mar 2019 10:08:50 -0400 (EDT) Received: from murgatroyd.Home (174-29-37-56.hlrn.qwest.net [174.29.37.56]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPSA id 1D439560C1; Wed, 20 Mar 2019 10:08:50 -0400 (EDT) From: Tom Tromey <tromey@adacore.com> To: gdb-patches@sourceware.org Cc: Tom Tromey <tromey@adacore.com> Subject: [PATCH] Merge handle_inferior_event and handle_inferior_event_1 Date: Wed, 20 Mar 2019 08:08:46 -0600 Message-Id: <20190320140846.13031-1-tromey@adacore.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit |
Commit Message
Tom Tromey
March 20, 2019, 2:08 p.m. UTC
I noticed that handle_inferior_event is just a small wrapper that frees the value chain. This patch replaces it with a scoped_value_mark, reducing the number of lines of code here. Regression tested on x86-64 Fedora 29. gdb/ChangeLog 2019-03-20 Tom Tromey <tromey@adacore.com> * infrun.c (handle_inferior_event): Rename from handle_inferior_event_1. Create a scoped_value_mark. (handle_inferior_event): Remove. --- gdb/ChangeLog | 6 ++++++ gdb/infrun.c | 23 ++++++----------------- 2 files changed, 12 insertions(+), 17 deletions(-)
Comments
On 03/20/2019 02:08 PM, Tom Tromey wrote: > I noticed that handle_inferior_event is just a small wrapper that > frees the value chain. This patch replaces it with a > scoped_value_mark, reducing the number of lines of code here. > > Regression tested on x86-64 Fedora 29. > > gdb/ChangeLog > 2019-03-20 Tom Tromey <tromey@adacore.com> > > * infrun.c (handle_inferior_event): Rename from > handle_inferior_event_1. Create a scoped_value_mark. > (handle_inferior_event): Remove. OK. Thanks, Pedro Alves
On Wednesday, March 20 2019, Tom Tromey wrote: > I noticed that handle_inferior_event is just a small wrapper that > frees the value chain. This patch replaces it with a > scoped_value_mark, reducing the number of lines of code here. > > Regression tested on x86-64 Fedora 29. A few days ago (March 18th) a user reported a bug against Fedora GDB: https://bugzilla.redhat.com/show_bug.cgi?id=1690120 And, after hours and hours investigating this, I found that your commit actually fixes it. Not sure if it was intended or not, but what a coincidence... Thanks,
On Thursday, March 21 2019, I wrote: > On Wednesday, March 20 2019, Tom Tromey wrote: > >> I noticed that handle_inferior_event is just a small wrapper that >> frees the value chain. This patch replaces it with a >> scoped_value_mark, reducing the number of lines of code here. >> >> Regression tested on x86-64 Fedora 29. > > A few days ago (March 18th) a user reported a bug against Fedora GDB: > > https://bugzilla.redhat.com/show_bug.cgi?id=1690120 > > And, after hours and hours investigating this, I found that your commit > actually fixes it. Not sure if it was intended or not, but what a > coincidence... Of course, something I'd also like to ask is: do you think this change is trivial enough to be included in the 8.3 release? I'd like to see it happen. I can create a bug if needed. Thanks,
>>>>> ">" == Sergio Durigan Junior <sergiodj@redhat.com> writes: >> A few days ago (March 18th) a user reported a bug against Fedora GDB: >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1690120 >> >> And, after hours and hours investigating this, I found that your commit >> actually fixes it. Not sure if it was intended or not, but what a >> coincidence... I am curious to know how this change could fix this bug. That seems mysterious to me. >> Of course, something I'd also like to ask is: do you think this change >> is trivial enough to be included in the 8.3 release? I'd like to see it >> happen. I can create a bug if needed. I would have thought so but the fact that it causes this behavior makes me worry a bit. Maybe that's backward! Tom
On 03/25/2019 03:35 PM, Tom Tromey wrote: >>>>>> ">" == Sergio Durigan Junior <sergiodj@redhat.com> writes: > >>> A few days ago (March 18th) a user reported a bug against Fedora GDB: >>> >>> https://bugzilla.redhat.com/show_bug.cgi?id=1690120 >>> >>> And, after hours and hours investigating this, I found that your commit >>> actually fixes it. Not sure if it was intended or not, but what a >>> coincidence... > > I am curious to know how this change could fix this bug. > That seems mysterious to me. There's one behavior change the patch makes, which is that before the patch, if handle_inferior_event_1 threw an error, value_free_to_mark would not be called. After the patch, it's always called, from scoped_value_mark's dtor. From the backtrace in the bug report, we see that the crash happens late during teardown, destroying an xmethod value in the global "all_values" vector: ... #13 std::vector<gdb::ref_ptr<value, value_ref_policy>, std::allocator<gdb::ref_ptr<value, value_ref_policy> > >::~vector (this=0x559d15b24a10 <all_values>, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/stl_vector.h:677 No locals. #14 0x00007f92a8fe0700 in __run_exit_handlers () from /lib64/libc.so.6 No symbol table info available. #15 0x00007f92a8fe0840 in exit () from /lib64/libc.so.6 No symbol table info available. ... So it looks like your patch made it so that the value in question avoids the crash because the value in question is no longer in the all_values vector by the time gdb is tearing down? Open questions then would be: - what was the exception in question that was thrown from within handle_inferior_event_1? (And if there was no exception, then the theory above is incorrect.) - why don't non-temporary xmethod values put in the value history cause a similar crash at tear down time? >>> Of course, something I'd also like to ask is: do you think this change >>> is trivial enough to be included in the 8.3 release? I'd like to see it >>> happen. I can create a bug if needed. > > I would have thought so but the fact that it causes this behavior makes > me worry a bit. Maybe that's backward! Thanks, Pedro Alves
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> So it looks like your patch made it so that the value in question
Pedro> avoids the crash because the value in question is no longer in the
Pedro> all_values vector by the time gdb is tearing down?
Makes sense.
Pedro> Open questions then would be:
Pedro> - what was the exception in question that was thrown from
Pedro> within handle_inferior_event_1? (And if there was
Pedro> no exception, then the theory above is incorrect.)
Pedro> - why don't non-temporary xmethod values put in the value history
Pedro> cause a similar crash at tear down time?
Also, if a crash can occur during value destruction like this, then I
think it must indicate a bug somewhere else, like invalid reference
counting somewhere.
Tom
On Monday, March 25 2019, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: > > Pedro> So it looks like your patch made it so that the value in question > Pedro> avoids the crash because the value in question is no longer in the > Pedro> all_values vector by the time gdb is tearing down? > > Makes sense. Thanks Pedro for the explanation. Yeah, after investigating it became clear to me that the patch fixes (or "fixes") the bug by destroying the value instances before we exit GDB. What's interesting is the fact that I cannot seem to reproduce easily. For example, if you print anything other than an xmethod, and then you print an xmethod, and then exit GDB, everything works correctly. For that reason, I'm still not able to write a testcase for it, because our testing infrastructure prints some stuff as a preparation for the test. > Pedro> Open questions then would be: > > Pedro> - what was the exception in question that was thrown from > Pedro> within handle_inferior_event_1? (And if there was > Pedro> no exception, then the theory above is incorrect.) > > Pedro> - why don't non-temporary xmethod values put in the value history > Pedro> cause a similar crash at tear down time? > > Also, if a crash can occur during value destruction like this, then I > think it must indicate a bug somewhere else, like invalid reference > counting somewhere. That's a good point, yeah. I can try investigating more. Having said that, I think the patch is simple enough to be safely backportable to 8.3, and it doesn't really break anything (in fact, as Pedro mentioned, it makes things more correct, because now we're sure that we will destroy the objects if an exception is thrown), so IMHO there's no downside to it. WDYT? Thanks,
>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes: >> Also, if a crash can occur during value destruction like this, then I >> think it must indicate a bug somewhere else, like invalid reference >> counting somewhere. Sergio> That's a good point, yeah. I can try investigating more. Maybe one idea would be to back out my patch and then run the failing test under valgrind. Sergio> Having said that, I think the patch is simple enough to be safely Sergio> backportable to 8.3, and it doesn't really break anything (in fact, as Sergio> Pedro mentioned, it makes things more correct, because now we're sure Sergio> that we will destroy the objects if an exception is thrown), so IMHO Sergio> there's no downside to it. WDYT? I agree. Tom
On Tuesday, March 26 2019, Tom Tromey wrote: >>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes: > >>> Also, if a crash can occur during value destruction like this, then I >>> think it must indicate a bug somewhere else, like invalid reference >>> counting somewhere. > > Sergio> That's a good point, yeah. I can try investigating more. > > Maybe one idea would be to back out my patch and then run the failing > test under valgrind. Thanks. > Sergio> Having said that, I think the patch is simple enough to be safely > Sergio> backportable to 8.3, and it doesn't really break anything (in fact, as > Sergio> Pedro mentioned, it makes things more correct, because now we're sure > Sergio> that we will destroy the objects if an exception is thrown), so IMHO > Sergio> there's no downside to it. WDYT? > > I agree. Thanks, Tom. I created <https://sourceware.org/bugzilla/show_bug.cgi?id=24391>. Would you like me to go ahead and backport the patch to the 8.3 branch, or do you want to do that? Thanks,
>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
Sergio> Thanks, Tom. I created
Sergio> <https://sourceware.org/bugzilla/show_bug.cgi?id=24391>. Would you like
Sergio> me to go ahead and backport the patch to the 8.3 branch, or do you want
Sergio> to do that?
Please do it! Thanks.
Tom
On Tuesday, March 26 2019, I wrote: > On Tuesday, March 26 2019, Tom Tromey wrote: > >>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes: >> >>>> Also, if a crash can occur during value destruction like this, then I >>>> think it must indicate a bug somewhere else, like invalid reference >>>> counting somewhere. >> >> Sergio> That's a good point, yeah. I can try investigating more. >> >> Maybe one idea would be to back out my patch and then run the failing >> test under valgrind. > > Thanks. I did that, and we (Pedro, Mark, Frank and I) did a session of collective investigation. I summarized what we found here: https://bugzilla.redhat.com/show_bug.cgi?id=1690120#c25 Thanks,
Sergio> I did that, and we (Pedro, Mark, Frank and I) did a session of Sergio> collective investigation. I summarized what we found here: Sergio> https://bugzilla.redhat.com/show_bug.cgi?id=1690120#c25 Thanks, that's very interesting. I suppose either better control over the order of destruction is needed, or maybe finalize_python should clear gdb_python_initialized and then this should be checked in xmethod destructor. Tom
On 03/27/2019 12:57 PM, Tom Tromey wrote: > Sergio> I did that, and we (Pedro, Mark, Frank and I) did a session of > Sergio> collective investigation. I summarized what we found here: > Sergio> https://bugzilla.redhat.com/show_bug.cgi?id=1690120#c25 > > Thanks, that's very interesting. > I suppose either better control over the order of destruction is needed, > or maybe finalize_python should clear gdb_python_initialized and then > this should be checked in xmethod destructor. I think the former is better. I think we should put an all_values.clear (); call in quit_force, before the do_final_cleanups call. Even better, add a new finalize_values function next to _initialize_values, and call that. Thanks, Pedro Alves
diff --git a/gdb/infrun.c b/gdb/infrun.c index 550fbe7f5b9..ad7892105a4 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4591,8 +4591,13 @@ handle_no_resumed (struct execution_control_state *ecs) once). */ static void -handle_inferior_event_1 (struct execution_control_state *ecs) +handle_inferior_event (struct execution_control_state *ecs) { + /* Make sure that all temporary struct value objects that were + created during the handling of the event get deleted at the + end. */ + scoped_value_mark free_values; + enum stop_kind stop_soon; if (ecs->ws.kind == TARGET_WAITKIND_IGNORE) @@ -5189,22 +5194,6 @@ Cannot fill $_exitsignal with the correct signal number.\n")); } } -/* A wrapper around handle_inferior_event_1, which also makes sure - that all temporary struct value objects that were created during - the handling of the event get deleted at the end. */ - -static void -handle_inferior_event (struct execution_control_state *ecs) -{ - struct value *mark = value_mark (); - - handle_inferior_event_1 (ecs); - /* Purge all temporary values created during the event handling, - as it could be a long time before we return to the command level - where such values would otherwise be purged. */ - value_free_to_mark (mark); -} - /* Restart threads back to what they were trying to do back when we paused them for an in-line step-over. The EVENT_THREAD thread is ignored. */