[PATCH] target/arm: Display helpful message when hflags mismatch

[PATCH] target/arm: Display helpful message when hflags mismatch

[Philippe Mathieu-Daudé]

Instead of crashing in a confuse way, give some hint to the user
about why we aborted. He might report the issue without having
to use a debugger.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/arm/helper.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0bf8f53d4b..6bfb62672b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11348,6 +11348,20 @@ void HELPER(rebuild_hflags_a64)(CPUARMState *env, int el)
     env->hflags = rebuild_hflags_a64(env, el, fp_el, mmu_idx);
 }
 
+static inline void assert_hflags_rebuild_correctly(CPUARMState *env)
+{
+#ifdef CONFIG_DEBUG_TCG
+    uint32_t env_flags_current = env->hflags;
+    uint32_t env_flags_rebuilt = rebuild_hflags_internal(env);
+
+    if (unlikely(env_flags_current != env_flags_rebuilt)) {
+        fprintf(stderr, "TCG hflags mismatch (current:0x%08x rebuilt:0x%08x)\n",
+                env_flags_current, env_flags_rebuilt);
+        abort();
+    }
+#endif
+}
+
 void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
                           target_ulong *cs_base, uint32_t *pflags)
 {
@@ -11355,9 +11369,7 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
     uint32_t pstate_for_ss;
 
     *cs_base = 0;
-#ifdef CONFIG_DEBUG_TCG
-    assert(flags == rebuild_hflags_internal(env));
-#endif
+    assert_hflags_rebuild_correctly(env);
 
     if (FIELD_EX32(flags, TBFLAG_ANY, AARCH64_STATE)) {
         *pc = env->pc;
-- 
2.21.0

Re: [PATCH] target/arm: Display helpful message when hflags mismatch

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Instead of crashing in a confuse way, give some hint to the user
> about why we aborted. He might report the issue without having
> to use a debugger.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  target/arm/helper.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0bf8f53d4b..6bfb62672b 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11348,6 +11348,20 @@ void HELPER(rebuild_hflags_a64)(CPUARMState *env, int el)
>      env->hflags = rebuild_hflags_a64(env, el, fp_el, mmu_idx);
>  }
>  
> +static inline void assert_hflags_rebuild_correctly(CPUARMState *env)
> +{
> +#ifdef CONFIG_DEBUG_TCG
> +    uint32_t env_flags_current = env->hflags;
> +    uint32_t env_flags_rebuilt = rebuild_hflags_internal(env);
> +
> +    if (unlikely(env_flags_current != env_flags_rebuilt)) {
> +        fprintf(stderr, "TCG hflags mismatch (current:0x%08x rebuilt:0x%08x)\n",
> +                env_flags_current, env_flags_rebuilt);
> +        abort();
> +    }
> +#endif
> +}
> +
>  void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>                            target_ulong *cs_base, uint32_t *pflags)
>  {
> @@ -11355,9 +11369,7 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>      uint32_t pstate_for_ss;
>  
>      *cs_base = 0;
> -#ifdef CONFIG_DEBUG_TCG
> -    assert(flags == rebuild_hflags_internal(env));
> -#endif
> +    assert_hflags_rebuild_correctly(env);

I'm trying to recall why we don't just use:

  g_assert_cmphex(flags, =, rebuild_hflags_internal(env))

I think it came up in one of the reviews.

>  
>      if (FIELD_EX32(flags, TBFLAG_ANY, AARCH64_STATE)) {
>          *pc = env->pc;


-- 
Alex Bennée

Re: [PATCH] target/arm: Display helpful message when hflags mismatch

[Richard Henderson]

On 12/9/19 8:00 AM, Alex Bennée wrote:
>> -#ifdef CONFIG_DEBUG_TCG
>> -    assert(flags == rebuild_hflags_internal(env));
>> -#endif
>> +    assert_hflags_rebuild_correctly(env);
> 
> I'm trying to recall why we don't just use:
> 
>   g_assert_cmphex(flags, =, rebuild_hflags_internal(env))
> 
> I think it came up in one of the reviews.

checkpatch.pl.

Because, I believe, there is an environment variable that causes this to *not*
abort on mismatch.


r~

Re: [PATCH] target/arm: Display helpful message when hflags mismatch

[Philippe Mathieu-Daudé]

On 12/12/19 1:36 AM, Richard Henderson wrote:
> On 12/9/19 8:00 AM, Alex Bennée wrote:
>>> -#ifdef CONFIG_DEBUG_TCG
>>> -    assert(flags == rebuild_hflags_internal(env));
>>> -#endif
>>> +    assert_hflags_rebuild_correctly(env);
>>
>> I'm trying to recall why we don't just use:
>>
>>    g_assert_cmphex(flags, =, rebuild_hflags_internal(env))

s/=/==/ ;)

>>
>> I think it came up in one of the reviews.
> 
> checkpatch.pl.
> 
> Because, I believe, there is an environment variable that causes this to *not*
> abort on mismatch.

Indeed, see commit 6e9389563e5:

commit 6e9389563e56607f72562bdb72db452fcd7e7f74
Author: Dr. David Alan Gilbert <dgilbert@redhat.com>
Date:   Thu Apr 27 17:55:26 2017 +0100

     checkpatch: Disallow glib asserts in main code

     Glib commit a6a875068779 (from 2013) made many of the glib assert
     macros non-fatal if a flag is set.
     This causes two problems:
       a) Compilers moan that your code is unsafe even though you've
          put an assert in before the point of use.
       b) Someone evil could, in a library, call
          g_test_set_nonfatal_assertions() and cause our assertions in
          important places not to fail and potentially allow memory 
overruns.

     Ban most of the glib assertion functions (basically everything except
     g_assert and g_assert_not_reached) except in tests/

     This makes checkpatch gives an error such as:

       ERROR: Use g_assert or g_assert_not_reached
       #77: FILE: vl.c:4725:
       +    g_assert_cmpstr("Chocolate", >, "Cheese");

Re: [PATCH] target/arm: Display helpful message when hflags mismatch

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191209134552.27733-1-philmd@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
remote: Counting objects: 5280, done.        
error: RPC failed; result=18, HTTP code = 200
fatal: The remote end hung up unexpectedly
fatal: protocol error: bad pack header
Clone of 'https://git.qemu.org/git/dtc.git' into submodule path 'dtc' failed
failed to update submodule dtc
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) unregistered for path 'dtc'
make[1]: *** [/var/tmp/patchew-tester-tmp-8vki3gda/src/docker-src.2019-12-09-14.00.09.13536] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-8vki3gda/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    4m52.604s
user    0m2.594s


The full log is available at
http://patchew.org/logs/20191209134552.27733-1-philmd@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH] target/arm: Display helpful message when hflags mismatch

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191209134552.27733-1-philmd@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
remote: Counting objects: 5280, done.        
error: RPC failed; result=18, HTTP code = 200
fatal: The remote end hung up unexpectedly
fatal: protocol error: bad pack header
Clone of 'https://git.qemu.org/git/dtc.git' into submodule path 'dtc' failed
failed to update submodule dtc
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) unregistered for path 'dtc'
make[1]: *** [/var/tmp/patchew-tester-tmp-_5dpbvna/src/docker-src.2019-12-09-14.05.27.14518] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-_5dpbvna/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    5m12.306s
user    0m2.438s


The full log is available at
http://patchew.org/logs/20191209134552.27733-1-philmd@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH] target/arm: Display helpful message when hflags mismatch

[Richard Henderson]

On Mon, 9 Dec 2019 at 03:46, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Instead of crashing in a confuse way, give some hint to the user
> about why we aborted. He might report the issue without having
> to use a debugger.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  target/arm/helper.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH] target/arm: Display helpful message when hflags mismatch

[Peter Maydell]

On Tue, 17 Dec 2019 at 16:02, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On Mon, 9 Dec 2019 at 03:46, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >
> > Instead of crashing in a confuse way, give some hint to the user
> > about why we aborted. He might report the issue without having
> > to use a debugger.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  target/arm/helper.c | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH] target/arm: Display helpful message when hflags mismatch

[Philippe Mathieu-Daudé]

On 12/17/19 5:08 PM, Peter Maydell wrote:
> On Tue, 17 Dec 2019 at 16:02, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On Mon, 9 Dec 2019 at 03:46, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>
>>> Instead of crashing in a confuse way, give some hint to the user
>>> about why we aborted. He might report the issue without having
>>> to use a debugger.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>   target/arm/helper.c | 18 +++++++++++++++---
>>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> 
> Applied to target-arm.next, thanks.

Thanks, you can also add (from a different thread):
Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com>