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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan