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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2023-04-04 at 08:41 +0200, Jan Beulich wrote:
> On 03.04.2023 20:40, Oleksii wrote:
> > 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.
> 
> RISC-V doesn't know of a '#' prefix, does it? '#' is a comment
> character
> there afaik, like for many other architectures.
It doesn't and for RISC-V it's a comment character.

afaik '%c' is needed to skip prefix('sign' ) (# or $ ( in case of
x86)).

I mean that RISC-V doesn't put anything before immediate so there is no
need to use %c as we don't need to skip prefix/'sign' before immediate
but if start to use '%c' it will cause an compiler issue.

~ Oleksii