| Message ID | ef670212-8257-b960-a3cb-9363cf076033@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/dom0-build: fix build with clang5 | expand |
On Mon, Jul 15, 2019 at 4:36 AM Jan Beulich <JBeulich@suse.com> 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> > --- > I'm open to going the !! or yet some different route. 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. Is disabling the check itself not an option? Seems to me to be a more sensible option then hacking around it. Tamas
On 15/07/2019 11:35, 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> > --- > I'm open to going the !! or yet some different route. 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/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); First and foremost, there is an identical construct on the ARM side, which wants an equivalent adjustment. As to the change, I'm going to suggest what I suggested instead of this the first time around, which is strlen(CONFIG_DOM0_MEM) to make the logic easier to follow. ~Andrew
On 15.07.2019 15:41, Tamas K Lengyel wrote: > On Mon, Jul 15, 2019 at 4:36 AM Jan Beulich <JBeulich@suse.com> 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> >> --- >> I'm open to going the !! or yet some different route. 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. > > Is disabling the check itself not an option? Seems to me to be a more > sensible option then hacking around it. How would you envision to disable the check? It's there for a reason after all. Jan
On 15.07.2019 15:49, Andrew Cooper wrote: > On 15/07/2019 11:35, 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> >> --- >> I'm open to going the !! or yet some different route. 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/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); > > First and foremost, there is an identical construct on the ARM side, > which wants an equivalent adjustment. Oh, indeed. I should have remembered ... > As to the change, I'm going to suggest what I suggested instead of this > the first time around, which is strlen(CONFIG_DOM0_MEM) to make the > logic easier to follow. How does use of strlen() make anything easier? I think it is a pretty common pattern to check the first character for nul if all one is after is to know whether a string is empty. Jan
On Mon, Jul 15, 2019 at 8:13 AM Jan Beulich <JBeulich@suse.com> wrote: > > On 15.07.2019 15:41, Tamas K Lengyel wrote: > > On Mon, Jul 15, 2019 at 4:36 AM Jan Beulich <JBeulich@suse.com> 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> > >> --- > >> I'm open to going the !! or yet some different route. 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. > > > > Is disabling the check itself not an option? Seems to me to be a more > > sensible option then hacking around it. > > How would you envision to disable the check? It's there for a reason > after all. By passing -Wno-constant-logical-operand to the compiler. It looks like a check for a non-common situation which evidently Xen uses, so what's the point of keeping it but hacking around with it with tricks that are fragile? Tamas
On 15.07.2019 16:44, Tamas K Lengyel wrote: > On Mon, Jul 15, 2019 at 8:13 AM Jan Beulich <JBeulich@suse.com> wrote: >> >> On 15.07.2019 15:41, Tamas K Lengyel wrote: >>> On Mon, Jul 15, 2019 at 4:36 AM Jan Beulich <JBeulich@suse.com> 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> >>>> --- >>>> I'm open to going the !! or yet some different route. 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. >>> >>> Is disabling the check itself not an option? Seems to me to be a more >>> sensible option then hacking around it. >> >> How would you envision to disable the check? It's there for a reason >> after all. > > By passing -Wno-constant-logical-operand to the compiler. It looks > like a check for a non-common situation which evidently Xen uses, so > what's the point of keeping it but hacking around with it with tricks > that are fragile? Oh, so you meant disabling the compiler warning. That may be an option, but I wouldn't routinely suggest such as often besides unhelpful instances there are also helpful ones. Jan
On 15/07/2019 15:19, Jan Beulich wrote: > On 15.07.2019 15:49, Andrew Cooper wrote: >> On 15/07/2019 11:35, 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> >>> --- >>> I'm open to going the !! or yet some different route. 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/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); >> First and foremost, there is an identical construct on the ARM side, >> which wants an equivalent adjustment. > Oh, indeed. I should have remembered ... > >> As to the change, I'm going to suggest what I suggested instead of this >> the first time around, which is strlen(CONFIG_DOM0_MEM) to make the >> logic easier to follow. > How does use of strlen() make anything easier? I think it is a pretty > common pattern to check the first character for nul if all one is > after is to know whether a string is empty. Only for things which are obviously a string. CONFIG_DOM0_MEM isn't. In this case, you have a name which would most obviously be an integer, which is compiling in a context where an array reference is valid, which is confusing to read. As strlen() is implemented with a builtin, it should be evaluated by the compiler, given that CONFIG_DOM0_MEM is a constant, but hopefully won't trigger this warning. ~Andrew
--- 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> --- I'm open to going the !! or yet some different route. 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. x86/dom0-build: fix build with clang5 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> --- I'm open to going the !! or yet some different route. 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/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 )