| Message ID | 5c88239b-de0f-5f81-72c4-7fdb07524278@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] dom0-build: fix build with clang5 | expand |
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,
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
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 ) >
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,
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.
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.
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
--- 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 )
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.