* [PATCH] xen/x86: don't use '.ifnes' in bug frame construction.
@ 2013-08-29 15:55 Tim Deegan
2013-08-29 17:50 ` Keir Fraser
2013-08-30 8:29 ` Jan Beulich
0 siblings, 2 replies; 6+ messages in thread
From: Tim Deegan @ 2013-08-29 15:55 UTC (permalink / raw)
To: xen-devel; +Cc: keir, jbeulich
Spotted because it breaks the clang build for LLVM <3.2. .ifnes is
not right here as it will choke on a string with embedded quotes.
.ifnb would be better except that LLVM <3.2 doesn't support that either.
It should be possible to use something like !!msg or !!msg[0] instead
of a separate flag, but I gave up trying to find something that would
make it through CPP, asm() and gas as a usable constant. :|
Signed-off-by: Tim Deegan <tim@xen.org>
---
xen/include/asm-x86/bug.h | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h
index 148975f..e5dd559 100644
--- a/xen/include/asm-x86/bug.h
+++ b/xen/include/asm-x86/bug.h
@@ -25,7 +25,7 @@ struct bug_frame {
#define BUGFRAME_bug 2
#define BUGFRAME_assert 3
-#define BUG_FRAME(type, line, ptr, msg) do { \
+#define BUG_FRAME(type, line, ptr, second_frame, msg) do { \
BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)); \
asm volatile ( ".Lbug%=: ud2\n" \
".pushsection .bug_frames.%c0, \"a\", @progbits\n" \
@@ -33,7 +33,7 @@ struct bug_frame {
".Lfrm%=:\n" \
".long (.Lbug%= - .Lfrm%=) + %c4\n" \
".long (%c1 - .Lfrm%=) + %c3\n" \
- ".ifnes \"" msg "\", \"\"\n" \
+ ".if " #second_frame "\n" \
".long 0, %c2 - .Lfrm%=\n" \
".endif\n" \
".popsection" \
@@ -44,12 +44,14 @@ struct bug_frame {
"i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)); \
} while (0)
-#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, "")
-#define BUG() BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, "")
-#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, "")
+#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
+#define BUG() BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL)
-#define assert_failed(msg) BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, msg)
+#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL)
+
+#define assert_failed(msg) \
+ BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg)
extern const struct bug_frame __start_bug_frames[],
__stop_bug_frames_0[],
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/x86: don't use '.ifnes' in bug frame construction.
2013-08-29 15:55 [PATCH] xen/x86: don't use '.ifnes' in bug frame construction Tim Deegan
@ 2013-08-29 17:50 ` Keir Fraser
2013-08-30 8:29 ` Jan Beulich
1 sibling, 0 replies; 6+ messages in thread
From: Keir Fraser @ 2013-08-29 17:50 UTC (permalink / raw)
To: Tim Deegan, xen-devel; +Cc: jbeulich
On 29/08/2013 16:55, "Tim Deegan" <tim@xen.org> wrote:
> Spotted because it breaks the clang build for LLVM <3.2. .ifnes is
> not right here as it will choke on a string with embedded quotes.
>
> .ifnb would be better except that LLVM <3.2 doesn't support that either.
> It should be possible to use something like !!msg or !!msg[0] instead
> of a separate flag, but I gave up trying to find something that would
> make it through CPP, asm() and gas as a usable constant. :|
>
> Signed-off-by: Tim Deegan <tim@xen.org>
I'll leave it to Jan to Ack/apply this one.
-- Keir
> ---
> xen/include/asm-x86/bug.h | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h
> index 148975f..e5dd559 100644
> --- a/xen/include/asm-x86/bug.h
> +++ b/xen/include/asm-x86/bug.h
> @@ -25,7 +25,7 @@ struct bug_frame {
> #define BUGFRAME_bug 2
> #define BUGFRAME_assert 3
>
> -#define BUG_FRAME(type, line, ptr, msg) do {
> \
> +#define BUG_FRAME(type, line, ptr, second_frame, msg) do {
> \
> BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH));
> \
> asm volatile ( ".Lbug%=: ud2\n"
> \
> ".pushsection .bug_frames.%c0, \"a\", @progbits\n"
> \
> @@ -33,7 +33,7 @@ struct bug_frame {
> ".Lfrm%=:\n"
> \
> ".long (.Lbug%= - .Lfrm%=) + %c4\n"
> \
> ".long (%c1 - .Lfrm%=) + %c3\n"
> \
> - ".ifnes \"" msg "\", \"\"\n"
> \
> + ".if " #second_frame "\n"
> \
> ".long 0, %c2 - .Lfrm%=\n"
> \
> ".endif\n"
> \
> ".popsection"
> \
> @@ -44,12 +44,14 @@ struct bug_frame {
> "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH));
> \
> } while (0)
>
> -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, "")
> -#define BUG() BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, "")
>
> -#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, "")
> +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
> +#define BUG() BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL)
>
> -#define assert_failed(msg) BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__,
> msg)
> +#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0,
> NULL)
> +
> +#define assert_failed(msg) \
> + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg)
>
> extern const struct bug_frame __start_bug_frames[],
> __stop_bug_frames_0[],
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/x86: don't use '.ifnes' in bug frame construction.
2013-08-29 15:55 [PATCH] xen/x86: don't use '.ifnes' in bug frame construction Tim Deegan
2013-08-29 17:50 ` Keir Fraser
@ 2013-08-30 8:29 ` Jan Beulich
2013-08-30 8:35 ` Andrew Cooper
2013-08-30 9:43 ` Tim Deegan
1 sibling, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2013-08-30 8:29 UTC (permalink / raw)
To: Tim Deegan; +Cc: xen-devel, keir
>>> On 29.08.13 at 17:55, Tim Deegan <tim@xen.org> wrote:
> Spotted because it breaks the clang build for LLVM <3.2. .ifnes is
> not right here as it will choke on a string with embedded quotes.
>
> .ifnb would be better except that LLVM <3.2 doesn't support that either.
> It should be possible to use something like !!msg or !!msg[0] instead
> of a separate flag, but I gave up trying to find something that would
> make it through CPP, asm() and gas as a usable constant. :|
I'm not really opposed to this (albeit I dislike redundancy like this),
but I'd really like to understand (also for the sake of my own
education, including to avoid introducing similar breakage in the
future) where the problem was, and how gcc+gas manage to
accept what you appear to have found broken in general (i.e. not
just for clang as I understand it).
And yes, I would have preferred (and used) .ifnb if I hadn't recalled
it having been me adding the support for it for binutils 2.17, while at
least one of the so far lowest common denominator distros (SLE10)
is only at 2.16.<something> (which appears to have the needed
support, but it would seem wrong to draw the line at some
intermediary release); not sure what binutils version RHEL5 uses.
Jan
> Signed-off-by: Tim Deegan <tim@xen.org>
> ---
> xen/include/asm-x86/bug.h | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h
> index 148975f..e5dd559 100644
> --- a/xen/include/asm-x86/bug.h
> +++ b/xen/include/asm-x86/bug.h
> @@ -25,7 +25,7 @@ struct bug_frame {
> #define BUGFRAME_bug 2
> #define BUGFRAME_assert 3
>
> -#define BUG_FRAME(type, line, ptr, msg) do {
> \
> +#define BUG_FRAME(type, line, ptr, second_frame, msg) do {
> \
> BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH));
> \
> asm volatile ( ".Lbug%=: ud2\n"
> \
> ".pushsection .bug_frames.%c0, \"a\", @progbits\n"
> \
> @@ -33,7 +33,7 @@ struct bug_frame {
> ".Lfrm%=:\n"
> \
> ".long (.Lbug%= - .Lfrm%=) + %c4\n"
> \
> ".long (%c1 - .Lfrm%=) + %c3\n"
> \
> - ".ifnes \"" msg "\", \"\"\n"
> \
> + ".if " #second_frame "\n"
> \
> ".long 0, %c2 - .Lfrm%=\n"
> \
> ".endif\n"
> \
> ".popsection"
> \
> @@ -44,12 +44,14 @@ struct bug_frame {
> "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)); \
> } while (0)
>
> -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, "")
> -#define BUG() BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, "")
>
> -#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, "")
> +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
> +#define BUG() BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL)
>
> -#define assert_failed(msg) BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__,
> msg)
> +#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0,
> NULL)
> +
> +#define assert_failed(msg) \
> + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg)
>
> extern const struct bug_frame __start_bug_frames[],
> __stop_bug_frames_0[],
> --
> 1.7.10.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/x86: don't use '.ifnes' in bug frame construction.
2013-08-30 8:29 ` Jan Beulich
@ 2013-08-30 8:35 ` Andrew Cooper
2013-08-30 9:43 ` Tim Deegan
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2013-08-30 8:35 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, keir, Tim Deegan
On 30/08/13 09:29, Jan Beulich wrote:
>>>> On 29.08.13 at 17:55, Tim Deegan <tim@xen.org> wrote:
>> Spotted because it breaks the clang build for LLVM <3.2. .ifnes is
>> not right here as it will choke on a string with embedded quotes.
>>
>> .ifnb would be better except that LLVM <3.2 doesn't support that either.
>> It should be possible to use something like !!msg or !!msg[0] instead
>> of a separate flag, but I gave up trying to find something that would
>> make it through CPP, asm() and gas as a usable constant. :|
> I'm not really opposed to this (albeit I dislike redundancy like this),
> but I'd really like to understand (also for the sake of my own
> education, including to avoid introducing similar breakage in the
> future) where the problem was, and how gcc+gas manage to
> accept what you appear to have found broken in general (i.e. not
> just for clang as I understand it).
>
> And yes, I would have preferred (and used) .ifnb if I hadn't recalled
> it having been me adding the support for it for binutils 2.17, while at
> least one of the so far lowest common denominator distros (SLE10)
> is only at 2.16.<something> (which appears to have the needed
> support, but it would seem wrong to draw the line at some
> intermediary release); not sure what binutils version RHEL5 uses.
>
> Jan
My CentOS 5.7 environment claims binutils 2.17.50
~Andrew
>
>> Signed-off-by: Tim Deegan <tim@xen.org>
>> ---
>> xen/include/asm-x86/bug.h | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h
>> index 148975f..e5dd559 100644
>> --- a/xen/include/asm-x86/bug.h
>> +++ b/xen/include/asm-x86/bug.h
>> @@ -25,7 +25,7 @@ struct bug_frame {
>> #define BUGFRAME_bug 2
>> #define BUGFRAME_assert 3
>>
>> -#define BUG_FRAME(type, line, ptr, msg) do {
>> \
>> +#define BUG_FRAME(type, line, ptr, second_frame, msg) do {
>> \
>> BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH));
>> \
>> asm volatile ( ".Lbug%=: ud2\n"
>> \
>> ".pushsection .bug_frames.%c0, \"a\", @progbits\n"
>> \
>> @@ -33,7 +33,7 @@ struct bug_frame {
>> ".Lfrm%=:\n"
>> \
>> ".long (.Lbug%= - .Lfrm%=) + %c4\n"
>> \
>> ".long (%c1 - .Lfrm%=) + %c3\n"
>> \
>> - ".ifnes \"" msg "\", \"\"\n"
>> \
>> + ".if " #second_frame "\n"
>> \
>> ".long 0, %c2 - .Lfrm%=\n"
>> \
>> ".endif\n"
>> \
>> ".popsection"
>> \
>> @@ -44,12 +44,14 @@ struct bug_frame {
>> "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)); \
>> } while (0)
>>
>> -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, "")
>> -#define BUG() BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, "")
>>
>> -#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, "")
>> +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
>> +#define BUG() BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL)
>>
>> -#define assert_failed(msg) BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__,
>> msg)
>> +#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0,
>> NULL)
>> +
>> +#define assert_failed(msg) \
>> + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg)
>>
>> extern const struct bug_frame __start_bug_frames[],
>> __stop_bug_frames_0[],
>> --
>> 1.7.10.4
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/x86: don't use '.ifnes' in bug frame construction.
2013-08-30 8:29 ` Jan Beulich
2013-08-30 8:35 ` Andrew Cooper
@ 2013-08-30 9:43 ` Tim Deegan
2013-08-30 9:58 ` Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Tim Deegan @ 2013-08-30 9:43 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, keir
At 09:29 +0100 on 30 Aug (1377854957), Jan Beulich wrote:
> >>> On 29.08.13 at 17:55, Tim Deegan <tim@xen.org> wrote:
> > Spotted because it breaks the clang build for LLVM <3.2. .ifnes is
> > not right here as it will choke on a string with embedded quotes.
> >
> > .ifnb would be better except that LLVM <3.2 doesn't support that either.
> > It should be possible to use something like !!msg or !!msg[0] instead
> > of a separate flag, but I gave up trying to find something that would
> > make it through CPP, asm() and gas as a usable constant. :|
>
> I'm not really opposed to this (albeit I dislike redundancy like this),
> but I'd really like to understand (also for the sake of my own
> education, including to avoid introducing similar breakage in the
> future) where the problem was, and how gcc+gas manage to
> accept what you appear to have found broken in general (i.e. not
> just for clang as I understand it).
The clang failure is just that the fancier .if operations are not
supported until LLVM 3.2 (and Xen builds file with 3.0 otherwise).
The problem with [.ifnes \"" msg "\" \"\"] is that .ifeqs and .ifnes
expect "-delimited strings and (at least in my testing yesterday) will
fail when msg has a quote in it. That doesn't happen in the current
Xen tree but something like ASSERT(strlen("boo") == 3) or the
ASSERT(predicate() && "explanatory message") idiom would fail.
> And yes, I would have preferred (and used) .ifnb if I hadn't recalled
> it having been me adding the support for it for binutils 2.17, while at
> least one of the so far lowest common denominator distros (SLE10)
> is only at 2.16.<something> (which appears to have the needed
> support, but it would seem wrong to draw the line at some
> intermediary release); not sure what binutils version RHEL5 uses.
Righto. Can I have an Ack/Nack please, since Keir defers to you and
I've run out of x86 maintainers? :)
Tim.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/x86: don't use '.ifnes' in bug frame construction.
2013-08-30 9:43 ` Tim Deegan
@ 2013-08-30 9:58 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2013-08-30 9:58 UTC (permalink / raw)
To: Tim Deegan; +Cc: xen-devel, keir
>>> On 30.08.13 at 11:43, Tim Deegan <tim@xen.org> wrote:
> At 09:29 +0100 on 30 Aug (1377854957), Jan Beulich wrote:
>> >>> On 29.08.13 at 17:55, Tim Deegan <tim@xen.org> wrote:
>> > Spotted because it breaks the clang build for LLVM <3.2. .ifnes is
>> > not right here as it will choke on a string with embedded quotes.
>> >
>> > .ifnb would be better except that LLVM <3.2 doesn't support that either.
>> > It should be possible to use something like !!msg or !!msg[0] instead
>> > of a separate flag, but I gave up trying to find something that would
>> > make it through CPP, asm() and gas as a usable constant. :|
>>
>> I'm not really opposed to this (albeit I dislike redundancy like this),
>> but I'd really like to understand (also for the sake of my own
>> education, including to avoid introducing similar breakage in the
>> future) where the problem was, and how gcc+gas manage to
>> accept what you appear to have found broken in general (i.e. not
>> just for clang as I understand it).
>
> The clang failure is just that the fancier .if operations are not
> supported until LLVM 3.2 (and Xen builds file with 3.0 otherwise).
>
> The problem with [.ifnes \"" msg "\" \"\"] is that .ifeqs and .ifnes
> expect "-delimited strings and (at least in my testing yesterday) will
> fail when msg has a quote in it. That doesn't happen in the current
> Xen tree but something like ASSERT(strlen("boo") == 3) or the
> ASSERT(predicate() && "explanatory message") idiom would fail.
>
>> And yes, I would have preferred (and used) .ifnb if I hadn't recalled
>> it having been me adding the support for it for binutils 2.17, while at
>> least one of the so far lowest common denominator distros (SLE10)
>> is only at 2.16.<something> (which appears to have the needed
>> support, but it would seem wrong to draw the line at some
>> intermediary release); not sure what binutils version RHEL5 uses.
>
> Righto. Can I have an Ack/Nack please, since Keir defers to you and
> I've run out of x86 maintainers? :)
Sure:
Acked-by: Jan Beulich <jbeulich@suse.com>
As said, before acking I merely wanted to understand why I
didn't see any issue, and what to avoid in the future.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-30 9:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-29 15:55 [PATCH] xen/x86: don't use '.ifnes' in bug frame construction Tim Deegan
2013-08-29 17:50 ` Keir Fraser
2013-08-30 8:29 ` Jan Beulich
2013-08-30 8:35 ` Andrew Cooper
2013-08-30 9:43 ` Tim Deegan
2013-08-30 9:58 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.