All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Laight <david.laight.linux@gmail.com>
To: Matt Coster <matt.coster@imgtec.com>
Cc: Yury Norov <yury.norov@gmail.com>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	Frank Binns <frank.binns@imgtec.com>,
	Alessio Belle <alessio.belle@imgtec.com>,
	Brajesh Gupta <brajesh.gupta@imgtec.com>,
	Alexandru Dadu <alexandru.dadu@imgtec.com>,
	<linux-kernel@vger.kernel.org>,
	Vincent Mailhol <mailhol@kernel.org>,
	kernel test robot <lkp@intel.com>
Subject: Re: [PATCH] bitfield: Fix FIELD_PREP_CONST() with __GENMASK_ULL() on gcc<14
Date: Thu, 9 Apr 2026 19:48:33 +0100	[thread overview]
Message-ID: <20260409194833.3214139d@pumpkin> (raw)
In-Reply-To: <20260409-field-prep-fix-v1-1-f0e9ae64f63c@imgtec.com>

On Thu, 9 Apr 2026 15:57:53 +0100
Matt Coster <matt.coster@imgtec.com> wrote:

> There is a bug in gcc<14[1] that causes the following minimal example to
> not be treated as a constant:
> 
>   int main() {
>       sizeof(struct {
>           int t : !(__builtin_ffsll(~0ULL) + 1 < 0);
>       });
>   }
> 
>   test.c: In function 'main':
>   test.c:3:21: error: bit-field 't' width not an integer constant
>       3 |                 int t : !(__builtin_ffsll(~0ULL) + 1 < 0);
>         |                     ^
> 
> The result of this bug is a bizarre interaction between FIELD_PREP_CONST()
> and __GENMASK_ULL(). Note that this does not occur with GENMASK_ULL() since
> that has not been based on the UAPI variant since commit 104ea1c84b91
> ("bits: unify the non-asm GENMASK*()").

Can you give the actual code that is failing.
I can only find the old version - that is pretty horrid itself.
It really isn't necessary to use GENMASK_ULL(15, 0) to get 0xffffu.

I've bumped into that file before.
It gave a false positive for FIELD_GET() returning a constant value
for non-constant input - which would be a nice check for invalid usage.

Someone went over the top on getting the pre-processor to bloat the
intermediate file by using the longest way possible to perform the
simplest of code patterns.
I suspect they've made it worse.

> 
> The underlying compiler bug has been fixed in gcc since 14.1.0, but the fix
> appears to have been incidental in a larger change[2].


> 
> [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124699
> [2]: https://gcc.gnu.org/cgit/gcc/commit/?id=0d00385eaf72ccacff17935b0d214a26773e095f
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202603222211.A2XiR1YU-lkp@intel.com/
> Signed-off-by: Matt Coster <matt.coster@imgtec.com>
> ---
> Some below-the-dash thoughts:
> 
> This is the most minimal workaround I could find. I'm not sure what a
> "real" fix would look like here so I'm open to suggestions.

Using a completely different expression instead of ffs() and shifts.
For constants it doesn't matter what you do.
Multiply/divide by mask & (~mask + 1) will be the same.
(I think it is used lower down the file.)

But passing the output of GENMASK() (any variant) into FIELD_PREP()
(any current variant) is a good way to generate pre-processor output
lines that are many kb long.

Oh, in my experiments, the value passed to ffsll() must be unsigned
and have the top bit set.

	David

> 
> The reproduction case is amazing because changing almost any subtle
> detail of it will result in the expression correctly being parsed as
> constant (even raising the construct to file scope; the use of
> BUILD_BUG_ON_ZERO() is part of the bizarre confluence required to
> trigger the bug).
> 
> The complexity of the associated change in gcc makes it difficult to
> trace what actually changed to fix the underlying bug. If I had more
> time, I'd have dug in further and (tried to) come up with an answer.
> 
> In reality, the (long long) cast could probably just be univerally
> applied without the conditional compilation. My only concern with that
> approach is that it risks turning the workaround into an arcane
> incantation whose meaning will eventually have been lost to time.
> ---
>  include/linux/bitfield.h | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 54aeeef1f0ec7..12f5c5a3c8d72 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -46,6 +46,30 @@
>  
>  #define __bf_shf(x) (__builtin_ffsll(x) - 1)
>  
> +#if defined(GCC_VERSION) && (GCC_VERSION < 140000)
> +/*
> + * This workaround is required for gcc<14. The issue is an interaction between
> + * FIELD_PREP_CONST() and __GENMASK_ULL() that can be boiled down this MCVE,
> + * which fails to compile as GCC doesn't recognize the expression as constant:
> + *
> + *   int main() {
> + *       sizeof(struct {
> + *           int t : !(__builtin_ffsll(~0ULL) + 1 < 0);
> + *       });
> + *   }
> + *
> + * The underlying issue was inadvertently "fixed" (or perhaps sidestepped) in
> + * commit 0d00385eaf7 ("wide-int: Allow up to 16320 bits wide_int and change
> + * widest_int precision to 32640 bits [PR102989]"), which first appeared in
> + * GCC 14.1.
> + *
> + * See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124699
> + */
> +#define __const_bf_shf(x) __bf_shf((long long)(x))
> +#else
> +#define __const_bf_shf(x) __bf_shf(x)
> +#endif
> +
>  #define __scalar_type_to_unsigned_cases(type)				\
>  		unsigned type:	(unsigned type)0,			\
>  		signed type:	(unsigned type)0
> @@ -157,11 +181,11 @@
>  		/* mask must be non-zero */				\
>  		BUILD_BUG_ON_ZERO((_mask) == 0) +			\
>  		/* check if value fits */				\
> -		BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) + \
> +		BUILD_BUG_ON_ZERO(~((_mask) >> __const_bf_shf(_mask)) & (_val)) + \
>  		/* check if mask is contiguous */			\
> -		__BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask))) +	\
> +		__BF_CHECK_POW2((_mask) + (1ULL << __const_bf_shf(_mask))) + \
>  		/* and create the value */				\
> -		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))	\
> +		(((typeof(_mask))(_val) << __const_bf_shf(_mask)) & (_mask)) \
>  	)
>  
>  /**
> 
> ---
> base-commit: b20a9b5f9c4baeae0b2e143046b195b910c59714
> change-id: 20260324-field-prep-fix-fdfc4eb68156
> 
> 


      parent reply	other threads:[~2026-04-09 18:48 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-09 14:57 [PATCH] bitfield: Fix FIELD_PREP_CONST() with __GENMASK_ULL() on gcc<14 Matt Coster
2026-04-09 17:46 ` Yury Norov
2026-04-10  9:09   ` David Laight
2026-04-09 18:48 ` David Laight [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260409194833.3214139d@pumpkin \
    --to=david.laight.linux@gmail.com \
    --cc=alessio.belle@imgtec.com \
    --cc=alexandru.dadu@imgtec.com \
    --cc=brajesh.gupta@imgtec.com \
    --cc=frank.binns@imgtec.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@rasmusvillemoes.dk \
    --cc=lkp@intel.com \
    --cc=mailhol@kernel.org \
    --cc=matt.coster@imgtec.com \
    --cc=yury.norov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.