Re: [PATCH 2/3] skip_prolgoue (amd64)

[Doug Evans <xdje42@gmail.com>]

On Sat, Dec 7, 2013 at 11:59 PM, Yao Qi <yao@codesourcery.com> 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  <yao@codesourcery.com>
>
>         * 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. :-)]