[PATCH] xen/x86: don't use '.ifnes' in bug frame construction.

[PATCH] xen/x86: don't use '.ifnes' in bug frame construction.

[Tim Deegan]

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

Re: [PATCH] xen/x86: don't use '.ifnes' in bug frame construction.

[Keir Fraser]

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[],

Re: [PATCH] xen/x86: don't use '.ifnes' in bug frame construction.

[Jan Beulich]

>>> 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

Re: [PATCH] xen/x86: don't use '.ifnes' in bug frame construction.

[Andrew Cooper]

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

Re: [PATCH] xen/x86: don't use '.ifnes' in bug frame construction.

[Tim Deegan]

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.

Re: [PATCH] xen/x86: don't use '.ifnes' in bug frame construction.

[Jan Beulich]

>>> 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