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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.04.17 at 12:34, <roger.pau@xxxxxxxxxx> wrote:
> On Mon, Apr 10, 2017 at 04:02:37AM -0600, Jan Beulich wrote:
>> >>> On 10.04.17 at 11:46, <roger.pau@xxxxxxxxxx> wrote:
>> > On Mon, Apr 10, 2017 at 03:27:35AM -0600, Jan Beulich wrote:
>> >> >>> On 10.04.17 at 11:01, <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel