[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
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 <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,
--
Julien Grall
|
