BUILD_BUG_ON(!__builtin_constant_p(feature)) breaks bcc trace tool

BUILD_BUG_ON(!__builtin_constant_p(feature)) breaks bcc trace tool

[Anton Blanchard]

Hi,

We added:

BUILD_BUG_ON(!__builtin_constant_p(feature)) 

to cpu_has_feature() and mmu_has_feature() in order to catch usage
issues (such as cpu_has_feature(cpu_has_feature(X)). Unfortunately LLVM
isn't smart enough to resolve this, and it errors out.

I work around it in my clang/LLVM builds of the kernel, but I have just
discovered that it causes a lot of issues for the bcc (eBPF) trace tool
(which uses LLVM).

How should we work around this? Wrap the checks in !clang perhaps?

Anton

Re: BUILD_BUG_ON(!__builtin_constant_p(feature)) breaks bcc trace tool

[Anton Blanchard]

Hi,

> We added:
> 
> BUILD_BUG_ON(!__builtin_constant_p(feature)) 
> 
> to cpu_has_feature() and mmu_has_feature() in order to catch usage
> issues (such as cpu_has_feature(cpu_has_feature(X)). Unfortunately
> LLVM isn't smart enough to resolve this, and it errors out.
> 
> I work around it in my clang/LLVM builds of the kernel, but I have
> just discovered that it causes a lot of issues for the bcc (eBPF)
> trace tool (which uses LLVM).
> 
> How should we work around this? Wrap the checks in !clang perhaps?

Looks like it's a weakness in LLVM with inlining:

#include <assert.h>

#if 1
static inline void foo(unsigned long x)
{
        assert(__builtin_constant_p(x));
}
#else
#define foo(X) assert(__builtin_constant_p(X))
#endif

int main(void)
{
        foo(1);

        return 0;
}

And there is an old bug on it:

https://llvm.org/bugs/show_bug.cgi?id=4898

Anton

Re: BUILD_BUG_ON(!__builtin_constant_p(feature)) breaks bcc trace tool

[Michael Ellerman]

Anton Blanchard <anton@samba.org> writes:
>> We added:
>> 
>> BUILD_BUG_ON(!__builtin_constant_p(feature)) 
>> 
>> to cpu_has_feature() and mmu_has_feature() in order to catch usage
>> issues (such as cpu_has_feature(cpu_has_feature(X)). Unfortunately
>> LLVM isn't smart enough to resolve this, and it errors out.
>> 
>> I work around it in my clang/LLVM builds of the kernel, but I have
>> just discovered that it causes a lot of issues for the bcc (eBPF)
>> trace tool (which uses LLVM).
>> 
>> How should we work around this? Wrap the checks in !clang perhaps?
>
> Looks like it's a weakness in LLVM with inlining:
>
> #include <assert.h>
>
> #if 1
> static inline void foo(unsigned long x)
> {
>         assert(__builtin_constant_p(x));
> }
> #else
> #define foo(X) assert(__builtin_constant_p(X))
> #endif
>
> int main(void)
> {
>         foo(1);
>
>         return 0;
> }

OK. cpu_has_feature() is __always_inline:

static __always_inline bool cpu_has_feature(unsigned long feature)
{
	int i;

	BUILD_BUG_ON(!__builtin_constant_p(feature));


But I guess even that doesn't work?

Hmm, in fact it seems because we don't define
CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING and CONFIG_OPTIMIZE_INLINING, we
get:

#define inline		inline		__attribute__((always_inline)) notrace

So in fact every inline function is marked always_inline all the time,
which seems dubious.

But still, it seems clang is ignoring always_inline.

cheers

Re: BUILD_BUG_ON(!__builtin_constant_p(feature)) breaks bcc trace tool

[Arnd Bergmann]

On Tue, Jan 24, 2017 at 7:15 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:

> Hmm, in fact it seems because we don't define
> CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING and CONFIG_OPTIMIZE_INLINING, we
> get:
>
> #define inline          inline          __attribute__((always_inline)) notrace
>
> So in fact every inline function is marked always_inline all the time,
> which seems dubious.
>
> But still, it seems clang is ignoring always_inline.

I think the problem is that __builtin_constant_p() is usually false on
clang, at least for arguments
that are not constant pointers.

    Arnd

RE: BUILD_BUG_ON(!__builtin_constant_p(feature)) breaks bcc trace tool

[David Laight]

From: Michael Ellerman
> Sent: 24 January 2017 06:16
> Anton Blanchard <anton@samba.org> writes:
> >> We added:
> >>
> >> BUILD_BUG_ON(!__builtin_constant_p(feature))
> >>
> >> to cpu_has_feature() and mmu_has_feature() in order to catch usage
> >> issues (such as cpu_has_feature(cpu_has_feature(X)). Unfortunately
> >> LLVM isn't smart enough to resolve this, and it errors out.
> >>
> >> I work around it in my clang/LLVM builds of the kernel, but I have
> >> just discovered that it causes a lot of issues for the bcc (eBPF)
> >> trace tool (which uses LLVM).
> >>
> >> How should we work around this? Wrap the checks in !clang perhaps?
> >
> > Looks like it's a weakness in LLVM with inlining:
> >
> > #include <assert.h>
> >
> > #if 1
> > static inline void foo(unsigned long x)

Does making this 'const unsigned long x' help?
ISTR that making a slight difference to the instruction patterns
gcc would use.

> > {
> >         assert(__builtin_constant_p(x));
> > }
> > #else
> > #define foo(X) assert(__builtin_constant_p(X))
> > #endif
...
> #define inline		inline		__attribute__((always_inline)) notrace
>=20
> So in fact every inline function is marked always_inline all the time,
> which seems dubious.

I've had to do that in the past to get gcc to inline some small leaf functi=
ons
that were only called once.
(That was for some embedded code where I don't actually want any function c=
alls
at all.)

To my mind 'inline' should mean 'always_inline' since you need to explicitl=
y
stop functions being inlined even when not marked 'inline'.

	David

Re: BUILD_BUG_ON(!__builtin_constant_p(feature)) breaks bcc trace tool

[Arnd Bergmann]

On Wed, Jan 25, 2017 at 11:35 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Michael Ellerman

>> #define inline                inline          __attribute__((always_inline)) notrace
>>
>> So in fact every inline function is marked always_inline all the time,
>> which seems dubious.
>
> I've had to do that in the past to get gcc to inline some small leaf functions
> that were only called once.
> (That was for some embedded code where I don't actually want any function calls
> at all.)
>
> To my mind 'inline' should mean 'always_inline' since you need to explicitly
> stop functions being inlined even when not marked 'inline'.

On x86, this is configurable using OPTIMIZE_INLINING, but IIRC no
other architecture
supports it in mainline Linux. I have played a bit with enabling it on
ARM, which
showed a couple of build warnings and errors from the changed inlining, but
they were all fixable. I have not submitted that since I have not actually been
able to do extensive runtime testing on it.

It may turn out to be worthwhile for powerpc, where you have a much more limited
set of configurations you care about and you already do some regular testing.
Potentially this is more efficient than the current default, and any
function that
actually requires being forced inline can be annotated as __always_inline rather
than plain inline.

     Arnd

Re: BUILD_BUG_ON(!__builtin_constant_p(feature)) breaks bcc trace tool

[Michael Ellerman]

Anton Blanchard <anton@samba.org> writes:
> We added:
>
> BUILD_BUG_ON(!__builtin_constant_p(feature)) 
>
> to cpu_has_feature() and mmu_has_feature() in order to catch usage
> issues (such as cpu_has_feature(cpu_has_feature(X)). Unfortunately LLVM
> isn't smart enough to resolve this, and it errors out.
>
> I work around it in my clang/LLVM builds of the kernel, but I have just
> discovered that it causes a lot of issues for the bcc (eBPF) trace tool
> (which uses LLVM).

I didn't understand that part, but Aneesh explained to me that it's
because bcc pulls in the kernel-internal headers.

I guess as a quick fix we just have to #ifdef it, can you confirm this
works?


diff --git a/arch/powerpc/include/asm/cpu_has_feature.h b/arch/powerpc/include/asm/cpu_has_feature.h
index b312b152461b..6e834caa3720 100644
--- a/arch/powerpc/include/asm/cpu_has_feature.h
+++ b/arch/powerpc/include/asm/cpu_has_feature.h
@@ -23,7 +23,9 @@ static __always_inline bool cpu_has_feature(unsigned long feature)
 {
 	int i;
 
+#ifndef __clang__ /* clang can't cope with this */
 	BUILD_BUG_ON(!__builtin_constant_p(feature));
+#endif
 
 #ifdef CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG
 	if (!static_key_initialized) {
diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
index a34c764ca8dd..233a7e8cc8e3 100644
--- a/arch/powerpc/include/asm/mmu.h
+++ b/arch/powerpc/include/asm/mmu.h
@@ -160,7 +160,9 @@ static __always_inline bool mmu_has_feature(unsigned long feature)
 {
 	int i;
 
+#ifndef __clang__ /* clang can't cope with this */
 	BUILD_BUG_ON(!__builtin_constant_p(feature));
+#endif
 
 #ifdef CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG
 	if (!static_key_initialized) {


cheers

Re: BUILD_BUG_ON(!__builtin_constant_p(feature)) breaks bcc trace tool

[Naveen N. Rao]

On 2017/01/24 04:36PM, Michael Ellerman wrote:
> Anton Blanchard <anton@samba.org> writes:
> > We added:
> >
> > BUILD_BUG_ON(!__builtin_constant_p(feature)) 
> >
> > to cpu_has_feature() and mmu_has_feature() in order to catch usage
> > issues (such as cpu_has_feature(cpu_has_feature(X)). Unfortunately LLVM
> > isn't smart enough to resolve this, and it errors out.
> >
> > I work around it in my clang/LLVM builds of the kernel, but I have just
> > discovered that it causes a lot of issues for the bcc (eBPF) trace tool
> > (which uses LLVM).
> 
> I didn't understand that part, but Aneesh explained to me that it's
> because bcc pulls in the kernel-internal headers.
> 
> I guess as a quick fix we just have to #ifdef it, can you confirm this
> works?

This works and solves the issue for me. In the absence of a better 
approach, can you please push this for v4.10?

Tested-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Thanks,
Naveen

> 
> 
> diff --git a/arch/powerpc/include/asm/cpu_has_feature.h b/arch/powerpc/include/asm/cpu_has_feature.h
> index b312b152461b..6e834caa3720 100644
> --- a/arch/powerpc/include/asm/cpu_has_feature.h
> +++ b/arch/powerpc/include/asm/cpu_has_feature.h
> @@ -23,7 +23,9 @@ static __always_inline bool cpu_has_feature(unsigned long feature)
>  {
>  	int i;
> 
> +#ifndef __clang__ /* clang can't cope with this */
>  	BUILD_BUG_ON(!__builtin_constant_p(feature));
> +#endif
> 
>  #ifdef CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG
>  	if (!static_key_initialized) {
> diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
> index a34c764ca8dd..233a7e8cc8e3 100644
> --- a/arch/powerpc/include/asm/mmu.h
> +++ b/arch/powerpc/include/asm/mmu.h
> @@ -160,7 +160,9 @@ static __always_inline bool mmu_has_feature(unsigned long feature)
>  {
>  	int i;
> 
> +#ifndef __clang__ /* clang can't cope with this */
>  	BUILD_BUG_ON(!__builtin_constant_p(feature));
> +#endif
> 
>  #ifdef CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG
>  	if (!static_key_initialized) {
> 
> 
> cheers
>