Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
On Wed, 5 Apr 2023, Julien Grall wrote: > On 03/04/2023 00:15, Stefano Stabellini wrote: > > On Fri, 31 Mar 2023, 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 from the current header: > > > > + * > > > > + * : > > > > + * ... > > > > + * : > > > > + * ... > > > > + * > > > > + * ... > > > > + * ... > > > > + * #define BUG() ... > > > > + * ... > > > > + * #define ASSERT() ... > > > > + * ... > > > > + */ > > > > + > > > >#include > > > > #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" > > > > > > ..
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
Hi, On 06/04/2023 07:35, Jan Beulich wrote: On 05.04.2023 18:39, Julien Grall wrote: To reduce the amount of patch to resend, I was actually thinking to merge patch #1-3 and #5 (so leave this patch alone) and modify the default in a follow-up. Any thoughts? Well, yes, that's what I did a couple of days ago already. Ah. I didn't check the tree when replying. So ignore me. Cheers, -- Julien Grall
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
On 05.04.2023 18:39, Julien Grall wrote: > To reduce the amount of patch to resend, I was actually thinking to > merge patch #1-3 and #5 (so leave this patch alone) and modify the > default in a follow-up. Any thoughts? Well, yes, that's what I did a couple of days ago already. Jan
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
Hi, On 04/04/2023 09:09, Oleksii wrote: 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 from the current header: + * + * : + * ... + * : + * ... + * + * ... + * ... + * #define BUG() ... + * ... + * #define ASSERT() ... + * ... + */ + #include #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
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
Hi, On 03/04/2023 00:15, Stefano Stabellini wrote: On Fri, 31 Mar 2023, 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 from the current header: + * + * : + * ... + * : + * ... + * + * ... + * ... + * #define BUG() ... + * ... + * #define ASSERT() ... + * ... + */ + #include #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). 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? I don't know, but I confirm your observation. In my system, both ARM64 and ARM32 compile without problems with "c". Without "c', only ARM64 compiles without problems, while ARM32 breaks. My ARM32 compiler is: arm-linux-gnueabihf-gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0 Additing a meaningful explanation to the commit message might be difficult in this case. Agree. One would need to look at the compiler code to confirm. We sho
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
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 from the current header: > > > > + * > > > > + * : > > > > + * ... > > > > + * : > > > > + * ... > > > > + * > > > > + * ... > > > > + * ... > > > > + * #define BUG() ... > > > > + * ... > > > > + * #define ASSERT() ... > > > > + * ... > > > > + */ > > > > + > > > > #include > > > > > > > > #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" \ > > > > - > > >
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
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 from the current header: >>> + * >>> + * : >>> + * ... >>> + * : >>> + * ... >>> + * >>> + * ... >>> + * ... >>> + * #define BUG() ... >>> + * ... >>> + * #define ASSERT() ... >>> + * ... >>> + */ >>> + >>> #include >>> >>> #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) >
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
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 from the current header: > > + * > > + * : > > + * ... > > + * : > > + * ... > > + * > > + * ... > > + * ... > > + * #define BUG() ... > > + * ... > > + * #define ASSERT() ... > > + * ... > > + */ > > + > > #include > > > > #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_w
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
On Fri, 31 Mar 2023, 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 from the current header: > > + * > > + * : > > + * ... > > + * : > > + * ... > > + * > > + * ... > > + * ... > > + * #define BUG() ... > > + * ... > > + * #define ASSERT() ... > > + * ... > > + */ > > + > > #include > > #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). > > 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 b
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
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 from the current header: + * + * : + * ... + * : + * ... + * + * ... + * ... + * #define BUG() ... + * ... + * #define ASSERT() ... + * ... + */ + #include #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 instanc