Re: [Qemu-devel] [PATCH] tcg: Merge GETPC and GETRA

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Tue, 2016-07-26 at 06:12 +0530, Richard Henderson wrote:
> The return address argument to the softmmu template helpers was
> confused.  In the legacy case, we wanted to indicate that there
> is no return address, and so passed in NULL.  However, we then
> immediately subtracted GETPC_ADJ from NULL, resulting in a non-zero
> value, indicating the presence of an (invalid) return address.
> 
> Push the GETPC_ADJ subtraction down to the only point it's required:
> immediately before use within cpu_restore_state, after all NULL
> pointer
> checks have been completed.  This makes GETPC and GETRA identical.
> 
> Remove GETRA as the lesser used macro, replacing all uses with GETPC.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---

This causes a problem with linux-user. We have cases where a store
instruction from the guest turns into *one* store instruction in
the host. The PC we get from the host segfault is exact, so the -2
in cpu_restore_state() takes us to the previous instruction.

We thus end up reporting the wrong instruction in the signal

I've tentatively fixed it with the hack below. It's not pretty
since we were trying to get rid of all those GETPC_ADJ sprinkled
all over but I don't see any obvious better way.

I'm not sure if there are other cases with softmmu for example, where
we do get an exact host PC and thus where the -2 might take us back too
far.

I don't see your patch upstream yet so feel free to fold my change into
yours.

@@ -103,13 +102,16 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
         return 0; /* not an MMU fault */
     }
     if (ret == 0) {
         return 1; /* the MMU fault was handled without causing real CPU fault */
     }
-    /* now we have a real cpu fault */
-    cpu_restore_state(cpu, pc);
-
+    /* Now we have a real cpu fault. We must undo the adjustment
+     * done by cpu_restore_state() since we aren't pointing to the
+     * next instruction but to the faulting one and going back
+     * can make us report the wrong guest PC.
+     */
+    cpu_restore_state(cpu, pc + GETPC_ADJ);
     sigprocmask(SIG_SETMASK, old_set, NULL);
     cpu_loop_exit(cpu);
 
     /* never comes here */
     return 1;