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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
>>> ---
>>> 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

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