Re: xen | Failed pipeline for staging | 221f2748

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.09.2024 17:08, Roger Pau Monné wrote:
> On Thu, Sep 12, 2024 at 04:30:29PM +0200, Jan Beulich wrote:
>> On 12.09.2024 14:52, GitLab wrote:
>>>
>>>
>>> Pipeline #1450753094 has failed!
>>>
>>> Project: xen ( https://gitlab.com/xen-project/hardware/xen )
>>> Branch: staging ( 
>>> https://gitlab.com/xen-project/hardware/xen/-/commits/staging )
>>>
>>> Commit: 221f2748 ( 
>>> https://gitlab.com/xen-project/hardware/xen/-/commit/221f2748e8dabe8361b8cdfcffbeab9102c4c899
>>>  )
>>> Commit Message: blkif: reconcile protocol specification with in...
>>> Commit Author: Roger Pau Monné
>>> Committed by: Jan Beulich ( https://gitlab.com/jbeulich )
>>>
>>>
>>> Pipeline #1450753094 ( 
>>> https://gitlab.com/xen-project/hardware/xen/-/pipelines/1450753094 ) 
>>> triggered by Jan Beulich ( https://gitlab.com/jbeulich )
>>> had 13 failed jobs.
>>>
>>> Job #7809027717 ( 
>>> https://gitlab.com/xen-project/hardware/xen/-/jobs/7809027717/raw )
>>>
>>> Stage: build
>>> Name: ubuntu-24.04-x86_64-clang
>>> Job #7809027747 ( 
>>> https://gitlab.com/xen-project/hardware/xen/-/jobs/7809027747/raw )
>>>
>>> Stage: build
>>> Name: opensuse-tumbleweed-clang
>>> Job #7809027539 ( 
>>> https://gitlab.com/xen-project/hardware/xen/-/jobs/7809027539/raw )
>>>
>>> Stage: build
>>> Name: debian-bookworm-clang-debug
>>
>> I picked this one as example - Clang dislikes the switch to bool in
>>
>> --- a/xen/arch/x86/include/asm/mm.h
>> +++ b/xen/arch/x86/include/asm/mm.h
>> @@ -286,8 +286,8 @@ struct page_info
>>          struct {
>>              u16 nr_validated_ptes:PAGETABLE_ORDER + 1;
>>              u16 :16 - PAGETABLE_ORDER - 1 - 1;
>> -            u16 partial_flags:1;
>> -            s16 linear_pt_count;
>> +            bool partial_flags:1;
>> +            int16_t linear_pt_count;
>>          };
>>  
>>          /*
>>
>> for places where that field's set using PTF_partial_set:
>>
>> arch/x86/mm.c:1582:35: error: converting the result of '<<' to a boolean 
>> always evaluates to true [-Werror,-Wtautological-constant-compare]
>>             page->partial_flags = PTF_partial_set;
>>                                   ^
>> I wonder why we're not using plain "true" there. Alternatively the change to
>> bool would need undoing.
> 
> I'm hitting this locally when building with clang.
> 
> I find it confusing that the partial_flags field cannot possibly be a
> flags field, for it having a width of 1 bit.

I meanwhile guess the idea may have been that the field could be widened
if needed, and the low bit would then retain its present meaning. How
well that would work out if a need for that arose I can't tell of course.

> I think my proposal would be to rename to partially_validated (or
> similar) and set it using `true`, but that would also imply re-writing
> part of the comment in struct page_info definition.

This may have been named just "partial" originally. Yet with the above
maybe we really should switch back to uint16_t (for the time being; I'm
unconvinced of the use of fixed-width types here, or in general when it
comes to bitfields).

Jan