Re: [PATCH v2 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-02-23 at 14:32 +0100, Jan Beulich wrote:
> On 20.02.2023 17:40, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/xen/bug.h
> > @@ -0,0 +1,161 @@
> > +#ifndef __XEN_BUG_H__
> > +#define __XEN_BUG_H__
> > +
> > +#define BUG_DISP_WIDTH    24
> > +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> > +
> > +#define BUGFRAME_run_fn 0
> > +#define BUGFRAME_warn   1
> > +#define BUGFRAME_bug    2
> > +#define BUGFRAME_assert 3
> > +
> > +#define BUGFRAME_NR     4
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +#include <xen/errno.h>
> > +#include <xen/lib.h>
> > +#include <xen/stringify.h>
> > +#include <xen/types.h>
> > +
> > +#include <asm/bug.h>
> 
> Looking at patch 2 onwards I came to wonder: How does that work
> without
> you first removing stuff from the respective asm/bug.h (or adding
> suitable #define-s to limit what gets defined below)? From all I can
> tell the compiler should object to ...
> 
> > +#ifndef BUG_FRAME_STRUCT
> > +
> > +struct bug_frame {
> > +    signed int loc_disp:BUG_DISP_WIDTH;
> > +    unsigned int line_hi:BUG_LINE_HI_WIDTH;
> > +    signed int ptr_disp:BUG_DISP_WIDTH;
> > +    unsigned int line_lo:BUG_LINE_LO_WIDTH;
> > +    signed int msg_disp[];
> > +};
> 
> ... this, as asm/bug.h will have declared such a struct already. The
> #define-s further down may not be a problem if what they expand to
> matches in both places, but that's then still a latent trap to fall
> into.
My fault. It doesn't work. I checked only RISC-V arch before and didn't
start all the test for patch 2...

So yeah, in patch 2 should be updated asm/bug.h headers with define
BUG_FRAME_STRUCT and remove all common parts [ BUG_DISP_WIDTH,
BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH, BUGFRAME_run_fn, BUGFRAME_warn,
BUGFRAME_bug, BUGFRAME_assert, BUGFRAME_NR,
{__start|__stop}_bug_frames{ | _{0-3} }[], ].

Thanks for noticing that.
> 
> Jan
~ Oleksii