[v2] dom0-build: fix build with clang5
diff mbox series

Message ID 5c88239b-de0f-5f81-72c4-7fdb07524278@suse.com
State New, archived
Headers show
Series
  • [v2] dom0-build: fix build with clang5
Related show

Commit Message

Jan Beulich July 17, 2019, 6:47 a.m. UTC
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.

Comments

Julien Grall July 29, 2019, 10:05 a.m. UTC | #1
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,
Jan Beulich July 29, 2019, 11:36 a.m. UTC | #2
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
Jan Beulich Dec. 20, 2019, 4:26 p.m. UTC | #3
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 )
>
Julien Grall Jan. 11, 2020, 4:27 p.m. UTC | #4
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,
Roger Pau Monné Jan. 15, 2020, 9:56 a.m. UTC | #5
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.
Roger Pau Monné Jan. 15, 2020, 10 a.m. UTC | #6
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.
Jan Beulich Jan. 15, 2020, 10:18 a.m. UTC | #7
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

Patch
diff mbox series

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