Tim Deegan
2013-Aug-29 15:55 UTC
[PATCH] xen/x86: don''t use ''.ifnes'' in bug frame construction.
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
Keir Fraser
2013-Aug-29 17:50 UTC
Re: [PATCH] xen/x86: don''t use ''.ifnes'' in bug frame construction.
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[],
Jan Beulich
2013-Aug-30 08:29 UTC
Re: [PATCH] xen/x86: don''t use ''.ifnes'' in bug frame construction.
>>> 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
Andrew Cooper
2013-Aug-30 08:35 UTC
Re: [PATCH] xen/x86: don''t use ''.ifnes'' in bug frame construction.
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. > > JanMy 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
Tim Deegan
2013-Aug-30 09:43 UTC
Re: [PATCH] xen/x86: don''t use ''.ifnes'' in bug frame construction.
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.
Jan Beulich
2013-Aug-30 09:58 UTC
Re: [PATCH] xen/x86: don''t use ''.ifnes'' in bug frame construction.
>>> 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