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

Doug Evans
2013-12-08T08:26:09Z

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. :-)]