Add support to run a function in an exception handler for Arm. Do it
the same way as on x86 via a bug_frame.
Use the same BUGFRAME_* #defines as on x86 in order to make a future
common header file more easily achievable.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4:
- new patch
V5:
- adjust BUG_FRAME() macro (Jan Beulich)
- adjust arm linker script (Jan Beulich)
- drop #ifdef x86 in common/virtual_region.c
I have verified the created bugframes are correct by inspecting the
created binary.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/arm/traps.c | 10 ++++++++-
xen/arch/arm/xen.lds.S | 2 ++
xen/common/virtual_region.c | 2 --
xen/include/asm-arm/bug.h | 45 +++++++++++++++++--------------------
4 files changed, 32 insertions(+), 27 deletions(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 22bd1bd4c6..912b9a3d77 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1236,8 +1236,16 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
if ( !bug )
return -ENOENT;
+ if ( id == BUGFRAME_run_fn )
+ {
+ void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
+
+ fn(regs);
+ return 0;
+ }
+
/* WARN, BUG or ASSERT: decode the filename pointer and line number. */
- filename = bug_file(bug);
+ filename = bug_ptr(bug);
if ( !is_kernel(filename) )
return -EINVAL;
fixup = strlen(filename);
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 6342ac4ead..004b182acb 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -49,6 +49,8 @@ SECTIONS
__stop_bug_frames_1 = .;
*(.bug_frames.2)
__stop_bug_frames_2 = .;
+ *(.bug_frames.3)
+ __stop_bug_frames_3 = .;
*(.rodata)
*(.rodata.*)
*(.data.rel.ro)
diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
index 4fbc02e35a..30b0b4ab9c 100644
--- a/xen/common/virtual_region.c
+++ b/xen/common/virtual_region.c
@@ -123,9 +123,7 @@ void __init setup_virtual_regions(const struct exception_table_entry *start,
__stop_bug_frames_0,
__stop_bug_frames_1,
__stop_bug_frames_2,
-#ifdef CONFIG_X86
__stop_bug_frames_3,
-#endif
NULL
};
diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h
index 36c803357c..4610835ac3 100644
--- a/xen/include/asm-arm/bug.h
+++ b/xen/include/asm-arm/bug.h
@@ -15,65 +15,62 @@
struct bug_frame {
signed int loc_disp; /* Relative address to the bug address */
- signed int file_disp; /* Relative address to the filename */
+ signed int ptr_disp; /* Relative address to the filename or function */
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_ptr(b) ((const void *)(b) + (b)->ptr_disp);
#define bug_line(b) ((b)->line)
#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
-#define BUGFRAME_warn 0
-#define BUGFRAME_bug 1
-#define BUGFRAME_assert 2
+#define BUGFRAME_run_fn 0
+#define BUGFRAME_warn 1
+#define BUGFRAME_bug 2
+#define BUGFRAME_assert 3
-#define BUGFRAME_NR 3
+#define BUGFRAME_NR 4
/* 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.
*/
-#define BUG_FRAME(type, line, file, has_msg, msg) do { \
+#define BUG_FRAME(type, line, ptr, 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" \
+ ".pushsection .bug_frames." __stringify(type) ", \"a\", %%progbits\n"\
+ "2:\n" \
".p2align 2\n" \
- ".long (1b - 4b)\n" \
- ".long (2b - 4b)\n" \
- ".long (3b - 4b)\n" \
+ ".long (1b - 2b)\n" \
+ ".long (%0 - 2b)\n" \
+ ".long (%1 - 2b)\n" \
".hword " __stringify(line) ", 0\n" \
- ".popsection"); \
+ ".popsection" :: "i" (ptr), "i" (msg)); \
} while (0)
-#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")
+#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, "")
+
+#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, "")
#define BUG() do { \
- BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, ""); \
+ BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \
unreachable(); \
} while (0)
#define assert_failed(msg) do { \
- BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
+ BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, msg); \
unreachable(); \
} while (0)
extern const struct bug_frame __start_bug_frames[],
__stop_bug_frames_0[],
__stop_bug_frames_1[],
- __stop_bug_frames_2[];
+ __stop_bug_frames_2[],
+ __stop_bug_frames_3[];
#endif /* __ARM_BUG_H__ */
/*
--
2.26.2
On 15.12.2020 07:33, Juergen Gross wrote:
> --- a/xen/include/asm-arm/bug.h
> +++ b/xen/include/asm-arm/bug.h
> @@ -15,65 +15,62 @@
>
> struct bug_frame {
> signed int loc_disp; /* Relative address to the bug address */
> - signed int file_disp; /* Relative address to the filename */
> + signed int ptr_disp; /* Relative address to the filename or function */
> 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_ptr(b) ((const void *)(b) + (b)->ptr_disp);
> #define bug_line(b) ((b)->line)
> #define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
>
> -#define BUGFRAME_warn 0
> -#define BUGFRAME_bug 1
> -#define BUGFRAME_assert 2
> +#define BUGFRAME_run_fn 0
> +#define BUGFRAME_warn 1
> +#define BUGFRAME_bug 2
> +#define BUGFRAME_assert 3
>
> -#define BUGFRAME_NR 3
> +#define BUGFRAME_NR 4
>
> /* 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.
> */
> -#define BUG_FRAME(type, line, file, has_msg, msg) do { \
> +#define BUG_FRAME(type, line, ptr, 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" \
> + ".pushsection .bug_frames." __stringify(type) ", \"a\", %%progbits\n"\
> + "2:\n" \
> ".p2align 2\n" \
> - ".long (1b - 4b)\n" \
> - ".long (2b - 4b)\n" \
> - ".long (3b - 4b)\n" \
> + ".long (1b - 2b)\n" \
> + ".long (%0 - 2b)\n" \
> + ".long (%1 - 2b)\n" \
> ".hword " __stringify(line) ", 0\n" \
> - ".popsection"); \
> + ".popsection" :: "i" (ptr), "i" (msg)); \
> } while (0)
The comment ahead of the construct now looks to be at best stale, if
not entirely pointless. The reference to %c looks quite strange here
to me anyway - I can only guess it appeared here because on x86 one
has to use %c to output constants as operands for .long and alike,
and this was then tried to use on Arm as well without there really
being a need.
Jan
Hi Jan,
On 15/12/2020 09:02, Jan Beulich wrote:
> On 15.12.2020 07:33, Juergen Gross wrote:
>> --- a/xen/include/asm-arm/bug.h
>> +++ b/xen/include/asm-arm/bug.h
>> @@ -15,65 +15,62 @@
>>
>> struct bug_frame {
>> signed int loc_disp; /* Relative address to the bug address */
>> - signed int file_disp; /* Relative address to the filename */
>> + signed int ptr_disp; /* Relative address to the filename or function */
>> 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_ptr(b) ((const void *)(b) + (b)->ptr_disp);
>> #define bug_line(b) ((b)->line)
>> #define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
>>
>> -#define BUGFRAME_warn 0
>> -#define BUGFRAME_bug 1
>> -#define BUGFRAME_assert 2
>> +#define BUGFRAME_run_fn 0
>> +#define BUGFRAME_warn 1
>> +#define BUGFRAME_bug 2
>> +#define BUGFRAME_assert 3
>>
>> -#define BUGFRAME_NR 3
>> +#define BUGFRAME_NR 4
>>
>> /* 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.
>> */
>> -#define BUG_FRAME(type, line, file, has_msg, msg) do { \
>> +#define BUG_FRAME(type, line, ptr, 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" \
>> + ".pushsection .bug_frames." __stringify(type) ", \"a\", %%progbits\n"\
>> + "2:\n" \
>> ".p2align 2\n" \
>> - ".long (1b - 4b)\n" \
>> - ".long (2b - 4b)\n" \
>> - ".long (3b - 4b)\n" \
>> + ".long (1b - 2b)\n" \
>> + ".long (%0 - 2b)\n" \
>> + ".long (%1 - 2b)\n" \
>> ".hword " __stringify(line) ", 0\n" \
>> - ".popsection"); \
>> + ".popsection" :: "i" (ptr), "i" (msg)); \
>> } while (0)
>
> The comment ahead of the construct now looks to be at best stale, if
> not entirely pointless. The reference to %c looks quite strange here
> to me anyway - I can only guess it appeared here because on x86 one
> has to use %c to output constants as operands for .long and alike,
> and this was then tried to use on Arm as well without there really
> being a need.
Well, %c is one the reason why we can't have a common BUG_FRAME
implementation. So I would like to retain this information before
someone wants to consolidate the code and missed this issue.
Regarding the mergeable string section, I agree that the comment in now
stale. However, could someone confirm that "i" will still retain the
same behavior as using mergeable string sections?
Cheers,
--
Julien Grall
On 15.12.2020 14:39, Julien Grall wrote:
> On 15/12/2020 09:02, Jan Beulich wrote:
>> On 15.12.2020 07:33, Juergen Gross wrote:
>>> --- a/xen/include/asm-arm/bug.h
>>> +++ b/xen/include/asm-arm/bug.h
>>> @@ -15,65 +15,62 @@
>>>
>>> struct bug_frame {
>>> signed int loc_disp; /* Relative address to the bug address */
>>> - signed int file_disp; /* Relative address to the filename */
>>> + signed int ptr_disp; /* Relative address to the filename or function */
>>> 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_ptr(b) ((const void *)(b) + (b)->ptr_disp);
>>> #define bug_line(b) ((b)->line)
>>> #define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
>>>
>>> -#define BUGFRAME_warn 0
>>> -#define BUGFRAME_bug 1
>>> -#define BUGFRAME_assert 2
>>> +#define BUGFRAME_run_fn 0
>>> +#define BUGFRAME_warn 1
>>> +#define BUGFRAME_bug 2
>>> +#define BUGFRAME_assert 3
>>>
>>> -#define BUGFRAME_NR 3
>>> +#define BUGFRAME_NR 4
>>>
>>> /* 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.
>>> */
>>> -#define BUG_FRAME(type, line, file, has_msg, msg) do { \
>>> +#define BUG_FRAME(type, line, ptr, 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" \
>>> + ".pushsection .bug_frames." __stringify(type) ", \"a\", %%progbits\n"\
>>> + "2:\n" \
>>> ".p2align 2\n" \
>>> - ".long (1b - 4b)\n" \
>>> - ".long (2b - 4b)\n" \
>>> - ".long (3b - 4b)\n" \
>>> + ".long (1b - 2b)\n" \
>>> + ".long (%0 - 2b)\n" \
>>> + ".long (%1 - 2b)\n" \
>>> ".hword " __stringify(line) ", 0\n" \
>>> - ".popsection"); \
>>> + ".popsection" :: "i" (ptr), "i" (msg)); \
>>> } while (0)
>>
>> The comment ahead of the construct now looks to be at best stale, if
>> not entirely pointless. The reference to %c looks quite strange here
>> to me anyway - I can only guess it appeared here because on x86 one
>> has to use %c to output constants as operands for .long and alike,
>> and this was then tried to use on Arm as well without there really
>> being a need.
>
> Well, %c is one the reason why we can't have a common BUG_FRAME
> implementation. So I would like to retain this information before
> someone wants to consolidate the code and missed this issue.
Fair enough, albeit I guess this then could do with re-wording.
> Regarding the mergeable string section, I agree that the comment in now
> stale. However, could someone confirm that "i" will still retain the
> same behavior as using mergeable string sections?
That's depend on compiler settings / behavior.
Jan
Hi Jan,
On 15/12/2020 13:59, Jan Beulich wrote:
> On 15.12.2020 14:39, Julien Grall wrote:
>> On 15/12/2020 09:02, Jan Beulich wrote:
>>> On 15.12.2020 07:33, Juergen Gross wrote:
>>>> --- a/xen/include/asm-arm/bug.h
>>>> +++ b/xen/include/asm-arm/bug.h
>>>> @@ -15,65 +15,62 @@
>>>>
>>>> struct bug_frame {
>>>> signed int loc_disp; /* Relative address to the bug address */
>>>> - signed int file_disp; /* Relative address to the filename */
>>>> + signed int ptr_disp; /* Relative address to the filename or function */
>>>> 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_ptr(b) ((const void *)(b) + (b)->ptr_disp);
>>>> #define bug_line(b) ((b)->line)
>>>> #define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
>>>>
>>>> -#define BUGFRAME_warn 0
>>>> -#define BUGFRAME_bug 1
>>>> -#define BUGFRAME_assert 2
>>>> +#define BUGFRAME_run_fn 0
>>>> +#define BUGFRAME_warn 1
>>>> +#define BUGFRAME_bug 2
>>>> +#define BUGFRAME_assert 3
>>>>
>>>> -#define BUGFRAME_NR 3
>>>> +#define BUGFRAME_NR 4
>>>>
>>>> /* 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.
>>>> */
>>>> -#define BUG_FRAME(type, line, file, has_msg, msg) do { \
>>>> +#define BUG_FRAME(type, line, ptr, 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" \
>>>> + ".pushsection .bug_frames." __stringify(type) ", \"a\", %%progbits\n"\
>>>> + "2:\n" \
>>>> ".p2align 2\n" \
>>>> - ".long (1b - 4b)\n" \
>>>> - ".long (2b - 4b)\n" \
>>>> - ".long (3b - 4b)\n" \
>>>> + ".long (1b - 2b)\n" \
>>>> + ".long (%0 - 2b)\n" \
>>>> + ".long (%1 - 2b)\n" \
>>>> ".hword " __stringify(line) ", 0\n" \
>>>> - ".popsection"); \
>>>> + ".popsection" :: "i" (ptr), "i" (msg)); \
>>>> } while (0)
>>>
>>> The comment ahead of the construct now looks to be at best stale, if
>>> not entirely pointless. The reference to %c looks quite strange here
>>> to me anyway - I can only guess it appeared here because on x86 one
>>> has to use %c to output constants as operands for .long and alike,
>>> and this was then tried to use on Arm as well without there really
>>> being a need.
>>
>> Well, %c is one the reason why we can't have a common BUG_FRAME
>> implementation. So I would like to retain this information before
>> someone wants to consolidate the code and missed this issue.
>
> Fair enough, albeit I guess this then could do with re-wording.
I agree.
>
>> Regarding the mergeable string section, I agree that the comment in now
>> stale. However, could someone confirm that "i" will still retain the
>> same behavior as using mergeable string sections?
>
> That's depend on compiler settings / behavior.
Ok. I wanted to see the difference between before and after but it looks
like I can't compile Xen after applying the patch:
In file included from
/home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:23:0,
from
/home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/bitmap.h:6,
from bitmap.c:10:
bitmap.c: In function ‘bitmap_allocate_region’:
/home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:44:5:
error: asm operand 0 probably doesn’t match constraints [-Werror]
asm ("1:"BUG_INSTR"\n"
\
^
/home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:60:5:
note: in expansion of macro ‘BUG_FRAME’
BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \
^~~~~~~~~
/home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:25:42:
note: in expansion of macro ‘BUG’
#define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
^~~
bitmap.c:330:2: note: in expansion of macro ‘BUG_ON’
BUG_ON(pages > BITS_PER_LONG);
^~~~~~
/home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:44:5:
error: asm operand 1 probably doesn’t match constraints [-Werror]
asm ("1:"BUG_INSTR"\n"
\
^
/home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:60:5:
note: in expansion of macro ‘BUG_FRAME’
BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \
^~~~~~~~~
/home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:25:42:
note: in expansion of macro ‘BUG’
#define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
^~~
bitmap.c:330:2: note: in expansion of macro ‘BUG_ON’
BUG_ON(pages > BITS_PER_LONG);
^~~~~~
/home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:44:5:
error: impossible constraint in ‘asm’
asm ("1:"BUG_INSTR"\n"
\
^
/home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:60:5:
note: in expansion of macro ‘BUG_FRAME’
BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \
^~~~~~~~~
/home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:25:42:
note: in expansion of macro ‘BUG’
#define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
^~~
bitmap.c:330:2: note: in expansion of macro ‘BUG_ON’
BUG_ON(pages > BITS_PER_LONG);
^~~~~~
cc1: all warnings being treated as errors
I am using GCC 7.5.0 built by Linaro (cross-compiler). Native
compilation with GCC 10.2.1 leads to the same error.
@Juergen, which compiler did you use?
--
Julien Grall
On 21.12.20 17:50, Julien Grall wrote:
> Hi Jan,
>
> On 15/12/2020 13:59, Jan Beulich wrote:
>> On 15.12.2020 14:39, Julien Grall wrote:
>>> On 15/12/2020 09:02, Jan Beulich wrote:
>>>> On 15.12.2020 07:33, Juergen Gross wrote:
>>>>> --- a/xen/include/asm-arm/bug.h
>>>>> +++ b/xen/include/asm-arm/bug.h
>>>>> @@ -15,65 +15,62 @@
>>>>> struct bug_frame {
>>>>> signed int loc_disp; /* Relative address to the bug
>>>>> address */
>>>>> - signed int file_disp; /* Relative address to the filename */
>>>>> + signed int ptr_disp; /* Relative address to the filename or
>>>>> function */
>>>>> 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_ptr(b) ((const void *)(b) + (b)->ptr_disp);
>>>>> #define bug_line(b) ((b)->line)
>>>>> #define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
>>>>> -#define BUGFRAME_warn 0
>>>>> -#define BUGFRAME_bug 1
>>>>> -#define BUGFRAME_assert 2
>>>>> +#define BUGFRAME_run_fn 0
>>>>> +#define BUGFRAME_warn 1
>>>>> +#define BUGFRAME_bug 2
>>>>> +#define BUGFRAME_assert 3
>>>>> -#define BUGFRAME_NR 3
>>>>> +#define BUGFRAME_NR 4
>>>>> /* 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.
>>>>> */
>>>>> -#define BUG_FRAME(type, line, file, has_msg, msg) do
>>>>> { \
>>>>> +#define BUG_FRAME(type, line, ptr, 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" \
>>>>> + ".pushsection .bug_frames." __stringify(type) ", \"a\",
>>>>> %%progbits\n"\
>>>>> +
>>>>> "2:\n" \
>>>>> ".p2align
>>>>> 2\n" \
>>>>> - ".long (1b -
>>>>> 4b)\n" \
>>>>> - ".long (2b -
>>>>> 4b)\n" \
>>>>> - ".long (3b -
>>>>> 4b)\n" \
>>>>> + ".long (1b -
>>>>> 2b)\n" \
>>>>> + ".long (%0 -
>>>>> 2b)\n" \
>>>>> + ".long (%1 -
>>>>> 2b)\n" \
>>>>> ".hword " __stringify(line) ",
>>>>> 0\n" \
>>>>> -
>>>>> ".popsection"); \
>>>>> + ".popsection" :: "i" (ptr), "i"
>>>>> (msg)); \
>>>>> } while (0)
>>>>
>>>> The comment ahead of the construct now looks to be at best stale, if
>>>> not entirely pointless. The reference to %c looks quite strange here
>>>> to me anyway - I can only guess it appeared here because on x86 one
>>>> has to use %c to output constants as operands for .long and alike,
>>>> and this was then tried to use on Arm as well without there really
>>>> being a need.
>>>
>>> Well, %c is one the reason why we can't have a common BUG_FRAME
>>> implementation. So I would like to retain this information before
>>> someone wants to consolidate the code and missed this issue.
>>
>> Fair enough, albeit I guess this then could do with re-wording.
>
> I agree.
>
>>
>>> Regarding the mergeable string section, I agree that the comment in now
>>> stale. However, could someone confirm that "i" will still retain the
>>> same behavior as using mergeable string sections?
>>
>> That's depend on compiler settings / behavior.
>
> Ok. I wanted to see the difference between before and after but it looks
> like I can't compile Xen after applying the patch:
>
>
> In file included from
> /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:23:0,
> from
> /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/bitmap.h:6,
> from bitmap.c:10:
> bitmap.c: In function ‘bitmap_allocate_region’:
> /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:44:5:
> error: asm operand 0 probably doesn’t match constraints [-Werror]
> asm ("1:"BUG_INSTR"\n" \
> ^
> /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:60:5:
> note: in expansion of macro ‘BUG_FRAME’
> BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \
> ^~~~~~~~~
> /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:25:42:
> note: in expansion of macro ‘BUG’
> #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
> ^~~
> bitmap.c:330:2: note: in expansion of macro ‘BUG_ON’
> BUG_ON(pages > BITS_PER_LONG);
> ^~~~~~
> /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:44:5:
> error: asm operand 1 probably doesn’t match constraints [-Werror]
> asm ("1:"BUG_INSTR"\n" \
> ^
> /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:60:5:
> note: in expansion of macro ‘BUG_FRAME’
> BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \
> ^~~~~~~~~
> /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:25:42:
> note: in expansion of macro ‘BUG’
> #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
> ^~~
> bitmap.c:330:2: note: in expansion of macro ‘BUG_ON’
> BUG_ON(pages > BITS_PER_LONG);
> ^~~~~~
> /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:44:5:
> error: impossible constraint in ‘asm’
> asm ("1:"BUG_INSTR"\n" \
> ^
> /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:60:5:
> note: in expansion of macro ‘BUG_FRAME’
> BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \
> ^~~~~~~~~
> /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:25:42:
> note: in expansion of macro ‘BUG’
> #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
> ^~~
> bitmap.c:330:2: note: in expansion of macro ‘BUG_ON’
> BUG_ON(pages > BITS_PER_LONG);
> ^~~~~~
> cc1: all warnings being treated as errors
>
> I am using GCC 7.5.0 built by Linaro (cross-compiler). Native
> compilation with GCC 10.2.1 leads to the same error.
>
> @Juergen, which compiler did you use?
>
gcc 7.4.0 aarch64 cross-compiler (SUSE)
Juergen
On 15.12.20 10:02, Jan Beulich wrote:
> On 15.12.2020 07:33, Juergen Gross wrote:
>> --- a/xen/include/asm-arm/bug.h
>> +++ b/xen/include/asm-arm/bug.h
>> @@ -15,65 +15,62 @@
>>
>> struct bug_frame {
>> signed int loc_disp; /* Relative address to the bug address */
>> - signed int file_disp; /* Relative address to the filename */
>> + signed int ptr_disp; /* Relative address to the filename or function */
>> 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_ptr(b) ((const void *)(b) + (b)->ptr_disp);
>> #define bug_line(b) ((b)->line)
>> #define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
>>
>> -#define BUGFRAME_warn 0
>> -#define BUGFRAME_bug 1
>> -#define BUGFRAME_assert 2
>> +#define BUGFRAME_run_fn 0
>> +#define BUGFRAME_warn 1
>> +#define BUGFRAME_bug 2
>> +#define BUGFRAME_assert 3
>>
>> -#define BUGFRAME_NR 3
>> +#define BUGFRAME_NR 4
>>
>> /* 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.
>> */
>> -#define BUG_FRAME(type, line, file, has_msg, msg) do { \
>> +#define BUG_FRAME(type, line, ptr, 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" \
>> + ".pushsection .bug_frames." __stringify(type) ", \"a\", %%progbits\n"\
>> + "2:\n" \
>> ".p2align 2\n" \
>> - ".long (1b - 4b)\n" \
>> - ".long (2b - 4b)\n" \
>> - ".long (3b - 4b)\n" \
>> + ".long (1b - 2b)\n" \
>> + ".long (%0 - 2b)\n" \
>> + ".long (%1 - 2b)\n" \
>> ".hword " __stringify(line) ", 0\n" \
>> - ".popsection"); \
>> + ".popsection" :: "i" (ptr), "i" (msg)); \
>> } while (0)
>
> The comment ahead of the construct now looks to be at best stale, if
> not entirely pointless. The reference to %c looks quite strange here
> to me anyway - I can only guess it appeared here because on x86 one
> has to use %c to output constants as operands for .long and alike,
> and this was then tried to use on Arm as well without there really
> being a need.
Probably so.
I can remove the comment, but would like an Arm maintainer to confirm.
Juergen
© 2016 - 2026 Red Hat, Inc.