| Message ID | 20170410090157.49501-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
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.
>>> 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
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.
>>> 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
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);
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(-)