[PATCH] arc: Fix __fls() const-foldability via __builtin_clzl()

[PATCH] arc: Fix __fls() const-foldability via __builtin_clzl()

[Kees Cook]

While tracking down a problem where constant expressions used by
BUILD_BUG_ON() suddenly stopped working[1], we found that an added static
initializer was convincing the compiler that it couldn't track the state
of the prior statically initialized value. Tracing this down found that
ffs() was used in the initializer macro, but since it wasn't marked with
__attribute__const__, the compiler had to assume the function might
change variable states as a side-effect (which is not true for ffs(),
which provides deterministic math results).

For arc architecture with CONFIG_ISA_ARCV2=y, the __fls() function
uses __builtin_arc_fls() which lacks GCC's const attribute, preventing
compile-time constant folding[2]. Fix this by handling compile-time
constants with the standard __builtin_clzl() builtin (which has const
attribute) while preserving the optimized arc-specific builtin for runtime
cases. This has the added benefit of skipping runtime calculation of
compile-time constant values.

Build tested ARCH=arc allyesconfig with GCC arc-linux 15.2.0.

Link: https://github.com/KSPP/linux/issues/364 [1]
Link: https://lore.kernel.org/all/202508031025.doWxtzzc-lkp@intel.com/
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202508031025.doWxtzzc-lkp@intel.com/
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Yury Norov <yury.norov@gmail.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Vineet Gupta <vgupta@kernel.org>
Cc: <linux-snps-arc@lists.infradead.org>
---
 arch/arc/include/asm/bitops.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arc/include/asm/bitops.h b/arch/arc/include/asm/bitops.h
index 5340c2871392..df894235fdbc 100644
--- a/arch/arc/include/asm/bitops.h
+++ b/arch/arc/include/asm/bitops.h
@@ -133,6 +133,8 @@ static inline __attribute__ ((const)) int fls(unsigned int x)
  */
 static inline __attribute__ ((const)) unsigned long __fls(unsigned long x)
 {
+	if (__builtin_constant_p(x))
+		return x ? BITS_PER_LONG - 1 - __builtin_clzl(x) : 0;
 	/* FLS insn has exactly same semantics as the API */
 	return	__builtin_arc_fls(x);
 }
-- 
2.34.1

Re: [PATCH] arc: Fix __fls() const-foldability via __builtin_clzl()

[Rasmus Villemoes]

On Mon, Aug 25 2025, Kees Cook <kees@kernel.org> wrote:

> While tracking down a problem where constant expressions used by
> BUILD_BUG_ON() suddenly stopped working[1], we found that an added static
> initializer was convincing the compiler that it couldn't track the state
> of the prior statically initialized value. Tracing this down found that
> ffs() was used in the initializer macro, but since it wasn't marked with
> __attribute__const__, the compiler had to assume the function might
> change variable states as a side-effect (which is not true for ffs(),
> which provides deterministic math results).
>
> For arc architecture with CONFIG_ISA_ARCV2=y, the __fls() function
> uses __builtin_arc_fls() which lacks GCC's const attribute, preventing
> compile-time constant folding[2].

That sounds like a compiler bug, or "missed optimization". Looking into
the gcc source code, it seems that most "generic" builtins (well, those
eligible because they're just math) are internally declared with that
const attribute. E.g. there's

gcc/builtins.def:DEF_GCC_BUILTIN        (BUILT_IN_CLZL, "clzl", BT_FN_INT_ULONG, ATTR_CONST_NOTHROW_LEAF_LIST)

The DEF_BUILTIN macro for e.g. avr seems to support providing such
attributes, e.g. there's

gcc/config/avr/builtins.def:DEF_BUILTIN (ABSHR,   1, hr_ftype_hr,   ssabsqq2, "__ssabs_1", attr_const)

But the arc-specific ones in gcc/config/arc/builtins.def have no such
infrastructure readily available.

Of course, I also don't know if __builtin_arc_fls() is even eligible for
the const attribute (I don't know if it might have some side effects on
a flags register or something). But if it is, perhaps it's possible to
amend gcc's builtin declaration by explicitly (re)declaring it

int __builtin_arc_fls(int x) __attribute_const;

?

If __builtin_arc_fls() simply doesn't qualify for attr_const for
$reason, I think it would be good to have that documented in the commit
msg. If it does, I think a gcc ticket and link to that would be in order.

Rasmus

Re: [PATCH] arc: Fix __fls() const-foldability via __builtin_clzl()

[Kees Cook]

On Tue, Aug 26, 2025 at 03:08:59PM +0200, Rasmus Villemoes wrote:
> If __builtin_arc_fls() simply doesn't qualify for attr_const for
> $reason, I think it would be good to have that documented in the commit
> msg. If it does, I think a gcc ticket and link to that would be in order.

I already sent the patch to fix it. :)

https://gcc.gnu.org/pipermail/gcc-patches/2025-August/693273.html

-- 
Kees Cook

Re: [PATCH] arc: Fix __fls() const-foldability via __builtin_clzl()

[Yury Norov]

On Tue, Aug 26, 2025 at 09:56:06AM -0700, Kees Cook wrote:
> On Tue, Aug 26, 2025 at 03:08:59PM +0200, Rasmus Villemoes wrote:
> > If __builtin_arc_fls() simply doesn't qualify for attr_const for
> > $reason, I think it would be good to have that documented in the commit
> > msg. If it does, I think a gcc ticket and link to that would be in order.
> 
> I already sent the patch to fix it. :)
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2025-August/693273.html

I'm OK taking the patch if it fixes real problem for you, but it looks
more like a GCC problem, right? Is Clang also affected?

If, say, Clang is not affected, and you expect that newer GCC versions
will not be affected too, let's protect the new code with a proper
ifdefery, so that it will be easier to drop the workaround later?

Thanks,
Yury

Re: [PATCH] arc: Fix __fls() const-foldability via __builtin_clzl()

[Kees Cook]

On Tue, Aug 26, 2025 at 09:24:04PM -0400, Yury Norov wrote:
> On Tue, Aug 26, 2025 at 09:56:06AM -0700, Kees Cook wrote:
> > On Tue, Aug 26, 2025 at 03:08:59PM +0200, Rasmus Villemoes wrote:
> > > If __builtin_arc_fls() simply doesn't qualify for attr_const for
> > > $reason, I think it would be good to have that documented in the commit
> > > msg. If it does, I think a gcc ticket and link to that would be in order.
> > 
> > I already sent the patch to fix it. :)
> > 
> > https://gcc.gnu.org/pipermail/gcc-patches/2025-August/693273.html
> 
> I'm OK taking the patch if it fixes real problem for you, but it looks
> more like a GCC problem, right? Is Clang also affected?

Without it, KUnit testing of ffs/fls fails on arc. Clang is not
affected. Even with the GCC fix landed, all older GCCs with still fail,
and since it provides a improved code generation for arc, it seems worth
it (compile-time-calculable values will be emitted instead of always
running the arc instructions).

> If, say, Clang is not affected, and you expect that newer GCC versions
> will not be affected too, let's protect the new code with a proper
> ifdefery, so that it will be easier to drop the workaround later?

I think the codegen benefit is worth it as I have it.

-- 
Kees Cook

Re: [PATCH] arc: Fix __fls() const-foldability via __builtin_clzl()

[Yury Norov]

On Tue, Aug 26, 2025 at 07:13:03PM -0700, Kees Cook wrote:
> On Tue, Aug 26, 2025 at 09:24:04PM -0400, Yury Norov wrote:
> > On Tue, Aug 26, 2025 at 09:56:06AM -0700, Kees Cook wrote:
> > > On Tue, Aug 26, 2025 at 03:08:59PM +0200, Rasmus Villemoes wrote:
> > > > If __builtin_arc_fls() simply doesn't qualify for attr_const for
> > > > $reason, I think it would be good to have that documented in the commit
> > > > msg. If it does, I think a gcc ticket and link to that would be in order.
> > > 
> > > I already sent the patch to fix it. :)
> > > 
> > > https://gcc.gnu.org/pipermail/gcc-patches/2025-August/693273.html
> > 
> > I'm OK taking the patch if it fixes real problem for you, but it looks
> > more like a GCC problem, right? Is Clang also affected?
> 
> Without it, KUnit testing of ffs/fls fails on arc. Clang is not

That's a solid point. Can you please notice it in changelog too?

> affected. Even with the GCC fix landed, all older GCCs with still fail,
> and since it provides a improved code generation for arc, it seems worth
> it (compile-time-calculable values will be emitted instead of always
> running the arc instructions).

Yep. This is what I meant. Once the fix is landed, we will not need this
code. When the minimal supported GCC version will become greater than
one that has the fix applied, it will be much easier to spot this hack
and drop it, if we mention that explicitly.

Something like that:

    #if IS_ENABLED(CONFIG_CC_IS_GCC) && CONFIG_GCC_VERSION < XXX
        if ( __builtin_constant_p(x))
                return x ? BITS_PER_LONG - 1 - __builtin_clzl(x) : 0;
    #endif

> > If, say, Clang is not affected, and you expect that newer GCC versions
> > will not be affected too, let's protect the new code with a proper
> > ifdefery, so that it will be easier to drop the workaround later?
> 
> I think the codegen benefit is worth it as I have it.

That's I'm surely agree.