Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hello Julien, 
On Fri, 2023-03-31 at 22:05 +0100, Julien Grall wrote:
> Hi Oleksii,
> 
> I was going to ack the patch but then I spotted something that would 
> want some clarification.
> 
> On 29/03/2023 11:50, Oleksii Kurochko wrote:
> > diff --git a/xen/arch/arm/include/asm/bug.h
> > b/xen/arch/arm/include/asm/bug.h
> > index cacaf014ab..3fb0471a9b 100644
> > --- a/xen/arch/arm/include/asm/bug.h
> > +++ b/xen/arch/arm/include/asm/bug.h
> > @@ -1,6 +1,24 @@
> >   #ifndef __ARM_BUG_H__
> >   #define __ARM_BUG_H__
> >   
> > +/*
> > + * Please do not include in the header any header that might
> > + * use BUG/ASSERT/etc maros asthey will be defined later after
> > + * the return to <xen/bug.h> from the current header:
> > + *
> > + * <xen/bug.h>:
> > + *  ...
> > + *   <asm/bug.h>:
> > + *     ...
> > + *     <any_header_which_uses_BUG/ASSERT/etc macros.h>
> > + *     ...
> > + *  ...
> > + *  #define BUG() ...
> > + *  ...
> > + *  #define ASSERT() ...
> > + *  ...
> > + */
> > +
> >   #include <xen/types.h>
> >   
> >   #if defined(CONFIG_ARM_32)
> > @@ -11,76 +29,7 @@
> >   # error "unknown ARM variant"
> >   #endif
> >   
> > -#define BUG_FRAME_STRUCT
> > -
> > -struct bug_frame {
> > -    signed int loc_disp;    /* Relative address to the bug address
> > */
> > -    signed int file_disp;   /* Relative address to the filename */
> > -    signed int msg_disp;    /* Relative address to the predicate
> > (for ASSERT) */
> > -    uint16_t line;          /* Line number */
> > -    uint32_t pad0:16;       /* Padding for 8-bytes align */
> > -};
> > -
> > -#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
> > -#define bug_file(b) ((const void *)(b) + (b)->file_disp);
> > -#define bug_line(b) ((b)->line)
> > -#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
> > -
> > -/* Many versions of GCC doesn't support the asm %c parameter which
> > would
> > - * be preferable to this unpleasantness. We use mergeable string
> > - * sections to avoid multiple copies of the string appearing in
> > the
> > - * Xen image. BUGFRAME_run_fn needs to be handled separately.
> > - */
> 
> Given this comment ...
> 
> > -#define BUG_FRAME(type, line, file, has_msg, msg) do
> > {                      \
> > -    BUILD_BUG_ON((line) >>
> > 16);                                             \
> > -    BUILD_BUG_ON((type) >=
> > BUGFRAME_NR);                                    \
> > -    asm
> > ("1:"BUG_INSTR"\n"                                                 
> > \
> > -         ".pushsection .rodata.str, \"aMS\", %progbits,
> > 1\n"                \
> > -         "2:\t.asciz " __stringify(file)
> > "\n"                               \
> > -        
> > "3:\n"                                                            
> > \
> > -         ".if " #has_msg
> > "\n"                                               \
> > -         "\t.asciz " #msg
> > "\n"                                              \
> > -        
> > ".endif\n"                                                        
> > \
> > -        
> > ".popsection\n"                                                   
> > \
> > -         ".pushsection .bug_frames." __stringify(type) ", \"a\",
> > %progbits\n"\
> > -        
> > "4:\n"                                                            
> > \
> > -         ".p2align
> > 2\n"                                                     \
> > -         ".long (1b -
> > 4b)\n"                                                \
> > -         ".long (2b -
> > 4b)\n"                                                \
> > -         ".long (3b -
> > 4b)\n"                                                \
> > -         ".hword " __stringify(line) ",
> > 0\n"                                \
> > -        
> > ".popsection");                                                   
> > \
> > -} while (0)
> > -
> > -/*
> > - * GCC will not allow to use "i"  when PIE is enabled (Xen doesn't
> > set the
> > - * flag but instead rely on the default value from the compiler).
> > So the
> > - * easiest way to implement run_in_exception_handler() is to pass
> > the to
> > - * be called function in a fixed register.
> > - */
> > -#define  run_in_exception_handler(fn) do
> > {                                  \
> > -    asm ("mov " __stringify(BUG_FN_REG) ",
> > %0\n"                            \
> > -        
> > "1:"BUG_INSTR"\n"                                                 
> > \
> > -         ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn)
> > ","       \
> > -         "             \"a\",
> > %%progbits\n"                                 \
> > -        
> > "2:\n"                                                            
> > \
> > -         ".p2align
> > 2\n"                                                     \
> > -         ".long (1b -
> > 2b)\n"                                                \
> > -         ".long 0, 0,
> > 0\n"                                                  \
> > -         ".popsection" :: "r" (fn) : __stringify(BUG_FN_REG)
> > );             \
> > -} while (0)
> > -
> > -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")
> > -
> > -#define BUG() do {                                              \
> > -    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, "");        \
> > -    unreachable();                                              \
> > -} while (0)
> > -
> > -#define assert_failed(msg) do {                                 \
> > -    BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg);     \
> > -    unreachable();                                              \
> > -} while (0)
> > +#define BUG_ASM_CONST   "c"
> 
> ... you should explain in the commit message why this is needed and
> the 
> problem described above is not a problem anymore.
> 
> For instance, I managed to build it without 'c' on arm64 [1]. But it 
> does break on arm32 [2]. I know that Arm is also where '%c' was an
> issue.
> 
> Skimming through linux, the reason seems to be that GCC may add '#'
> when 
> it should not. That said, I haven't look at the impact on the generic
> implementation. Neither I looked at which version may be affected
> (the 
> original message was from 2011).
You are right that some compilers add '#' when it shouldn't. The same
thing happens with RISC-V.

So I'll update both the commit message and comment.

> 
> However, without an explanation, I am afraid this can't go in because
> I 
> am worry we may break some users (thankfully that might just be a 
> compilation issues rather than weird behavior).
> 
> Bertrand, Stefano, do you know if this is still an issue?
> 
> Cheers,
> 
> [1] aarch64-linux-gnu-gcc (Linaro GCC 7.5-2019.12) 7.5.0
> [2] arm-none-linux-gnueabihf-gcc (GNU Toolchain for the A-profile 
> Architecture 10.3-2021.07 (arm-10.29)) 10.3.1 20210621
> 
~ Oleksii