| Message ID | d14e4ffc-d07b-4a60-8949-ecdcea86cd77@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86emul: drop and avoid use of BUG() | expand |
On 23/08/2024 8:16 am, Jan Beulich wrote: > Generally it is not a good idea to use BUG() in emulator code. Even for > internal flaws we're better off returning errors to callers, rather than > crashing the system. Replace the sole remaining use and remove the > test / fuzzing harness surrogate. Put in place a declaration pleasing > the compiler when finding uses in Xen headers, while at the same time > breaking the build (at linking time) in case an active reference would > newly appear. > > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/tools/tests/x86_emulator/x86-emulate.h +++ b/tools/tests/x86_emulator/x86-emulate.h @@ -42,7 +42,6 @@ #include <xen-tools/common-macros.h> -#define BUG() abort() #define ASSERT assert #define ASSERT_UNREACHABLE() assert(!__LINE__) --- a/xen/arch/x86/x86_emulate/decode.c +++ b/xen/arch/x86/x86_emulate/decode.c @@ -1122,7 +1122,9 @@ int x86emul_decode(struct x86_emulate_st switch ( def_ad_bytes ) { default: - BUG(); /* Shouldn't be possible. */ + ASSERT_UNREACHABLE(); /* Shouldn't be possible. */ + return X86EMUL_UNHANDLEABLE; + case 2: if ( ctxt->regs->eflags & X86_EFLAGS_VM ) break; --- a/xen/arch/x86/x86_emulate/private.h +++ b/xen/arch/x86/x86_emulate/private.h @@ -15,6 +15,9 @@ # include <asm/x86-vendors.h> # include <asm/x86_emulate.h> +# undef BUG /* Make sure it's not used anywhere here. */ +void BUG(void); + # ifndef CONFIG_HVM # define X86EMUL_NO_FPU # define X86EMUL_NO_MMX
Generally it is not a good idea to use BUG() in emulator code. Even for internal flaws we're better off returning errors to callers, rather than crashing the system. Replace the sole remaining use and remove the test / fuzzing harness surrogate. Put in place a declaration pleasing the compiler when finding uses in Xen headers, while at the same time breaking the build (at linking time) in case an active reference would newly appear. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- The #undef and override may be going a little too far, but I guess we can revisit this if and when it actually gets in the way. While BUG_ON() references BUG() and is hence covered, we may want to consider to #undef/declare that separately. That way the linker error would make clear which one it is that was referenced. I could use EXPECT() instead of kind of open-coding it, but I don't really like EXPECT(false) or variations thereof.