[PATCH for-4.9 v2 1/2] xsm: fix clang 3.5 build after c47d1d

[PATCH for-4.9 v2 1/2] xsm: fix clang 3.5 build after c47d1d

[Roger Pau Monne]

The changes introduced on c47d1d broke the clang build due to undefined
references to __xsm_action_mismatch_detected, because clang hasn't optimized
the code properly. The following patch allows the clang build to work again,
while keeping the same functionality.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Tamas K Lengyel <tamas.lengyel@zentific.com>
---
Changes since v1:
 - Remove unused "break".
 - Remove if condition.

NB: this fixes travis build: https://travis-ci.org/royger/xen/builds/219697038
---
 xen/include/xsm/dummy.h | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 56a8814d82..4cb901cb58 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -557,25 +557,21 @@ static XSM_INLINE int xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d)
 
 static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, uint64_t mode, uint32_t op)
 {
-    xsm_default_t a;
     XSM_ASSERT_ACTION(XSM_OTHER);
 
     switch ( mode )
     {
     case XEN_ALTP2M_mixed:
-        a = XSM_TARGET;
-        break;
+        return xsm_default_action(XSM_TARGET, current->domain, d);
     case XEN_ALTP2M_external:
-        a = XSM_DM_PRIV;
-        break;
+        return xsm_default_action(XSM_DM_PRIV, current->domain, d);
     case XEN_ALTP2M_limited:
-        a = (HVMOP_altp2m_vcpu_enable_notify == op) ? XSM_TARGET : XSM_DM_PRIV;
-        break;
+        return xsm_default_action(HVMOP_altp2m_vcpu_enable_notify == op ?
+                                      XSM_TARGET : XSM_DM_PRIV,
+                                  current->domain, d);
     default:
         return -EPERM;
-    };
-
-    return xsm_default_action(a, current->domain, d);
+    }
 }
 
 static XSM_INLINE int xsm_vm_event_control(XSM_DEFAULT_ARG struct domain *d, int mode, int op)
-- 
2.11.0 (Apple Git-81)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH for-4.9 v2 2/2] x86/dm: fix clang build

[Roger Pau Monne]

The current code in dm_op breaks clang build with:

dm.c:411:21: error: 'break' is bound to loop, GCC binds it to switch [-Werror,-Wgcc-compat]
            while ( read_atomic(&p2m->ioreq.entry_count) &&
                    ^
xen/include/asm/atomic.h:53:43: note: expanded from macro 'read_atomic'
    default: x_ = 0; __bad_atomic_size(); break;          \
                                          ^

Move the read_atomic check inside the loop body in order to fix the error.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/hvm/dm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index d72b7bd835..d7aaaf6ff8 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -408,9 +408,16 @@ static int dm_op(domid_t domid,
         {
             struct p2m_domain *p2m = p2m_get_hostp2m(d);
 
-            while ( read_atomic(&p2m->ioreq.entry_count) &&
-                    first_gfn <= p2m->max_mapped_pfn )
+            while ( first_gfn <= p2m->max_mapped_pfn )
             {
+                /*
+                 * NB: read_atomic cannot be used in the loop condition because
+                 * clang complains with: "'break' is bound to loop, GCC binds
+                 * it to switch", so move it inside of the loop instead.
+                 */
+                if ( !read_atomic(&p2m->ioreq.entry_count) )
+                    break;
+
                 /* Iterate p2m table for 256 gfns each time. */
                 p2m_finish_type_change(d, _gfn(first_gfn), 256,
                                        p2m_ioreq_server, p2m_ram_rw);
-- 
2.11.0 (Apple Git-81)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2 1/2] xsm: fix clang 3.5 build after c47d1d

[Jan Beulich]

>>> On 10.04.17 at 11:01, <roger.pau@citrix.com> wrote:
> The changes introduced on c47d1d broke the clang build due to undefined
> references to __xsm_action_mismatch_detected, because clang hasn't optimized
> the code properly. The following patch allows the clang build to work again,
> while keeping the same functionality.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2 2/2] x86/dm: fix clang build

[Jan Beulich]

>>> On 10.04.17 at 11:01, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -408,9 +408,16 @@ static int dm_op(domid_t domid,
>          {
>              struct p2m_domain *p2m = p2m_get_hostp2m(d);
>  
> -            while ( read_atomic(&p2m->ioreq.entry_count) &&
> -                    first_gfn <= p2m->max_mapped_pfn )
> +            while ( first_gfn <= p2m->max_mapped_pfn )
>              {
> +                /*
> +                 * NB: read_atomic cannot be used in the loop condition because
> +                 * clang complains with: "'break' is bound to loop, GCC binds
> +                 * it to switch", so move it inside of the loop instead.
> +                 */
> +                if ( !read_atomic(&p2m->ioreq.entry_count) )
> +                    break;

How is this behavior of clang in line with the language spec, namely
"A break statement terminates execution of the smallest enclosing
switch or iteration statement"?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2 2/2] x86/dm: fix clang build

[Roger Pau Monne]

On Mon, Apr 10, 2017 at 03:27:35AM -0600, Jan Beulich wrote:
> >>> On 10.04.17 at 11:01, <roger.pau@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/dm.c
> > +++ b/xen/arch/x86/hvm/dm.c
> > @@ -408,9 +408,16 @@ static int dm_op(domid_t domid,
> >          {
> >              struct p2m_domain *p2m = p2m_get_hostp2m(d);
> >  
> > -            while ( read_atomic(&p2m->ioreq.entry_count) &&
> > -                    first_gfn <= p2m->max_mapped_pfn )
> > +            while ( first_gfn <= p2m->max_mapped_pfn )
> >              {
> > +                /*
> > +                 * NB: read_atomic cannot be used in the loop condition because
> > +                 * clang complains with: "'break' is bound to loop, GCC binds
> > +                 * it to switch", so move it inside of the loop instead.
> > +                 */
> > +                if ( !read_atomic(&p2m->ioreq.entry_count) )
> > +                    break;
> 
> How is this behavior of clang in line with the language spec, namely
> "A break statement terminates execution of the smallest enclosing
> switch or iteration statement"?

I don't think the condition itself is part of the iteration statement (AFAIK
the condition is the expression, not the statement).

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2 2/2] x86/dm: fix clang build

[Jan Beulich]

>>> On 10.04.17 at 11:46, <roger.pau@citrix.com> wrote:
> On Mon, Apr 10, 2017 at 03:27:35AM -0600, Jan Beulich wrote:
>> >>> On 10.04.17 at 11:01, <roger.pau@citrix.com> wrote:
>> > --- a/xen/arch/x86/hvm/dm.c
>> > +++ b/xen/arch/x86/hvm/dm.c
>> > @@ -408,9 +408,16 @@ static int dm_op(domid_t domid,
>> >          {
>> >              struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> >  
>> > -            while ( read_atomic(&p2m->ioreq.entry_count) &&
>> > -                    first_gfn <= p2m->max_mapped_pfn )
>> > +            while ( first_gfn <= p2m->max_mapped_pfn )
>> >              {
>> > +                /*
>> > +                 * NB: read_atomic cannot be used in the loop condition because
>> > +                 * clang complains with: "'break' is bound to loop, GCC binds
>> > +                 * it to switch", so move it inside of the loop instead.
>> > +                 */
>> > +                if ( !read_atomic(&p2m->ioreq.entry_count) )
>> > +                    break;
>> 
>> How is this behavior of clang in line with the language spec, namely
>> "A break statement terminates execution of the smallest enclosing
>> switch or iteration statement"?
> 
> I don't think the condition itself is part of the iteration statement (AFAIK
> the condition is the expression, not the statement).

I don't understand (and if I take your wording verbally, then you're
giving even more reason for clang being wrong, but I think taking it
verbally would be wrong - "while" itself in my understanding very
much belongs to the iteration statement, as does the condition; the
body of the while statement then is _another_ statement, but not
necessarily an iteration one anymore). Anyway, in

    while ( ({ switch ( x ) { default: break; } }) );

I don't think there's any question what "the smallest enclosing switch
or iteration statement" is.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2 2/2] x86/dm: fix clang build

[Roger Pau Monne]

On Mon, Apr 10, 2017 at 04:02:37AM -0600, Jan Beulich wrote:
> >>> On 10.04.17 at 11:46, <roger.pau@citrix.com> wrote:
> > On Mon, Apr 10, 2017 at 03:27:35AM -0600, Jan Beulich wrote:
> >> >>> On 10.04.17 at 11:01, <roger.pau@citrix.com> wrote:
> >> > --- a/xen/arch/x86/hvm/dm.c
> >> > +++ b/xen/arch/x86/hvm/dm.c
> >> > @@ -408,9 +408,16 @@ static int dm_op(domid_t domid,
> >> >          {
> >> >              struct p2m_domain *p2m = p2m_get_hostp2m(d);
> >> >  
> >> > -            while ( read_atomic(&p2m->ioreq.entry_count) &&
> >> > -                    first_gfn <= p2m->max_mapped_pfn )
> >> > +            while ( first_gfn <= p2m->max_mapped_pfn )
> >> >              {
> >> > +                /*
> >> > +                 * NB: read_atomic cannot be used in the loop condition because
> >> > +                 * clang complains with: "'break' is bound to loop, GCC binds
> >> > +                 * it to switch", so move it inside of the loop instead.
> >> > +                 */
> >> > +                if ( !read_atomic(&p2m->ioreq.entry_count) )
> >> > +                    break;
> >> 
> >> How is this behavior of clang in line with the language spec, namely
> >> "A break statement terminates execution of the smallest enclosing
> >> switch or iteration statement"?
> > 
> > I don't think the condition itself is part of the iteration statement (AFAIK
> > the condition is the expression, not the statement).
> 
> I don't understand (and if I take your wording verbally, then you're
> giving even more reason for clang being wrong, but I think taking it
> verbally would be wrong - "while" itself in my understanding very
> much belongs to the iteration statement, as does the condition; the
> body of the while statement then is _another_ statement, but not
> necessarily an iteration one anymore). Anyway, in
> 
>     while ( ({ switch ( x ) { default: break; } }) );
> 
> I don't think there's any question what "the smallest enclosing switch
> or iteration statement" is.

I think the language might not be clear on this case, the definition of a while
loop statement is:

while ( expression ) statement

And then the definition of break:

A break statement terminates execution of the smallest enclosing switch or
iteration statement.

I agree completely with you that the "break" should _only_ apply to the inner
switch, regardless of where that switch is placed. In this case it's used
inside of an expression, so maybe that's an excuse for the clang guys to get
creative?

That's why I've added the comment, because I think this is non-obvious.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2 1/2] xsm: fix clang 3.5 build after c47d1d

[Julien Grall]

Hi Roger,

On 10/04/17 10:01, Roger Pau Monne wrote:
> The changes introduced on c47d1d broke the clang build due to undefined
> references to __xsm_action_mismatch_detected, because clang hasn't optimized
> the code properly. The following patch allows the clang build to work again,
> while keeping the same functionality.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Tamas K Lengyel <tamas.lengyel@zentific.com>
> ---
> Changes since v1:
>  - Remove unused "break".
>  - Remove if condition.

This new version does not fix the ARM32 build (though the previous
one does). I still get:

prelink.o: In function `xsm_default_action':
/home/julieng/works/xen/xen/include/xsm/dummy.h:80: undefined reference to `__xsm_action_mismatch_detected'
arm-linux-gnueabihf-ld: /home/julieng/works/xen/xen/.xen-syms.0: hidden symbol `__xsm_action_mismatch_detected' isn't defined

I have just noticed this is also breaking ARM64 build (GCC 6.1).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2 2/2] x86/dm: fix clang build

[Jan Beulich]

>>> On 10.04.17 at 12:34, <roger.pau@citrix.com> wrote:
> On Mon, Apr 10, 2017 at 04:02:37AM -0600, Jan Beulich wrote:
>> >>> On 10.04.17 at 11:46, <roger.pau@citrix.com> wrote:
>> > On Mon, Apr 10, 2017 at 03:27:35AM -0600, Jan Beulich wrote:
>> >> >>> On 10.04.17 at 11:01, <roger.pau@citrix.com> wrote:
>> >> > --- a/xen/arch/x86/hvm/dm.c
>> >> > +++ b/xen/arch/x86/hvm/dm.c
>> >> > @@ -408,9 +408,16 @@ static int dm_op(domid_t domid,
>> >> >          {
>> >> >              struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> >> >  
>> >> > -            while ( read_atomic(&p2m->ioreq.entry_count) &&
>> >> > -                    first_gfn <= p2m->max_mapped_pfn )
>> >> > +            while ( first_gfn <= p2m->max_mapped_pfn )
>> >> >              {
>> >> > +                /*
>> >> > +                 * NB: read_atomic cannot be used in the loop condition because
>> >> > +                 * clang complains with: "'break' is bound to loop, GCC binds
>> >> > +                 * it to switch", so move it inside of the loop instead.
>> >> > +                 */
>> >> > +                if ( !read_atomic(&p2m->ioreq.entry_count) )
>> >> > +                    break;
>> >> 
>> >> How is this behavior of clang in line with the language spec, namely
>> >> "A break statement terminates execution of the smallest enclosing
>> >> switch or iteration statement"?
>> > 
>> > I don't think the condition itself is part of the iteration statement (AFAIK
>> > the condition is the expression, not the statement).
>> 
>> I don't understand (and if I take your wording verbally, then you're
>> giving even more reason for clang being wrong, but I think taking it
>> verbally would be wrong - "while" itself in my understanding very
>> much belongs to the iteration statement, as does the condition; the
>> body of the while statement then is _another_ statement, but not
>> necessarily an iteration one anymore). Anyway, in
>> 
>>     while ( ({ switch ( x ) { default: break; } }) );
>> 
>> I don't think there's any question what "the smallest enclosing switch
>> or iteration statement" is.
> 
> I think the language might not be clear on this case, the definition of a while
> loop statement is:
> 
> while ( expression ) statement
> 
> And then the definition of break:
> 
> A break statement terminates execution of the smallest enclosing switch or
> iteration statement.
> 
> I agree completely with you that the "break" should _only_ apply to the inner
> switch, regardless of where that switch is placed. In this case it's used
> inside of an expression, so maybe that's an excuse for the clang guys to get
> creative?
> 
> That's why I've added the comment, because I think this is non-obvious.

I fully agree that a comment is needed here; I don't, however,
agree to the need to work around such basic compiler bugs in the
first place. In the case here we could call ourselves lucky that the
two parts of the control expression can be freely swapped. What
if that wasn't the case, and the read_atomic() had to go first?

Plus such a shortcoming restricts what code polishing we may do
elsewhere. What if I wanted to eliminate one layer from the
cmpxchg()/__cmpxchg() pair, moving its switch() statement into
a macro just like read_atomic() is?

The only workaround that I would really view as acceptable in a
case like the one here would be if they had a command line option
forcing strict gcc compatibility. Everything else I would put under
question, since the purpose of compiling with different compilers
is to make the overall code _better_, not worse.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2 1/2] xsm: fix clang 3.5 build after c47d1d

[Jan Beulich]

>>> On 10.04.17 at 12:39, <julien.grall@arm.com> wrote:
> Hi Roger,
> 
> On 10/04/17 10:01, Roger Pau Monne wrote:
>> The changes introduced on c47d1d broke the clang build due to undefined
>> references to __xsm_action_mismatch_detected, because clang hasn't optimized
>> the code properly. The following patch allows the clang build to work again,
>> while keeping the same functionality.
>> 
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: Tamas K Lengyel <tamas.lengyel@zentific.com>
>> ---
>> Changes since v1:
>>  - Remove unused "break".
>>  - Remove if condition.
> 
> This new version does not fix the ARM32 build (though the previous
> one does). I still get:
> 
> prelink.o: In function `xsm_default_action':
> /home/julieng/works/xen/xen/include/xsm/dummy.h:80: undefined reference to 
> `__xsm_action_mismatch_detected'
> arm-linux-gnueabihf-ld: /home/julieng/works/xen/xen/.xen-syms.0: hidden 
> symbol `__xsm_action_mismatch_detected' isn't defined

I have to admit that I was afraid of this, after having seen this is
an issue for ARM too. I guess the suggested use of a conditional
expression (instead of if/else) needs to be undone, which is quite
sad.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2 1/2] xsm: fix clang 3.5 build after c47d1d

[Julien Grall]

On 10/04/17 11:50, Jan Beulich wrote:
>>>> On 10.04.17 at 12:39, <julien.grall@arm.com> wrote:
>> Hi Roger,
>>
>> On 10/04/17 10:01, Roger Pau Monne wrote:
>>> The changes introduced on c47d1d broke the clang build due to undefined
>>> references to __xsm_action_mismatch_detected, because clang hasn't optimized
>>> the code properly. The following patch allows the clang build to work again,
>>> while keeping the same functionality.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>> Cc: Tamas K Lengyel <tamas.lengyel@zentific.com>
>>> ---
>>> Changes since v1:
>>>  - Remove unused "break".
>>>  - Remove if condition.
>>
>> This new version does not fix the ARM32 build (though the previous
>> one does). I still get:
>>
>> prelink.o: In function `xsm_default_action':
>> /home/julieng/works/xen/xen/include/xsm/dummy.h:80: undefined reference to
>> `__xsm_action_mismatch_detected'
>> arm-linux-gnueabihf-ld: /home/julieng/works/xen/xen/.xen-syms.0: hidden
>> symbol `__xsm_action_mismatch_detected' isn't defined
>
> I have to admit that I was afraid of this, after having seen this is
> an issue for ARM too. I guess the suggested use of a conditional
> expression (instead of if/else) needs to be undone, which is quite
> sad.

I think this would be the safest for now, I would like to cut an RC soon 
with what has been pushed before the code freeze. This would mean v1 + 
removing pointless break.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel