Re: [PATCH] target/arm: Display helpful message when hflags mismatch
On 12/17/19 5:08 PM, Peter Maydell wrote: On Tue, 17 Dec 2019 at 16:02, Richard Henderson wrote: On Mon, 9 Dec 2019 at 03:46, Philippe Mathieu-Daudé 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é --- target/arm/helper.c | 18 +++--- 1 file changed, 15 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson Applied to target-arm.next, thanks. Thanks, you can also add (from a different thread): Tested-by: Niek Linnenbank
Re: [PATCH] target/arm: Display helpful message when hflags mismatch
On Tue, 17 Dec 2019 at 16:02, Richard Henderson wrote: > > On Mon, 9 Dec 2019 at 03:46, Philippe Mathieu-Daudé 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é > > --- > > target/arm/helper.c | 18 +++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > Reviewed-by: Richard Henderson Applied to target-arm.next, thanks. -- PMM
Re: [PATCH] target/arm: Display helpful message when hflags mismatch
On Mon, 9 Dec 2019 at 03:46, Philippe Mathieu-Daudé 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é > --- > target/arm/helper.c | 18 +++--- > 1 file changed, 15 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH] target/arm: Display helpful message when hflags mismatch
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
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
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
Patchew URL: https://patchew.org/QEMU/20191209134552.27733-1-phi...@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 real5m12.306s user0m2.438s The full log is available at http://patchew.org/logs/20191209134552.27733-1-phi...@redhat.com/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH] target/arm: Display helpful message when hflags mismatch
Patchew URL: https://patchew.org/QEMU/20191209134552.27733-1-phi...@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 real4m52.604s user0m2.594s The full log is available at http://patchew.org/logs/20191209134552.27733-1-phi...@redhat.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH] target/arm: Display helpful message when hflags mismatch
Philippe Mathieu-Daudé 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é
> ---
> 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
[PATCH] target/arm: Display helpful message when hflags mismatch
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é
---
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
