| Message ID | 8fdb98350ae4fc6029738d0aabe13a57e1945a50.1680086655.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | introduce generic implementation of macros from bug.h | expand |
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). 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
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 <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). > > 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. Maybe instead we could run a few tests with different versions of arm64 and arm32 gcc to check that everything still works? If everything checks out, given that the issue has been unchanged for 10+ years we could just keep "c" and move forward with it?
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
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
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
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 <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). >> >> 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 should at least acknowledge the change in the commit message and also... > > Maybe instead we could run a few tests with different versions of arm64 > and arm32 gcc to check that everything still works? If everything checks > out, given that the issue has been unchanged for 10+ years we could just > keep "c" and move forward with it? ... confirm that we are still able to compile with GCC 4.9 (arm32) and GCC 5.1 (arm64). Do we have those compiler in the CI? (I couldn't easily confirm from the automation directory). Cheers,
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 <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. I am a bit confused with this answer. My point was that on at least on Arm 32-bit we need to use 'c' otherwise it breaks. AFAIU, this is not the same problem on 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. ... because here you say it will break when using 'c'. Did I miss anything? Anyway, it sounds like to me that the default definition in xen/bug.h should be using 'c' rather than been empty because this seems to be the more common approach. 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? Cheers,
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
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,
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 <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). > > > > > > 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 should at > least acknowledge the change in the commit message and also... > > > > > Maybe instead we could run a few tests with different versions of arm64 > > and arm32 gcc to check that everything still works? If everything checks > > out, given that the issue has been unchanged for 10+ years we could just > > keep "c" and move forward with it? > > ... confirm that we are still able to compile with GCC 4.9 (arm32) and GCC 5.1 > (arm64). > > Do we have those compiler in the CI? (I couldn't easily confirm from the > automation directory). In the CI we have: - arm64v8/alpine:3.12, gcc 9.3.0 - arm64v8/debian:unstable, gcc 12.2.0 - arm64v8/debian:unstable cross arm32, gcc 12.2.0 - yocto, not sure exactly but it is going to be 9.x or newer
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index 239d3aed3c..aad6644a7b 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -12,6 +12,7 @@ config ARM_64 config ARM def_bool y + select GENERIC_BUG_FRAME select HAS_ALTERNATIVE select HAS_DEVICE_TREE select HAS_PASSTHROUGH diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c index a2fc1c22cb..61c61132c7 100644 --- a/xen/arch/arm/arm32/traps.c +++ b/xen/arch/arm/arm32/traps.c @@ -48,7 +48,7 @@ void do_trap_undefined_instruction(struct cpu_user_regs *regs) if ( instr != BUG_OPCODE ) goto die; - if ( do_bug_frame(regs, pc) ) + if ( do_bug_frame(regs, pc) < 0 ) goto die; regs->pc += 4; diff --git a/xen/arch/arm/include/asm/arm32/bug.h b/xen/arch/arm/include/asm/arm32/bug.h index 25cce151dc..3e66f35969 100644 --- a/xen/arch/arm/include/asm/arm32/bug.h +++ b/xen/arch/arm/include/asm/arm32/bug.h @@ -10,6 +10,4 @@ #define BUG_INSTR ".word " __stringify(BUG_OPCODE) -#define BUG_FN_REG r0 - #endif /* __ARM_ARM32_BUG_H__ */ diff --git a/xen/arch/arm/include/asm/arm64/bug.h b/xen/arch/arm/include/asm/arm64/bug.h index 5e11c0dfd5..59f664d7de 100644 --- a/xen/arch/arm/include/asm/arm64/bug.h +++ b/xen/arch/arm/include/asm/arm64/bug.h @@ -6,6 +6,4 @@ #define BUG_INSTR "brk " __stringify(BRK_BUG_FRAME_IMM) -#define BUG_FN_REG x0 - #endif /* __ARM_ARM64_BUG_H__ */ 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. - */ -#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" #endif /* __ARM_BUG_H__ */ /* diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h index 883dae368e..c6518008ec 100644 --- a/xen/arch/arm/include/asm/traps.h +++ b/xen/arch/arm/include/asm/traps.h @@ -69,8 +69,6 @@ void do_cp(struct cpu_user_regs *regs, const union hsr hsr); void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr); void do_trap_hvc_smccc(struct cpu_user_regs *regs); -int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc); - void noreturn do_unexpected_trap(const char *msg, const struct cpu_user_regs *regs); void do_trap_hyp_sync(struct cpu_user_regs *regs); diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 061c92acbd..9c6eb66422 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1197,85 +1197,6 @@ void do_unexpected_trap(const char *msg, const struct cpu_user_regs *regs) panic("CPU%d: Unexpected Trap: %s\n", smp_processor_id(), msg); } -int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc) -{ - const struct bug_frame *bug = NULL; - const char *prefix = "", *filename, *predicate; - unsigned long fixup; - int id = -1, lineno; - const struct virtual_region *region; - - region = find_text_region(pc); - if ( region ) - { - for ( id = 0; id < BUGFRAME_NR; id++ ) - { - const struct bug_frame *b; - unsigned int i; - - for ( i = 0, b = region->frame[id].bugs; - i < region->frame[id].n_bugs; b++, i++ ) - { - if ( ((vaddr_t)bug_loc(b)) == pc ) - { - bug = b; - goto found; - } - } - } - } - found: - if ( !bug ) - return -ENOENT; - - if ( id == BUGFRAME_run_fn ) - { - void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG; - - fn(regs); - return 0; - } - - /* WARN, BUG or ASSERT: decode the filename pointer and line number. */ - filename = bug_file(bug); - if ( !is_kernel(filename) ) - return -EINVAL; - fixup = strlen(filename); - if ( fixup > 50 ) - { - filename += fixup - 47; - prefix = "..."; - } - lineno = bug_line(bug); - - switch ( id ) - { - case BUGFRAME_warn: - printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); - show_execution_state(regs); - return 0; - - case BUGFRAME_bug: - printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); - show_execution_state(regs); - panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno); - - case BUGFRAME_assert: - /* ASSERT: decode the predicate string pointer. */ - predicate = bug_msg(bug); - if ( !is_kernel(predicate) ) - predicate = "<unknown>"; - - printk("Assertion '%s' failed at %s%s:%d\n", - predicate, prefix, filename, lineno); - show_execution_state(regs); - panic("Assertion '%s' failed at %s%s:%d\n", - predicate, prefix, filename, lineno); - } - - return -EINVAL; -} - #ifdef CONFIG_ARM_64 static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr) { @@ -1292,7 +1213,7 @@ static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr) switch ( hsr.brk.comment ) { case BRK_BUG_FRAME_IMM: - if ( do_bug_frame(regs, regs->pc) ) + if ( do_bug_frame(regs, regs->pc) < 0 ) goto die; regs->pc += 4;
The following changes were made: * make GENERIC_BUG_FRAME mandatory for ARM * As do_bug_frame() returns -EINVAL in case something goes wrong otherwise id of bug frame. Thereby 'if' cases where do_bug_frame() was updated to check if the returned value is less than 0 * Switch ARM's implementation of bug.h macros to generic one Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V9: * add additional explanation to <asm/bug.h> header --- Changes in V8: * Nothing changed. --- Changes in V7: * Rebase the patch. --- Changes in V6: * Update the "changes in v5" * Rebase on top of the patch [xen: introduce CONFIG_GENERIC_BUG_FRAME] as there were minor changes. --- Changes in V5: * common/bug.c changes were removed after rebase (the patch [xen: introduce CONFIG_GENERIC_BUG_FRAME] was reworked to make ARM implementation to use generic do_bug_frame()) --- Changes in V4: * Switch ARM implementation to generic one * Remove BUG_FN_REG from arm{16,32}/bug.h as it isn't needed after switch to generic implementation * Update commit message --- Changes in V3: * As prototype and what do_bug_frame() returns was changed so patch 3 and 4 was updated to use a new version of do_bug_frame --- Changes in V2: * Rename bug_file() in ARM implementation to bug_ptr() as generic do_bug_frame() uses bug_ptr(). * Remove generic parts from bug.h * Remove declaration of 'int do_bug_frame(...)' from <asm/traps.h> as it was introduced in <xen/bug.h> --- xen/arch/arm/Kconfig | 1 + xen/arch/arm/arm32/traps.c | 2 +- xen/arch/arm/include/asm/arm32/bug.h | 2 - xen/arch/arm/include/asm/arm64/bug.h | 2 - xen/arch/arm/include/asm/bug.h | 89 ++++++---------------------- xen/arch/arm/include/asm/traps.h | 2 - xen/arch/arm/traps.c | 81 +------------------------ 7 files changed, 22 insertions(+), 157 deletions(-)