[v1,09/13] target/sh4: revert to using cpu_lduw_code to decode gusa

Message ID 20200709141327.14631-10-alex.bennee@linaro.org
State New
Headers show
Series
  • misc rc0 fixes (docs, plugins, docker)
Related show

Commit Message

Alex Bennée July 9, 2020, 2:13 p.m.
The translator_ld* functions very much expect us to be decoding one
instruction at a time. Otherwise we will see weirdness such as:

  qemu-sh4: warning: plugin_disas: 6 bytes left over

when we use the disas functions. For what SH4 is doing here (scanning
ahead in the instruction stream) this is the right function to use.

Reported-by: Claudio Fontana <cfontana@suse.de>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
 target/sh4/translate.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

-- 
2.20.1

Comments

Philippe Mathieu-Daudé July 9, 2020, 2:42 p.m. | #1
On 7/9/20 4:13 PM, Alex Bennée wrote:
> The translator_ld* functions very much expect us to be decoding one

> instruction at a time. Otherwise we will see weirdness such as:

> 

>   qemu-sh4: warning: plugin_disas: 6 bytes left over

> 

> when we use the disas functions. For what SH4 is doing here (scanning

> ahead in the instruction stream) this is the right function to use.

> 

> Reported-by: Claudio Fontana <cfontana@suse.de>

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>  target/sh4/translate.c | 8 ++++++--

>  1 file changed, 6 insertions(+), 2 deletions(-)

> 

> diff --git a/target/sh4/translate.c b/target/sh4/translate.c

> index 6192d83e8c66..919da72a0c98 100644

> --- a/target/sh4/translate.c

> +++ b/target/sh4/translate.c

> @@ -1915,9 +1915,13 @@ static void decode_gusa(DisasContext *ctx, CPUSH4State *env)

>          goto fail;

>      }

>  

> -    /* Read all of the insns for the region.  */

> +    /*

> +     * Read all of the insns for the region. We do this directly with

> +     * cpu_lduw_code to avoid confusing the plugins by decoding

> +     * multiple instructions.

> +     */

>      for (i = 0; i < max_insns; ++i) {

> -        insns[i] = translator_lduw(env, pc + i * 2);

> +        insns[i] = cpu_lduw_code(env, pc + i * 2);


Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


>      }

>  

>      ld_adr = ld_dst = ld_mop = -1;

>
Richard Henderson July 10, 2020, 9:26 p.m. | #2
On 7/9/20 7:13 AM, Alex Bennée wrote:
> The translator_ld* functions very much expect us to be decoding one

> instruction at a time. Otherwise we will see weirdness such as:

> 

>   qemu-sh4: warning: plugin_disas: 6 bytes left over

> 

> when we use the disas functions. For what SH4 is doing here (scanning

> ahead in the instruction stream) this is the right function to use.


It is not just scanning ahead.  It does that, but after having done so it may
also commit to translating them all at once.

In the case to which you refer, above, we have translated 4 insns into one
operation.  Just having plugin_disas see the first one is not really correct
either.


r~

> 

> Reported-by: Claudio Fontana <cfontana@suse.de>

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>  target/sh4/translate.c | 8 ++++++--

>  1 file changed, 6 insertions(+), 2 deletions(-)

> 

> diff --git a/target/sh4/translate.c b/target/sh4/translate.c

> index 6192d83e8c66..919da72a0c98 100644

> --- a/target/sh4/translate.c

> +++ b/target/sh4/translate.c

> @@ -1915,9 +1915,13 @@ static void decode_gusa(DisasContext *ctx, CPUSH4State *env)

>          goto fail;

>      }

>  

> -    /* Read all of the insns for the region.  */

> +    /*

> +     * Read all of the insns for the region. We do this directly with

> +     * cpu_lduw_code to avoid confusing the plugins by decoding

> +     * multiple instructions.

> +     */

>      for (i = 0; i < max_insns; ++i) {

> -        insns[i] = translator_lduw(env, pc + i * 2);

> +        insns[i] = cpu_lduw_code(env, pc + i * 2);

>      }

>  

>      ld_adr = ld_dst = ld_mop = -1;

>

Patch

diff --git a/target/sh4/translate.c b/target/sh4/translate.c
index 6192d83e8c66..919da72a0c98 100644
--- a/target/sh4/translate.c
+++ b/target/sh4/translate.c
@@ -1915,9 +1915,13 @@  static void decode_gusa(DisasContext *ctx, CPUSH4State *env)
         goto fail;
     }
 
-    /* Read all of the insns for the region.  */
+    /*
+     * Read all of the insns for the region. We do this directly with
+     * cpu_lduw_code to avoid confusing the plugins by decoding
+     * multiple instructions.
+     */
     for (i = 0; i < max_insns; ++i) {
-        insns[i] = translator_lduw(env, pc + i * 2);
+        insns[i] = cpu_lduw_code(env, pc + i * 2);
     }
 
     ld_adr = ld_dst = ld_mop = -1;