Re: xen | Failed pipeline for staging | 221f2748

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 12, 2024 at 05:13:04PM +0200, Jan Beulich wrote:
> 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).

Seeing it's not straightforward how to fix this, I think it's best if
for the time being we revert this part of the change, going back to
use uint16_t for the field.

Thanks, Roger.