Message ID | 20150129071929.GE5193@adacore.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 31928 invoked by alias); 29 Jan 2015 07:19:39 -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 31909 invoked by uid 89); 29 Jan 2015 07:19:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00 autolearn=ham version=3.3.2 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 (AES256-SHA encrypted) ESMTPS; Thu, 29 Jan 2015 07:19:36 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 409EF116678; Thu, 29 Jan 2015 02:19:34 -0500 (EST) 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 EOC0yEkDgc8E; Thu, 29 Jan 2015 02:19:34 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id C8C1011660C; Thu, 29 Jan 2015 02:19:33 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id B0CC0491CD; Thu, 29 Jan 2015 11:19:29 +0400 (RET) Date: Thu, 29 Jan 2015 11:19:29 +0400 From: Joel Brobecker <brobecker@adacore.com> To: Doug Evans <xdje42@gmail.com> Cc: Pedro Alves <palves@redhat.com>, gdb-patches@sourceware.org Subject: Re: [PATCH][PR symtab/17855] Fix. Message-ID: <20150129071929.GE5193@adacore.com> References: <m33876ary6.fsf@sspiff.org> <54C0E1FF.4050201@redhat.com> <m3egqe83x3.fsf@sspiff.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="2oS5YaxWCcQjTEyO" Content-Disposition: inline In-Reply-To: <m3egqe83x3.fsf@sspiff.org> User-Agent: Mutt/1.5.21 (2010-09-15) |
Commit Message
Joel Brobecker
Jan. 29, 2015, 7:19 a.m. UTC
Aargh - I was working on it just now because I wasn't sure if anyone felt they "had the ball on their court". > Like so? > > 2015-01-28 Doug Evans <xdje42@gmail.com> > > PR symtab/17855 > * symfile.c (clear_symtab_users): Move call to breakpoint_re_set > closer to end. What I've tested is moving the call last, as shown in the attached diff. Not technically necessarily better, but as the comment explains, I move it last so that it's after we've purged all relevant caches. That way, the function is organized in two rough sections: - do purges first; - do updates next. Your comment is a lot more detailed, I like it. > diff --git a/gdb/symfile.c b/gdb/symfile.c > index d55e361..bad244c 100644 > --- a/gdb/symfile.c > +++ b/gdb/symfile.c > @@ -3023,13 +3023,8 @@ clear_symtab_users (int add_flags) > /* Someday, we should do better than this, by only blowing away > the things that really need to be blown. */ > > - /* Clear the "current" symtab first, because it is no longer valid. > - breakpoint_re_set may try to access the current symtab. */ > clear_current_source_symtab_and_line (); > - > clear_displays (); > - if ((add_flags & SYMFILE_DEFER_BP_RESET) == 0) > - breakpoint_re_set (); > clear_last_displayed_sal (); > clear_pc_function_cache (); > observer_notify_new_objfile (NULL); > @@ -3040,9 +3035,19 @@ clear_symtab_users (int add_flags) > expression_context_block = NULL; > innermost_block = NULL; > > + /* Now that most everything has been reset, reset any existing breakpoints. > + Reasons for doing this after the above are that breakpoint resetting may > + involve: > + - reading the current source symtab and line, > + - reading the last displayed sal, > + - reading the pc function cache, > + - symbol lookup which requires, for example, invalidating any caches > + first. */ > + if ((add_flags & SYMFILE_DEFER_BP_RESET) == 0) > + breakpoint_re_set (); > + > /* Varobj may refer to old symbols, perform a cleanup. */ > varobj_invalidate (); > - > } > > static void > > >
Comments
On Wed, Jan 28, 2015 at 11:19 PM, Joel Brobecker <brobecker@adacore.com> wrote: > Aargh - I was working on it just now because I wasn't sure if anyone > felt they "had the ball on their court". > >> Like so? >> >> 2015-01-28 Doug Evans <xdje42@gmail.com> >> >> PR symtab/17855 >> * symfile.c (clear_symtab_users): Move call to breakpoint_re_set >> closer to end. > > What I've tested is moving the call last, as shown in the attached > diff. Not technically necessarily better, but as the comment explains, > I move it last so that it's after we've purged all relevant caches. > That way, the function is organized in two rough sections: > - do purges first; > - do updates next. > Your comment is a lot more detailed, I like it. I think yours is fine too. I'm ok with using it.
Doug Evans <xdje42@gmail.com> writes: > On Wed, Jan 28, 2015 at 11:19 PM, Joel Brobecker <brobecker@adacore.com> wrote: >> Aargh - I was working on it just now because I wasn't sure if anyone >> felt they "had the ball on their court". >> >>> Like so? >>> >>> 2015-01-28 Doug Evans <xdje42@gmail.com> >>> >>> PR symtab/17855 >>> * symfile.c (clear_symtab_users): Move call to breakpoint_re_set >>> closer to end. >> >> What I've tested is moving the call last, as shown in the attached >> diff. Not technically necessarily better, but as the comment explains, >> I move it last so that it's after we've purged all relevant caches. >> That way, the function is organized in two rough sections: >> - do purges first; >> - do updates next. >> Your comment is a lot more detailed, I like it. > > I think yours is fine too. > I'm ok with using it. Hi. There's value in the simplicity of your approach so I committed it to trunk and the branch.
On 01/31/2015 10:07 PM, Doug Evans wrote: > Doug Evans <xdje42@gmail.com> writes: >> On Wed, Jan 28, 2015 at 11:19 PM, Joel Brobecker <brobecker@adacore.com> wrote: >>> Your comment is a lot more detailed, I like it. >> >> I think yours is fine too. >> I'm ok with using it. > > Hi. > There's value in the simplicity of your approach so I > committed it to trunk and the branch. > Thanks guys. Pedro Alves
diff --git a/gdb/symfile.c b/gdb/symfile.c index d55e361..04a5c58 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -3028,8 +3028,6 @@ clear_symtab_users (int add_flags) clear_current_source_symtab_and_line (); clear_displays (); - if ((add_flags & SYMFILE_DEFER_BP_RESET) == 0) - breakpoint_re_set (); clear_last_displayed_sal (); clear_pc_function_cache (); observer_notify_new_objfile (NULL); @@ -3043,6 +3041,10 @@ clear_symtab_users (int add_flags) /* Varobj may refer to old symbols, perform a cleanup. */ varobj_invalidate (); + /* Now that the various caches have been cleared, we can re_set + our breakpoints without risking it using stale data. */ + if ((add_flags & SYMFILE_DEFER_BP_RESET) == 0) + breakpoint_re_set (); } static void