Re: [PATCH 00/22] add support for Clang LTO

[Jakub Kicinski]

On Tue, 7 Jul 2020 10:17:25 -0700 Nick Desaulniers wrote:
> On Tue, Jul 7, 2020 at 9:56 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> >  
> > > On Tue, Jul 07, 2020 at 08:51:07AM -0700, Sami Tolvanen wrote:  
> > > > After spending some time debugging this with Nick, it looks like the
> > > > error is caused by a recent optimization change in LLVM, which together
> > > > with the inlining of ur_load_imm_any into jeq_imm, changes a runtime
> > > > check in FIELD_FIT that would always fail, to a compile-time check that
> > > > breaks the build. In jeq_imm, we have:
> > > >
> > > >     /* struct bpf_insn: _s32 imm */
> > > >     u64 imm = insn->imm; /* sign extend */
> > > >     ...
> > > >     if (imm >> 32) { /* non-zero only if insn->imm is negative */
> > > >             /* inlined from ur_load_imm_any */
> > > >     u32 __imm = imm >> 32; /* therefore, always 0xffffffff */
> > > >
> > > >         /*
> > > >      * __imm has a value known at compile-time, which means
> > > >      * __builtin_constant_p(__imm) is true and we end up with
> > > >      * essentially this in __BF_FIELD_CHECK:
> > > >      */
> > > >     if (__builtin_constant_p(__imm) && __imm > 255)  
> >
> > I think FIELD_FIT() should not pass the value into __BF_FIELD_CHECK().
> >
> > So:
> >
> > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > index 48ea093ff04c..4e035aca6f7e 100644
> > --- a/include/linux/bitfield.h
> > +++ b/include/linux/bitfield.h
> > @@ -77,7 +77,7 @@
> >   */
> >  #define FIELD_FIT(_mask, _val)                                         \
> >         ({                                                              \
> > -               __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_FIT: ");     \
> > +               __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: ");     \
> >                 !((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
> >         })
> >
> > It's perfectly legal to pass a constant which does not fit, in which
> > case FIELD_FIT() should just return false not break the build.
> >
> > Right?  
> 
> I see the value of the __builtin_constant_p check; this is just a very
> interesting case where rather than an integer literal appearing in the
> source, the compiler is able to deduce that the parameter can only
> have one value in one case, and allows __builtin_constant_p to
> evaluate to true for it.
> 
> I had definitely asked Sami about the comment above FIELD_FIT:
> """
>  76  * Return: true if @_val can fit inside @_mask, false if @_val is
> too big.
> """
> in which FIELD_FIT doesn't return false if @_val is too big and a
> compile time constant. (Rather it breaks the build).
> 
> Of the 14 expansion sites of FIELD_FIT I see in mainline, it doesn't
> look like any integral literals are passed, so maybe the compile time
> checks of _val are of little value for FIELD_FIT.

Also I just double checked and all FIELD_FIT() uses check the return
value.

> So I think your suggested diff is the most concise fix.

Feel free to submit that officially as a patch if it fixes the build
for you, here's my sign-off:

Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx>