| Message ID | 20191209134552.27733-1-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Display helpful message when hflags mismatch | expand |
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;
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
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
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~
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");
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~
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
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>
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;
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(-)