[Qemu-devel] [PATCH] target-*: Get rid of "PC advancement" trick

[Qemu-devel] [PATCH] target-*: Get rid of "PC advancement" trick

[Sergey Fedorov]

The "PC advancement" trick was used just after recognizing that a
breakpoint exception was going to be generated. This trick has had two
points:
 1. Guarantee that tb->size isn't zero: there are many places where it's
    expected to be non-zero. In fact, that is even stated in the comment
    for this field.
 2. Try to satisfy disassembler's check for instruction length. To this
    end, PC advancement was done for estimated instruction length, but
    actually, didn't work properly in variable-instruction-length cases.

Substitute this trick with checking for TB size at the end of
translation. If we get an empty TB then just set tb->size to 1 and skip
disassembling. Setting tb->size to 1 is enough to get correct behaviour,
whereas an empty TB doesn't obviously need to be disassembled.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
---
 target-alpha/translate.c      | 15 ++++++++++-----
 target-arm/translate-a64.c    | 16 ++++++++++------
 target-arm/translate.c        | 18 ++++++++++--------
 target-cris/translate.c       | 15 ++++++++++-----
 target-i386/translate.c       | 15 ++++++++++-----
 target-lm32/translate.c       | 15 ++++++++++-----
 target-m68k/translate.c       | 15 ++++++++++-----
 target-microblaze/translate.c | 15 ++++++++++-----
 target-mips/translate.c       | 15 ++++++++++-----
 target-moxie/translate.c      | 15 ++++++++++-----
 target-openrisc/translate.c   | 15 ++++++++++-----
 target-ppc/translate.c        | 15 ++++++++++-----
 target-s390x/translate.c      | 15 ++++++++++-----
 target-sh4/translate.c        | 15 ++++++++++-----
 target-unicore32/translate.c  | 15 ++++++++++-----
 target-xtensa/translate.c     | 15 ++++++++++-----
 16 files changed, 160 insertions(+), 84 deletions(-)

diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index 9909c70..d27f504 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -2917,11 +2917,6 @@ void gen_intermediate_code(CPUAlphaState *env, struct TranslationBlock *tb)
 
         if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
             ret = gen_excp(&ctx, EXCP_DEBUG, 0);
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            ctx.pc += 4;
             break;
         }
         if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
@@ -2983,6 +2978,16 @@ void gen_intermediate_code(CPUAlphaState *env, struct TranslationBlock *tb)
 
     gen_tb_end(tb, num_insns);
 
+    if (ctx.pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
     tb->size = ctx.pc - pc_start;
     tb->icount = num_insns;
 
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 14e8131..11719f8 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -11097,12 +11097,6 @@ void gen_intermediate_code_a64(ARMCPU *cpu, TranslationBlock *tb)
                         dc->is_jmp = DISAS_UPDATE;
                     } else {
                         gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
-                        /* The address covered by the breakpoint must be
-                           included in [tb->pc, tb->pc + tb->size) in order
-                           to for it to be properly cleared -- thus we
-                           increment the PC here so that the logic setting
-                           tb->size below does the right thing.  */
-                        dc->pc += 4;
                         goto done_generating;
                     }
                     break;
@@ -11213,6 +11207,16 @@ void gen_intermediate_code_a64(ARMCPU *cpu, TranslationBlock *tb)
 done_generating:
     gen_tb_end(tb, num_insns);
 
+    if (dc->pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
 #ifdef DEBUG_DISAS
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("----------------\n");
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 5d22879..07d5d02 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11381,14 +11381,6 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
                         dc->is_jmp = DISAS_UPDATE;
                     } else {
                         gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
-                        /* The address covered by the breakpoint must be
-                           included in [tb->pc, tb->pc + tb->size) in order
-                           to for it to be properly cleared -- thus we
-                           increment the PC here so that the logic setting
-                           tb->size below does the right thing.  */
-                        /* TODO: Advance PC by correct instruction length to
-                         * avoid disassembler error messages */
-                        dc->pc += 2;
                         goto done_generating;
                     }
                     break;
@@ -11583,6 +11575,16 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
 done_generating:
     gen_tb_end(tb, num_insns);
 
+    if (dc->pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
 #ifdef DEBUG_DISAS
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("----------------\n");
diff --git a/target-cris/translate.c b/target-cris/translate.c
index 2d710cc..b3274a3 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3166,11 +3166,6 @@ void gen_intermediate_code(CPUCRISState *env, struct TranslationBlock *tb)
             tcg_gen_movi_tl(env_pc, dc->pc);
             t_gen_raise_exception(EXCP_DEBUG);
             dc->is_jmp = DISAS_UPDATE;
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            dc->pc += 2;
             break;
         }
 
@@ -3293,6 +3288,16 @@ void gen_intermediate_code(CPUCRISState *env, struct TranslationBlock *tb)
     }
     gen_tb_end(tb, num_insns);
 
+    if (dc->pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
     tb->size = dc->pc - pc_start;
     tb->icount = num_insns;
 
diff --git a/target-i386/translate.c b/target-i386/translate.c
index a3dd167..b6f4855 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7987,11 +7987,6 @@ void gen_intermediate_code(CPUX86State *env, TranslationBlock *tb)
                                          tb->flags & HF_RF_MASK
                                          ? BP_GDB : BP_ANY))) {
             gen_debug(dc, pc_ptr - dc->cs_base);
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            pc_ptr += 1;
             goto done_generating;
         }
         if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
@@ -8046,6 +8041,16 @@ void gen_intermediate_code(CPUX86State *env, TranslationBlock *tb)
 done_generating:
     gen_tb_end(tb, num_insns);
 
+    if (pc_ptr == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
 #ifdef DEBUG_DISAS
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         int disas_flags;
diff --git a/target-lm32/translate.c b/target-lm32/translate.c
index fa5b0b9..ef150bf 100644
--- a/target-lm32/translate.c
+++ b/target-lm32/translate.c
@@ -1078,11 +1078,6 @@ void gen_intermediate_code(CPULM32State *env, struct TranslationBlock *tb)
             tcg_gen_movi_tl(cpu_pc, dc->pc);
             t_gen_raise_exception(dc, EXCP_DEBUG);
             dc->is_jmp = DISAS_UPDATE;
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            dc->pc += 4;
             break;
         }
 
@@ -1131,6 +1126,16 @@ void gen_intermediate_code(CPULM32State *env, struct TranslationBlock *tb)
 
     gen_tb_end(tb, num_insns);
 
+    if (dc->pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
     tb->size = dc->pc - pc_start;
     tb->icount = num_insns;
 
diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 41ae2c6..583a455 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -3004,11 +3004,6 @@ void gen_intermediate_code(CPUM68KState *env, TranslationBlock *tb)
         if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
             gen_exception(dc, dc->pc, EXCP_DEBUG);
             dc->is_jmp = DISAS_JUMP;
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            dc->pc += 2;
             break;
         }
 
@@ -3053,6 +3048,16 @@ void gen_intermediate_code(CPUM68KState *env, TranslationBlock *tb)
     }
     gen_tb_end(tb, num_insns);
 
+    if (dc->pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
 #ifdef DEBUG_DISAS
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("----------------\n");
diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
index 154b9d6..4a4beb7 100644
--- a/target-microblaze/translate.c
+++ b/target-microblaze/translate.c
@@ -1693,11 +1693,6 @@ void gen_intermediate_code(CPUMBState *env, struct TranslationBlock *tb)
         if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
             t_gen_raise_exception(dc, EXCP_DEBUG);
             dc->is_jmp = DISAS_UPDATE;
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            dc->pc += 4;
             break;
         }
 
@@ -1803,6 +1798,16 @@ void gen_intermediate_code(CPUMBState *env, struct TranslationBlock *tb)
     }
     gen_tb_end(tb, num_insns);
 
+    if (dc->pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
     tb->size = dc->pc - pc_start;
     tb->icount = num_insns;
 
diff --git a/target-mips/translate.c b/target-mips/translate.c
index 5626647..488603b 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -19627,11 +19627,6 @@ void gen_intermediate_code(CPUMIPSState *env, struct TranslationBlock *tb)
             save_cpu_state(&ctx, 1);
             ctx.bstate = BS_BRANCH;
             gen_helper_raise_exception_debug(cpu_env);
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            ctx.pc += 4;
             goto done_generating;
         }
 
@@ -19722,6 +19717,16 @@ void gen_intermediate_code(CPUMIPSState *env, struct TranslationBlock *tb)
 done_generating:
     gen_tb_end(tb, num_insns);
 
+    if (ctx.pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
     tb->size = ctx.pc - pc_start;
     tb->icount = num_insns;
 
diff --git a/target-moxie/translate.c b/target-moxie/translate.c
index 6dedcb7..e682735 100644
--- a/target-moxie/translate.c
+++ b/target-moxie/translate.c
@@ -848,11 +848,6 @@ void gen_intermediate_code(CPUMoxieState *env, struct TranslationBlock *tb)
             tcg_gen_movi_i32(cpu_pc, ctx.pc);
             gen_helper_debug(cpu_env);
             ctx.bstate = BS_EXCP;
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            ctx.pc += 2;
             goto done_generating;
         }
 
@@ -890,6 +885,16 @@ void gen_intermediate_code(CPUMoxieState *env, struct TranslationBlock *tb)
  done_generating:
     gen_tb_end(tb, num_insns);
 
+    if (ctx.pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
     tb->size = ctx.pc - pc_start;
     tb->icount = num_insns;
 }
diff --git a/target-openrisc/translate.c b/target-openrisc/translate.c
index 606490a..762e82e 100644
--- a/target-openrisc/translate.c
+++ b/target-openrisc/translate.c
@@ -1665,11 +1665,6 @@ void gen_intermediate_code(CPUOpenRISCState *env, struct TranslationBlock *tb)
             tcg_gen_movi_tl(cpu_pc, dc->pc);
             gen_exception(dc, EXCP_DEBUG);
             dc->is_jmp = DISAS_UPDATE;
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            dc->pc += 4;
             break;
         }
 
@@ -1736,6 +1731,16 @@ void gen_intermediate_code(CPUOpenRISCState *env, struct TranslationBlock *tb)
 
     gen_tb_end(tb, num_insns);
 
+    if (dc->pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
     tb->size = dc->pc - pc_start;
     tb->icount = num_insns;
 
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 41a7258..ba83e08 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -11488,11 +11488,6 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb)
 
         if (unlikely(cpu_breakpoint_test(cs, ctx.nip, BP_ANY))) {
             gen_debug_exception(ctxp);
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            ctx.nip += 4;
             break;
         }
 
@@ -11589,6 +11584,16 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb)
     }
     gen_tb_end(tb, num_insns);
 
+    if (ctx.nip == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
     tb->size = ctx.nip - pc_start;
     tb->icount = num_insns;
 
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index c79a2cb..09421ad 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -5360,11 +5360,6 @@ void gen_intermediate_code(CPUS390XState *env, struct TranslationBlock *tb)
         if (unlikely(cpu_breakpoint_test(cs, dc.pc, BP_ANY))) {
             status = EXIT_PC_STALE;
             do_debug = true;
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            dc.pc += 2;
             break;
         }
 
@@ -5417,6 +5412,16 @@ void gen_intermediate_code(CPUS390XState *env, struct TranslationBlock *tb)
 
     gen_tb_end(tb, num_insns);
 
+    if (dc.pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
     tb->size = dc.pc - pc_start;
     tb->icount = num_insns;
 
diff --git a/target-sh4/translate.c b/target-sh4/translate.c
index 7bc6216..ed2fa57 100644
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -1855,11 +1855,6 @@ void gen_intermediate_code(CPUSH4State * env, struct TranslationBlock *tb)
             tcg_gen_movi_i32(cpu_pc, ctx.pc);
             gen_helper_debug(cpu_env);
             ctx.bstate = BS_BRANCH;
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            ctx.pc += 2;
             break;
         }
 
@@ -1908,6 +1903,16 @@ void gen_intermediate_code(CPUSH4State * env, struct TranslationBlock *tb)
 
     gen_tb_end(tb, num_insns);
 
+    if (ctx.pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
     tb->size = ctx.pc - pc_start;
     tb->icount = num_insns;
 
diff --git a/target-unicore32/translate.c b/target-unicore32/translate.c
index d2f92f0..b02783f 100644
--- a/target-unicore32/translate.c
+++ b/target-unicore32/translate.c
@@ -1917,11 +1917,6 @@ void gen_intermediate_code(CPUUniCore32State *env, TranslationBlock *tb)
             gen_set_pc_im(dc->pc);
             gen_exception(EXCP_DEBUG);
             dc->is_jmp = DISAS_JUMP;
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            dc->pc += 4;
             goto done_generating;
         }
 
@@ -2017,6 +2012,16 @@ void gen_intermediate_code(CPUUniCore32State *env, TranslationBlock *tb)
 done_generating:
     gen_tb_end(tb, num_insns);
 
+    if (dc->pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
 #ifdef DEBUG_DISAS
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("----------------\n");
diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
index 06b0163..6e5f8ba 100644
--- a/target-xtensa/translate.c
+++ b/target-xtensa/translate.c
@@ -3088,11 +3088,6 @@ void gen_intermediate_code(CPUXtensaState *env, TranslationBlock *tb)
             tcg_gen_movi_i32(cpu_pc, dc.pc);
             gen_exception(&dc, EXCP_DEBUG);
             dc.is_jmp = DISAS_UPDATE;
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            dc.pc += 2;
             break;
         }
 
@@ -3146,6 +3141,16 @@ void gen_intermediate_code(CPUXtensaState *env, TranslationBlock *tb)
     }
     gen_tb_end(tb, insn_count);
 
+    if (dc.pc == pc_start) {
+        /* We have got an empty TB but there are many places where tb->size is
+         * expected it to be non-zero. Just setting it to 1 is enough to get
+         * correct behaviour.
+         */
+        tb->size = 1;
+        tb->icount = 0;
+        return;
+    }
+
 #ifdef DEBUG_DISAS
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("----------------\n");
-- 
1.9.1

Re: [Qemu-devel] [PATCH] target-*: Get rid of "PC advancement" trick

[Richard Henderson]

On 12/10/2015 10:47 AM, Sergey Fedorov wrote:
> The "PC advancement" trick was used just after recognizing that a
> breakpoint exception was going to be generated. This trick has had two
> points:
>   1. Guarantee that tb->size isn't zero: there are many places where it's
>      expected to be non-zero. In fact, that is even stated in the comment
>      for this field.
>   2. Try to satisfy disassembler's check for instruction length. To this
>      end, PC advancement was done for estimated instruction length, but
>      actually, didn't work properly in variable-instruction-length cases.
>
> Substitute this trick with checking for TB size at the end of
> translation. If we get an empty TB then just set tb->size to 1 and skip
> disassembling. Setting tb->size to 1 is enough to get correct behaviour,
> whereas an empty TB doesn't obviously need to be disassembled.

This doesn't help when the TB already has instructions, the TB would ordinarily 
cross a page boundary, and the breakpoint is at the page boundary.


r~

Re: [Qemu-devel] [PATCH] target-*: Get rid of "PC advancement" trick

[Sergey Fedorov]

On 12/12/15 02:39, Richard Henderson wrote:
> On 12/10/2015 10:47 AM, Sergey Fedorov wrote:
>> The "PC advancement" trick was used just after recognizing that a
>> breakpoint exception was going to be generated. This trick has had two
>> points:
>>   1. Guarantee that tb->size isn't zero: there are many places where 
>> it's
>>      expected to be non-zero. In fact, that is even stated in the 
>> comment
>>      for this field.
>>   2. Try to satisfy disassembler's check for instruction length. To this
>>      end, PC advancement was done for estimated instruction length, but
>>      actually, didn't work properly in variable-instruction-length 
>> cases.
>>
>> Substitute this trick with checking for TB size at the end of
>> translation. If we get an empty TB then just set tb->size to 1 and skip
>> disassembling. Setting tb->size to 1 is enough to get correct behaviour,
>> whereas an empty TB doesn't obviously need to be disassembled.
>
> This doesn't help when the TB already has instructions, the TB would 
> ordinarily cross a page boundary, and the breakpoint is at the page 
> boundary.

I see your point. But I am wondering why most architectures stop 
translating on a page boundary whereas i386 and m86k don't. There are 
some comments which say that's to ensure instruction fetch aborts occur 
at the right place. Isn't it necessary for all architectures?

At least for those architectures which do stop translating on a page 
boundary, I think this patch is applicable. Certainly, it would be 
better to have a single solution for all architectures.

Thanks,
Sergey

Re: [Qemu-devel] [PATCH] target-*: Get rid of "PC advancement" trick

[Richard Henderson]

On 12/12/2015 12:02 PM, Sergey Fedorov wrote:
> On 12/12/15 02:39, Richard Henderson wrote:
>> On 12/10/2015 10:47 AM, Sergey Fedorov wrote:
>>> The "PC advancement" trick was used just after recognizing that a
>>> breakpoint exception was going to be generated. This trick has had two
>>> points:
>>>   1. Guarantee that tb->size isn't zero: there are many places where it's
>>>      expected to be non-zero. In fact, that is even stated in the comment
>>>      for this field.
>>>   2. Try to satisfy disassembler's check for instruction length. To this
>>>      end, PC advancement was done for estimated instruction length, but
>>>      actually, didn't work properly in variable-instruction-length cases.
>>>
>>> Substitute this trick with checking for TB size at the end of
>>> translation. If we get an empty TB then just set tb->size to 1 and skip
>>> disassembling. Setting tb->size to 1 is enough to get correct behaviour,
>>> whereas an empty TB doesn't obviously need to be disassembled.
>>
>> This doesn't help when the TB already has instructions, the TB would
>> ordinarily cross a page boundary, and the breakpoint is at the page boundary.
>
> I see your point. But I am wondering why most architectures stop translating on
> a page boundary whereas i386 and m86k don't. There are some comments which say
> that's to ensure instruction fetch aborts occur at the right place. Isn't it
> necessary for all architectures?

In order to support targets with variable-sized instructions, like i386, where 
a single instruction may straddle the page boundary, generic code can handle a 
TB taking code from two (and only two) pages.

It's true that some of the way we treat i386 TBs plays a bit fast and loose, in 
that it's possible to design a code sequence that will report a page access 
error at the beginning of the TB, rather than another runtime exception which 
might have been visible if we'd executed the TB.

I believe that someone fixed that for arm thumb, but no one has bothered about 
this for i386, m68k, or s390.  Because it just doesn't happen in practice; one 
has to go out of one's way to design the errant code sequence.


> At least for those architectures which do stop translating on a page boundary,
> I think this patch is applicable. Certainly, it would be better to have a
> single solution for all architectures.

For that, I think it might be interesting to arrange for non-empty TBs to exit 
prior to recognizing a breakpoint.  So that a breakpoint TB is always just the 
one operation.  Except for the fact that "generate an exception" has 
traditionally been a target-specific helper, we could almost make the entire 
breakpoint generation be done in common code.

I'd think something like a generic "must we end the TB now" predicate would be 
the proper hook.  It would contain all of the usual stuff: tcg_op_buf_full and 
checks for singlestep, but then add "is there a breakpoint at the next pc".


r~

Re: [Qemu-devel] [PATCH] target-*: Get rid of "PC advancement" trick

[Sergey Fedorov]

On 12/14/15 19:19, Richard Henderson wrote:
> On 12/12/2015 12:02 PM, Sergey Fedorov wrote:
>> On 12/12/15 02:39, Richard Henderson wrote:
>>> On 12/10/2015 10:47 AM, Sergey Fedorov wrote:
>>>> The "PC advancement" trick was used just after recognizing that a
>>>> breakpoint exception was going to be generated. This trick has had two
>>>> points:
>>>>   1. Guarantee that tb->size isn't zero: there are many places 
>>>> where it's
>>>>      expected to be non-zero. In fact, that is even stated in the 
>>>> comment
>>>>      for this field.
>>>>   2. Try to satisfy disassembler's check for instruction length. To 
>>>> this
>>>>      end, PC advancement was done for estimated instruction length, 
>>>> but
>>>>      actually, didn't work properly in variable-instruction-length 
>>>> cases.
>>>>
>>>> Substitute this trick with checking for TB size at the end of
>>>> translation. If we get an empty TB then just set tb->size to 1 and 
>>>> skip
>>>> disassembling. Setting tb->size to 1 is enough to get correct 
>>>> behaviour,
>>>> whereas an empty TB doesn't obviously need to be disassembled.
>>>
>>> This doesn't help when the TB already has instructions, the TB would
>>> ordinarily cross a page boundary, and the breakpoint is at the page 
>>> boundary.
>>
>> I see your point. But I am wondering why most architectures stop 
>> translating on
>> a page boundary whereas i386 and m86k don't. There are some comments 
>> which say
>> that's to ensure instruction fetch aborts occur at the right place. 
>> Isn't it
>> necessary for all architectures?
>
> In order to support targets with variable-sized instructions, like 
> i386, where a single instruction may straddle the page boundary, 
> generic code can handle a TB taking code from two (and only two) pages.
>
> It's true that some of the way we treat i386 TBs plays a bit fast and 
> loose, in that it's possible to design a code sequence that will 
> report a page access error at the beginning of the TB, rather than 
> another runtime exception which might have been visible if we'd 
> executed the TB.
>
> I believe that someone fixed that for arm thumb, but no one has 
> bothered about this for i386, m68k, or s390.  Because it just doesn't 
> happen in practice; one has to go out of one's way to design the 
> errant code sequence.

As I can see, Peter M. fixed the translation of page-straddling ARM 
Thumb32 insns in 541ebcd (27 Oct 2015). The fixed code ensures that we 
stop translating if we encounter a page-straddling insn in addition to 
stopping at an insn from a next page. Thus we put a page-straddling insn 
into a separate TB so that the insn fetch exception would report the 
actual PC value for both pages. Looks like any other arch should do the 
same, right?

But, if we forget about page-straddling instructions for a moment, 
shouldn't every arch stop translating when crossing a page boundary? If 
yes then a breakpoint at a page boundary could only translate to an 
empty TB.

>
>
>> At least for those architectures which do stop translating on a page 
>> boundary,
>> I think this patch is applicable. Certainly, it would be better to 
>> have a
>> single solution for all architectures.
>
> For that, I think it might be interesting to arrange for non-empty TBs 
> to exit prior to recognizing a breakpoint.  So that a breakpoint TB is 
> always just the one operation.  Except for the fact that "generate an 
> exception" has traditionally been a target-specific helper, we could 
> almost make the entire breakpoint generation be done in common code.
>
> I'd think something like a generic "must we end the TB now" predicate 
> would be the proper hook.  It would contain all of the usual stuff: 
> tcg_op_buf_full and checks for singlestep, but then add "is there a 
> breakpoint at the next pc".

This could be a next step :)

Thanks,
Sergey

Re: [Qemu-devel] [PATCH] target-*: Get rid of "PC advancement" trick

[Richard Henderson]

On 12/15/2015 01:02 PM, Sergey Fedorov wrote:
>> For that, I think it might be interesting to arrange for non-empty TBs to
>> exit prior to recognizing a breakpoint.  So that a breakpoint TB is always
>> just the one operation.  Except for the fact that "generate an exception" has
>> traditionally been a target-specific helper, we could almost make the entire
>> breakpoint generation be done in common code.
>>
>> I'd think something like a generic "must we end the TB now" predicate would
>> be the proper hook.  It would contain all of the usual stuff: tcg_op_buf_full
>> and checks for singlestep, but then add "is there a breakpoint at the next pc".
>
> This could be a next step :)

Or perhaps a first step, since the patch you posted doesn't seem to me to be an 
improvement at all, merely a rearrangement of code.


r~

Re: [Qemu-devel] [PATCH] target-*: Get rid of "PC advancement" trick

[Sergey Fedorov]

On 12/16/15 20:03, Richard Henderson wrote:
> On 12/15/2015 01:02 PM, Sergey Fedorov wrote:
>>> For that, I think it might be interesting to arrange for non-empty 
>>> TBs to
>>> exit prior to recognizing a breakpoint.  So that a breakpoint TB is 
>>> always
>>> just the one operation.  Except for the fact that "generate an 
>>> exception" has
>>> traditionally been a target-specific helper, we could almost make 
>>> the entire
>>> breakpoint generation be done in common code.
>>>
>>> I'd think something like a generic "must we end the TB now" 
>>> predicate would
>>> be the proper hook.  It would contain all of the usual stuff: 
>>> tcg_op_buf_full
>>> and checks for singlestep, but then add "is there a breakpoint at 
>>> the next pc".
>>
>> This could be a next step :)
>
> Or perhaps a first step, since the patch you posted doesn't seem to me 
> to be an improvement at all, merely a rearrangement of code.

I should give it a thought. The main motivation of this patch was to 
simply suppress useless disas log and the instruction length mismatch 
disas warning in case of an empty TB. The original reason was that there 
is a place in ARM translation with "PC advancement" tick missed.

Thanks,
Sergey