[Xen-devel] [PATCH v2] dom0-build: fix build with clang5

[Xen-devel] [PATCH v2] dom0-build: fix build with clang5

[Jan Beulich]

With non-empty CONFIG_DOM0_MEM clang5 produces

dom0_build.c:344:24: error: use of logical '&&' with constant operand [-Werror,-Wconstant-logical-operand]
     if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
                        ^  ~~~~~~~~~~~~~~~~~~
dom0_build.c:344:24: note: use '&' for a bitwise operation
     if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
                        ^~
                        &
dom0_build.c:344:24: note: remove constant to silence this warning
     if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
                       ~^~~~~~~~~~~~~~~~~~~~~
1 error generated.

Obviously neither of the two suggestions are an option here. Oddly
enough swapping the operands of the && helps, while e.g. casting or
parenthesizing doesn't. Another workable variant looks to be the use of
!! on the constant.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Also adjust the Arm incarnation of the same construct.
---
I'm open to going the !! or yet some different route (but not really the
suggested strlen() one). No matter which one we choose, I'm afraid it is
going to remain guesswork what newer (and future) versions of clang will
choke on.

--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2125,7 +2125,7 @@ int __init construct_dom0(struct domain
  
      printk("*** LOADING DOMAIN 0 ***\n");
  
-    if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
+    if ( CONFIG_DOM0_MEM[0] && !dom0_mem_set )
          parse_dom0_mem(CONFIG_DOM0_MEM);
  
      if ( dom0_mem <= 0 )
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -341,7 +341,7 @@ unsigned long __init dom0_compute_nr_pag
      unsigned long avail = 0, nr_pages, min_pages, max_pages;
      bool need_paging;
  
-    if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
+    if ( CONFIG_DOM0_MEM[0] && !dom0_mem_set )
          parse_dom0_mem(CONFIG_DOM0_MEM);
  
      for_each_node_mask ( node, dom0_nodes )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] dom0-build: fix build with clang5

[Julien Grall]

Hi,

On 7/17/19 7:47 AM, Jan Beulich wrote:
> With non-empty CONFIG_DOM0_MEM clang5 produces
> 
> dom0_build.c:344:24: error: use of logical '&&' with constant operand [-Werror,-Wconstant-logical-operand]
>       if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>                          ^  ~~~~~~~~~~~~~~~~~~
> dom0_build.c:344:24: note: use '&' for a bitwise operation
>       if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>                          ^~
>                          &
> dom0_build.c:344:24: note: remove constant to silence this warning
>       if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>                         ~^~~~~~~~~~~~~~~~~~~~~
> 1 error generated.
> 
> Obviously neither of the two suggestions are an option here. Oddly
> enough swapping the operands of the && helps, while e.g. casting or
> parenthesizing doesn't. Another workable variant looks to be the use of
> !! on the constant.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Also adjust the Arm incarnation of the same construct.
> ---
> I'm open to going the !! or yet some different route (but not really the
> suggested strlen() one). No matter which one we choose, I'm afraid it is
> going to remain guesswork what newer (and future) versions of clang will
> choke on.

I quite like the strlen one, however looking around online this may not 
solve the problem. AFAIK, Clang is not happy because the constant is not 
a boolean.

So !! or != 0 should work here.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2] dom0-build: fix build with clang5

[Jan Beulich]

On 29.07.2019 12:05, Julien Grall wrote:
> Hi,
> 
> On 7/17/19 7:47 AM, Jan Beulich wrote:
>> With non-empty CONFIG_DOM0_MEM clang5 produces
>>
>> dom0_build.c:344:24: error: use of logical '&&' with constant operand [-Werror,-Wconstant-logical-operand]
>>       if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>>                          ^  ~~~~~~~~~~~~~~~~~~
>> dom0_build.c:344:24: note: use '&' for a bitwise operation
>>       if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>>                          ^~
>>                          &
>> dom0_build.c:344:24: note: remove constant to silence this warning
>>       if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>>                         ~^~~~~~~~~~~~~~~~~~~~~
>> 1 error generated.
>>
>> Obviously neither of the two suggestions are an option here. Oddly
>> enough swapping the operands of the && helps, while e.g. casting or
>> parenthesizing doesn't. Another workable variant looks to be the use of
>> !! on the constant.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: Also adjust the Arm incarnation of the same construct.
>> ---
>> I'm open to going the !! or yet some different route (but not really the
>> suggested strlen() one). No matter which one we choose, I'm afraid it is
>> going to remain guesswork what newer (and future) versions of clang will
>> choke on.
> 
> I quite like the strlen one, however looking around online this may not
> solve the problem. AFAIK, Clang is not happy because the constant is not
> a boolean.

I don't think it's as simple as "not a boolean", but I also haven't
played with it more than I had to in order to find possible
workarounds. As to the strlen() approach - I've expressed my dislike
before.

> So !! or != 0 should work here.

Well, with perhaps as much of a chance of breaking again as any
other of the possible solutions, unless they recognize and address
the oddity. Andrew - would you be willing to ack a v3 using !!
instead of re-ordering the && operands (seeing that you didn't ack
v2 yet, and perhaps you're also not meaning to)?

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] Ping: [PATCH v2] dom0-build: fix build with clang5

[Jan Beulich]

On 17.07.2019 08:47, Jan Beulich wrote:
> With non-empty CONFIG_DOM0_MEM clang5 produces
> 
> dom0_build.c:344:24: error: use of logical '&&' with constant operand [-Werror,-Wconstant-logical-operand]
>      if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>                         ^  ~~~~~~~~~~~~~~~~~~
> dom0_build.c:344:24: note: use '&' for a bitwise operation
>      if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>                         ^~
>                         &
> dom0_build.c:344:24: note: remove constant to silence this warning
>      if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>                        ~^~~~~~~~~~~~~~~~~~~~~
> 1 error generated.
> 
> Obviously neither of the two suggestions are an option here. Oddly
> enough swapping the operands of the && helps, while e.g. casting or
> parenthesizing doesn't. Another workable variant looks to be the use of
> !! on the constant.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Also adjust the Arm incarnation of the same construct.
> ---
> I'm open to going the !! or yet some different route (but not really the
> suggested strlen() one). No matter which one we choose, I'm afraid it is
> going to remain guesswork what newer (and future) versions of clang will
> choke on.

I guess the disagreement on how to exactly address the issue has
stalled this. But I think we should rather have _some_ (e.g.
this) solution in the repo, than continue to ship versions which
don't build. People wanting to beautify the code further could
then submit incremental patches.

Jan

> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2125,7 +2125,7 @@ int __init construct_dom0(struct domain
>   
>       printk("*** LOADING DOMAIN 0 ***\n");
>   
> -    if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
> +    if ( CONFIG_DOM0_MEM[0] && !dom0_mem_set )
>           parse_dom0_mem(CONFIG_DOM0_MEM);
>   
>       if ( dom0_mem <= 0 )
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -341,7 +341,7 @@ unsigned long __init dom0_compute_nr_pag
>       unsigned long avail = 0, nr_pages, min_pages, max_pages;
>       bool need_paging;
>   
> -    if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
> +    if ( CONFIG_DOM0_MEM[0] && !dom0_mem_set )
>           parse_dom0_mem(CONFIG_DOM0_MEM);
>   
>       for_each_node_mask ( node, dom0_nodes )
> 


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

Re: [Xen-devel] Ping: [PATCH v2] dom0-build: fix build with clang5

[Julien Grall]

On 20/12/2019 16:26, Jan Beulich wrote:
> On 17.07.2019 08:47, Jan Beulich wrote:
>> With non-empty CONFIG_DOM0_MEM clang5 produces
>>
>> dom0_build.c:344:24: error: use of logical '&&' with constant operand [-Werror,-Wconstant-logical-operand]
>>       if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>>                          ^  ~~~~~~~~~~~~~~~~~~
>> dom0_build.c:344:24: note: use '&' for a bitwise operation
>>       if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>>                          ^~
>>                          &
>> dom0_build.c:344:24: note: remove constant to silence this warning
>>       if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>>                         ~^~~~~~~~~~~~~~~~~~~~~
>> 1 error generated.
>>
>> Obviously neither of the two suggestions are an option here. Oddly
>> enough swapping the operands of the && helps, while e.g. casting or
>> parenthesizing doesn't. Another workable variant looks to be the use of
>> !! on the constant.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: Also adjust the Arm incarnation of the same construct.
>> ---
>> I'm open to going the !! or yet some different route (but not really the
>> suggested strlen() one). No matter which one we choose, I'm afraid it is
>> going to remain guesswork what newer (and future) versions of clang will
>> choke on.
> 
> I guess the disagreement on how to exactly address the issue has
> stalled this. But I think we should rather have _some_ (e.g.
> this) solution in the repo, than continue to ship versions which
> don't build. People wanting to beautify the code further could
> then submit incremental patches.

I would prefer a more readable code but for the sake of unblocking:

Acked-by: Julien Grall <julien@xen.org>

Note that clang is not officially supported to build Xen on Arm. So the 
build concern is x86 only.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] Ping: [PATCH v2] dom0-build: fix build with clang5

[Roger Pau Monné]

On Fri, Dec 20, 2019 at 05:26:34PM +0100, Jan Beulich wrote:
> On 17.07.2019 08:47, Jan Beulich wrote:
> > With non-empty CONFIG_DOM0_MEM clang5 produces
> > 
> > dom0_build.c:344:24: error: use of logical '&&' with constant operand [-Werror,-Wconstant-logical-operand]
> >      if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
> >                         ^  ~~~~~~~~~~~~~~~~~~
> > dom0_build.c:344:24: note: use '&' for a bitwise operation
> >      if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
> >                         ^~
> >                         &
> > dom0_build.c:344:24: note: remove constant to silence this warning
> >      if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
> >                        ~^~~~~~~~~~~~~~~~~~~~~
> > 1 error generated.
> > 
> > Obviously neither of the two suggestions are an option here. Oddly
> > enough swapping the operands of the && helps, while e.g. casting or
> > parenthesizing doesn't. Another workable variant looks to be the use of
> > !! on the constant.
> > 
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > v2: Also adjust the Arm incarnation of the same construct.
> > ---
> > I'm open to going the !! or yet some different route (but not really the
> > suggested strlen() one). No matter which one we choose, I'm afraid it is
> > going to remain guesswork what newer (and future) versions of clang will
> > choke on.
> 
> I guess the disagreement on how to exactly address the issue has
> stalled this. But I think we should rather have _some_ (e.g.
> this) solution in the repo, than continue to ship versions which
> don't build. People wanting to beautify the code further could
> then submit incremental patches.

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

I'm not providing a RB because this is all guesswork, so it doesn't
feel appropriate to review something that's based on undocumented
compiler behavior.

Another option would be to pass -Wconstant-logical-operand but that
would prevent caching some licit issues.

Thanks, Roger.

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

Re: [Xen-devel] Ping: [PATCH v2] dom0-build: fix build with clang5

[Roger Pau Monné]

On Wed, Jan 15, 2020 at 10:56:37AM +0100, Roger Pau Monné wrote:
> On Fri, Dec 20, 2019 at 05:26:34PM +0100, Jan Beulich wrote:
> > On 17.07.2019 08:47, Jan Beulich wrote:
> > > With non-empty CONFIG_DOM0_MEM clang5 produces
> > > 
> > > dom0_build.c:344:24: error: use of logical '&&' with constant operand [-Werror,-Wconstant-logical-operand]
> > >      if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
> > >                         ^  ~~~~~~~~~~~~~~~~~~
> > > dom0_build.c:344:24: note: use '&' for a bitwise operation
> > >      if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
> > >                         ^~
> > >                         &
> > > dom0_build.c:344:24: note: remove constant to silence this warning
> > >      if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
> > >                        ~^~~~~~~~~~~~~~~~~~~~~
> > > 1 error generated.
> > > 
> > > Obviously neither of the two suggestions are an option here. Oddly
> > > enough swapping the operands of the && helps, while e.g. casting or
> > > parenthesizing doesn't. Another workable variant looks to be the use of
> > > !! on the constant.
> > > 
> > > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > > ---
> > > v2: Also adjust the Arm incarnation of the same construct.
> > > ---
> > > I'm open to going the !! or yet some different route (but not really the
> > > suggested strlen() one). No matter which one we choose, I'm afraid it is
> > > going to remain guesswork what newer (and future) versions of clang will
> > > choke on.
> > 
> > I guess the disagreement on how to exactly address the issue has
> > stalled this. But I think we should rather have _some_ (e.g.
> > this) solution in the repo, than continue to ship versions which
> > don't build. People wanting to beautify the code further could
> > then submit incremental patches.
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I'm not providing a RB because this is all guesswork, so it doesn't
> feel appropriate to review something that's based on undocumented
> compiler behavior.
> 
> Another option would be to pass -Wconstant-logical-operand but that
> would prevent caching some licit issues.

Forgot to mention, but could you please add a comment to note that the
condition is ordered this way to make clang5 happy?

Roger.

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

Re: [Xen-devel] Ping: [PATCH v2] dom0-build: fix build with clang5

[Jan Beulich]

On 15.01.2020 11:00, Roger Pau Monné wrote:
> On Wed, Jan 15, 2020 at 10:56:37AM +0100, Roger Pau Monné wrote:
>> On Fri, Dec 20, 2019 at 05:26:34PM +0100, Jan Beulich wrote:
>>> On 17.07.2019 08:47, Jan Beulich wrote:
>>>> With non-empty CONFIG_DOM0_MEM clang5 produces
>>>>
>>>> dom0_build.c:344:24: error: use of logical '&&' with constant operand [-Werror,-Wconstant-logical-operand]
>>>>      if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>>>>                         ^  ~~~~~~~~~~~~~~~~~~
>>>> dom0_build.c:344:24: note: use '&' for a bitwise operation
>>>>      if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>>>>                         ^~
>>>>                         &
>>>> dom0_build.c:344:24: note: remove constant to silence this warning
>>>>      if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
>>>>                        ~^~~~~~~~~~~~~~~~~~~~~
>>>> 1 error generated.
>>>>
>>>> Obviously neither of the two suggestions are an option here. Oddly
>>>> enough swapping the operands of the && helps, while e.g. casting or
>>>> parenthesizing doesn't. Another workable variant looks to be the use of
>>>> !! on the constant.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> v2: Also adjust the Arm incarnation of the same construct.
>>>> ---
>>>> I'm open to going the !! or yet some different route (but not really the
>>>> suggested strlen() one). No matter which one we choose, I'm afraid it is
>>>> going to remain guesswork what newer (and future) versions of clang will
>>>> choke on.
>>>
>>> I guess the disagreement on how to exactly address the issue has
>>> stalled this. But I think we should rather have _some_ (e.g.
>>> this) solution in the repo, than continue to ship versions which
>>> don't build. People wanting to beautify the code further could
>>> then submit incremental patches.
>>
>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> I'm not providing a RB because this is all guesswork, so it doesn't
>> feel appropriate to review something that's based on undocumented
>> compiler behavior.
>>
>> Another option would be to pass -Wconstant-logical-operand but that
>> would prevent caching some licit issues.
> 
> Forgot to mention, but could you please add a comment to note that the
> condition is ordered this way to make clang5 happy?

I've added

    /* The ordering of operands is to work around a clang5 issue. */

to both instances.

Jan

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