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.

--- 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

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>