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