From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18813 invoked by alias); 8 Dec 2013 08:26:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 18804 invoked by uid 89); 8 Dec 2013 08:26:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.2 required=5.0 tests=AWL,BAYES_50,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wg0-f42.google.com Received: from Unknown (HELO mail-wg0-f42.google.com) (74.125.82.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sun, 08 Dec 2013 08:26:07 +0000 Received: by mail-wg0-f42.google.com with SMTP id a1so2295725wgh.5 for ; Sun, 08 Dec 2013 00:25:57 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.194.232.133 with SMTP id to5mr28585512wjc.41.1386491157606; Sun, 08 Dec 2013 00:25:57 -0800 (PST) Received: by 10.194.37.73 with HTTP; Sun, 8 Dec 2013 00:25:57 -0800 (PST) In-Reply-To: <52A426E8.2030808@codesourcery.com> References: <1385735051-27558-1-git-send-email-yao@codesourcery.com> <1385735051-27558-3-git-send-email-yao@codesourcery.com> <201311291436.rATEaZ5Z030292@glazunov.sibelius.xs4all.nl> <201311291605.rATG5XVb030184@glazunov.sibelius.xs4all.nl> <52994E79.4000004@codesourcery.com> <5299B9D0.2020304@redhat.com> <529C37A2.9000207@codesourcery.com> <529E9462.9010001@codesourcery.com> <529F1B1F.2040606@redhat.com> <52A426E8.2030808@codesourcery.com> Date: Sun, 08 Dec 2013 08:26:00 -0000 Message-ID: Subject: Re: [PATCH 2/3] skip_prolgoue (amd64) From: Doug Evans To: Yao Qi Cc: Pedro Alves , Mark Kettenis , "gdb-patches@sourceware.org" Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes X-SW-Source: 2013-12/txt/msg00299.txt.bz2 On Sat, Dec 7, 2013 at 11:59 PM, Yao Qi wrote: > On 12/04/2013 08:07 PM, Pedro Alves wrote: >>> >+ if (non_stop) >>> >+ { >>> >+ /* In non-stop mode, one thread stops and caches the contents of >>> >+ stack or code, while other running threads may change the >>> >+ code (through JIT) or stack. The target cache can get stale >>> >+ without us being able to detect it. Flush target cache >>> >+ before handling each event. */ >>> >+ target_dcache_invalidate (); >>> >+ } >> I don't actually think this should be gated on non-stop. It >> should be unconditional. I mentioned before that it'd be most >> visible with non-stop, but that doesn't imply it's not >> visible with all-stop. If we're seeing or going to wait for >> a target event, it's because the target was running, >> irrespective of all-stop/non-stop. I really think we >> should invalidate the cache at all places we invalidate the >> overlay cache (wait_for_inferior, etc.), not just fetch_inferior_event. > > After some discussions, it becomes clear to me that we should flush > target cache before handling events, in the place of the callers of > handle_inferior_event. I am wondering why don't we flush cache inside > handle_inferior_event? Although flushing cache is not much relevant > to handle_inferior_event, this can avoid doing cache flush in every > caller of handle_inferior_event. > >> For all-stop, it shouldn't really make a difference to >> performance, as we invalidate the cache on resumes anyway, >> and in all-stop, there must always be a resume prior to >> any stop... > > If that is the right way to go, I'll move 'overlay_cache_invalid = 1' > to handle_inferior_event too. WDYT? > > gdb: > > 2013-12-08 Yao Qi > > * infrun.c: Include "target-dcache.h". > (fetch_inferior_event): Flush target cache. > --- > gdb/infrun.c | 6 ++++++ > 1 files changed, 6 insertions(+), 0 deletions(-) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 3b55583..0a12107 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -60,6 +60,7 @@ > #include "objfiles.h" > #include "completer.h" > #include "target-descriptions.h" > +#include "target-dcache.h" > > /* Prototypes for local functions */ > > @@ -3168,6 +3169,11 @@ handle_inferior_event (struct execution_control_state *ecs) > { > enum stop_kind stop_soon; > > + /* If we've got an event from target, it means the target was > + running, so cache would be staled. Flush target cache before > + handling each event. */ > + target_dcache_invalidate (); > + > if (ecs->ws.kind == TARGET_WAITKIND_IGNORE) > { > /* We had an event in the inferior, but we are not interested in > -- If you're going for correctness here, note that this is just another heuristic. It gets one closer, but as long as at least one thread is still running .... Question: Does this obviate the need to call target_dcache_invalidate elsewhere? If so, removing other uses that this subsumes should be part of this patch. Nit: "cache would be staled" is bad English. "the cache could be stale" would be better, though I'm sure there is something even better than that. [I suspect there's at least one too many commas in there too, but one can nitpick too much. :-)]