Re: [PATCH 2/2] xen/bug: Complete outstanding TODO

From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	 Xen-devel <xen-devel@lists.xenproject.org>,
	 George Dunlap <George.Dunlap@citrix.com>,
	Jan Beulich <JBeulich@suse.com>,
	 Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wl@xen.org>,
	 Federico Serafini <federico.serafini@bugseng.com>
Subject: Re: [PATCH 2/2] xen/bug: Complete outstanding TODO
Date: Wed, 20 Dec 2023 10:58:43 -0800 (PST)	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2312201053180.685950@ubuntu-linux-20-04-desktop> (raw)
In-Reply-To: <139031ab-522c-41d7-b9d7-e9b7d3656555@xen.org>

[-- Attachment #1: Type: text/plain, Size: 2485 bytes --]

On Fri, 15 Dec 2023, Julien Grall wrote:
> Hi Andrew,
> 
> On 15/12/2023 18:14, Andrew Cooper wrote:
> > Since this TODO was written, BUILD_BUG_ON() has been moved out of xen/lib.h
> > into xen/macros.h, which has done all the hard work.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > CC: George Dunlap <George.Dunlap@citrix.com>
> > CC: Jan Beulich <JBeulich@suse.com>
> > CC: Stefano Stabellini <sstabellini@kernel.org>
> > CC: Wei Liu <wl@xen.org>
> > CC: Julien Grall <julien@xen.org>
> > CC: Federico Serafini <federico.serafini@bugseng.com>
> > ---
> >   xen/include/xen/bug.h | 16 +++++++---------
> >   1 file changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
> > index cb5138410ea7..8cca4486a477 100644
> > --- a/xen/include/xen/bug.h
> > +++ b/xen/include/xen/bug.h
> > @@ -20,7 +20,8 @@
> >   #define BUG_DEBUGGER_TRAP_FATAL(regs) 0
> >   #endif
> >   -#include <xen/lib.h>
> > +#include <xen/macros.h>
> > +#include <xen/types.h>
> >     #ifndef BUG_FRAME_STRUCT
> >   @@ -104,14 +105,11 @@ typedef void bug_fn_t(const struct cpu_user_regs
> > *regs);
> >     #ifndef run_in_exception_handler
> >   -/*
> > - * TODO: untangle header dependences, break BUILD_BUG_ON() out of
> > xen/lib.h,
> > - * and use a real static inline here to get proper type checking of fn().
> > - */
> > -#define run_in_exception_handler(fn) do {                   \
> > -    (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \
> > -    BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL);             \
> > -} while ( false )
> > +static void always_inline run_in_exception_handler(
> > +    void (*fn)(struct cpu_user_regs *regs))
> 
> Based on the other threads, shouldn't this be using bug_fn_t?

Unfortunately it doesn't compile:

common/bug.c: In function ‘do_bug_frame’:
common/bug.c:72:38: error: passing argument 1 of ‘run_in_exception_handler’ from incompatible pointer type [-Werror=incompatible-pointer-types]
   72 |             run_in_exception_handler(fn);
      |                                      ^~
      |                                      |
      |                                      void (*)(struct cpu_user_regs *)

due to the missing const in common/bug.c:72.

Not to make things more complicated in this patch, I think we should
take the patch as is as a simple cleanup (and do further cleanups in the
future):

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>