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
