"KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang

"KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang

[Dmitry Vyukov]

Hi Radim,

I've just noticed that your commit "KVM: x86: generalize
guest_cpuid_has_ helpers" breaks clang build on this assert:

static __always_inline struct cpuid_reg x86_feature_cpuid(unsigned x86_feature)
{
    unsigned x86_leaf = x86_feature / 32;

    BUILD_BUG_ON(!__builtin_constant_p(x86_leaf));


In clang __builtin_constant_p is never true for function arguments,
it's true only for compile-time constants (what you can use as stack
array size, or C++ template argument). What would work is an
additional macro along the lines of:

#define x86_feature_cpuid(x) (BUILD_BUG_ON(!__builtin_constant_p(x),
__x86_feature_cpuid(x))

But again assuming that caller pass the constant directly.

Could you please fix it?

Thanks

Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang

[Radim Krčmář]

2017-09-12 16:42+0200, Dmitry Vyukov:
> Hi Radim,
> 
> I've just noticed that your commit "KVM: x86: generalize
> guest_cpuid_has_ helpers" breaks clang build on this assert:
> 
> static __always_inline struct cpuid_reg x86_feature_cpuid(unsigned x86_feature)
> {
>     unsigned x86_leaf = x86_feature / 32;
> 
>     BUILD_BUG_ON(!__builtin_constant_p(x86_leaf));
> 
> 
> In clang __builtin_constant_p is never true for function arguments,
> it's true only for compile-time constants (what you can use as stack
> array size, or C++ template argument). What would work is an
> additional macro along the lines of:

GCC optimizes it thanks to __always_inline, so the x86_feature is
constant in each instance of this function ... the goal is to have
compile-time input checking.

> #define x86_feature_cpuid(x) (BUILD_BUG_ON(!__builtin_constant_p(x),
> __x86_feature_cpuid(x))
> 
> But again assuming that caller pass the constant directly.

The __builtin_constant_p() check is just a canary, the important ones
are

  BUILD_BUG_ON(x86_leaf >= ARRAY_SIZE(reverse_cpuid));
  BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);

and these would be very awkward if moved out of the function.

> Could you please fix it?

Sure, I can just make them BUG_ON (or WARN_ON with error handling), but
I tried with clang version 4.0.1 and got no errors -- are you using an
older version?  (or a command other than `make HOSTCC=clang CC=clang`)

Thanks.

Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang

[Dmitry Vyukov]

On Tue, Sep 12, 2017 at 5:18 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> 2017-09-12 16:42+0200, Dmitry Vyukov:
>> Hi Radim,
>>
>> I've just noticed that your commit "KVM: x86: generalize
>> guest_cpuid_has_ helpers" breaks clang build on this assert:
>>
>> static __always_inline struct cpuid_reg x86_feature_cpuid(unsigned x86_feature)
>> {
>>     unsigned x86_leaf = x86_feature / 32;
>>
>>     BUILD_BUG_ON(!__builtin_constant_p(x86_leaf));
>>
>>
>> In clang __builtin_constant_p is never true for function arguments,
>> it's true only for compile-time constants (what you can use as stack
>> array size, or C++ template argument). What would work is an
>> additional macro along the lines of:
>
> GCC optimizes it thanks to __always_inline, so the x86_feature is
> constant in each instance of this function ... the goal is to have
> compile-time input checking.
>
>> #define x86_feature_cpuid(x) (BUILD_BUG_ON(!__builtin_constant_p(x),
>> __x86_feature_cpuid(x))
>>
>> But again assuming that caller pass the constant directly.
>
> The __builtin_constant_p() check is just a canary, the important ones
> are
>
>   BUILD_BUG_ON(x86_leaf >= ARRAY_SIZE(reverse_cpuid));
>   BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);
>
> and these would be very awkward if moved out of the function.
>
>> Could you please fix it?
>
> Sure, I can just make them BUG_ON (or WARN_ON with error handling), but
> I tried with clang version 4.0.1 and got no errors -- are you using an
> older version?  (or a command other than `make HOSTCC=clang CC=clang`)


Interesting.

I use clang version 6.0.0 (trunk 313027).

I am on 8fac2f96ab86b0e14ec4e42851e21e9b518bdc55 on Linus tree. Here
is my config (which is basically defconfig + kvm enabled):
https://gist.githubusercontent.com/dvyukov/4360060ab49374b1e983312f587f1b4e/raw/2e4def6f4318bde81ab316546400513b02673bc9/gistfile1.txt
Build with: make CC=/build/bin/clang
and get:

  MODPOST vmlinux.o
arch/x86/kvm/x86.o: In function `kvm_set_apic_base':
x86.c:(.text+0x4738): undefined reference to `__compiletime_assert_62'
arch/x86/kvm/x86.o: In function `kvm_set_cr4':
x86.c:(.text+0x54ae): undefined reference to `__compiletime_assert_62'
x86.c:(.text+0x54dc): undefined reference to `__compiletime_assert_62'
x86.c:(.text+0x5509): undefined reference to `__compiletime_assert_62'
x86.c:(.text+0x5536): undefined reference to `__compiletime_assert_62'
arch/x86/kvm/x86.o:x86.c:(.text+0x5563): more undefined references to
`__compiletime_assert_62' follow
make: *** [vmlinux] Error 1


I've commented out the first BUILD_BUG_ON, and these did not cause build errors:

  BUILD_BUG_ON(x86_leaf >= ARRAY_SIZE(reverse_cpuid));
  BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);

I guess clang still eliminates dead branches. Clang optimizer does
know that these are constant, it just does not allow build
success/failure nor runtime behavior depend on optimization level and
compiler version. I.e. with gcc you can get build failure with only
some compiler flags and/or compiler versions. Clang gives stable
result. But the optimizer does use constant propagation, etc during
optimization.

Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang

[Dmitry Vyukov]

On Tue, Sep 12, 2017 at 5:51 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Sep 12, 2017 at 5:18 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
>> 2017-09-12 16:42+0200, Dmitry Vyukov:
>>> Hi Radim,
>>>
>>> I've just noticed that your commit "KVM: x86: generalize
>>> guest_cpuid_has_ helpers" breaks clang build on this assert:
>>>
>>> static __always_inline struct cpuid_reg x86_feature_cpuid(unsigned x86_feature)
>>> {
>>>     unsigned x86_leaf = x86_feature / 32;
>>>
>>>     BUILD_BUG_ON(!__builtin_constant_p(x86_leaf));
>>>
>>>
>>> In clang __builtin_constant_p is never true for function arguments,
>>> it's true only for compile-time constants (what you can use as stack
>>> array size, or C++ template argument). What would work is an
>>> additional macro along the lines of:
>>
>> GCC optimizes it thanks to __always_inline, so the x86_feature is
>> constant in each instance of this function ... the goal is to have
>> compile-time input checking.
>>
>>> #define x86_feature_cpuid(x) (BUILD_BUG_ON(!__builtin_constant_p(x),
>>> __x86_feature_cpuid(x))
>>>
>>> But again assuming that caller pass the constant directly.
>>
>> The __builtin_constant_p() check is just a canary, the important ones
>> are
>>
>>   BUILD_BUG_ON(x86_leaf >= ARRAY_SIZE(reverse_cpuid));
>>   BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);
>>
>> and these would be very awkward if moved out of the function.
>>
>>> Could you please fix it?
>>
>> Sure, I can just make them BUG_ON (or WARN_ON with error handling), but
>> I tried with clang version 4.0.1 and got no errors -- are you using an
>> older version?  (or a command other than `make HOSTCC=clang CC=clang`)
>
>
> Interesting.
>
> I use clang version 6.0.0 (trunk 313027).
>
> I am on 8fac2f96ab86b0e14ec4e42851e21e9b518bdc55 on Linus tree. Here
> is my config (which is basically defconfig + kvm enabled):
> https://gist.githubusercontent.com/dvyukov/4360060ab49374b1e983312f587f1b4e/raw/2e4def6f4318bde81ab316546400513b02673bc9/gistfile1.txt
> Build with: make CC=/build/bin/clang
> and get:
>
>   MODPOST vmlinux.o
> arch/x86/kvm/x86.o: In function `kvm_set_apic_base':
> x86.c:(.text+0x4738): undefined reference to `__compiletime_assert_62'
> arch/x86/kvm/x86.o: In function `kvm_set_cr4':
> x86.c:(.text+0x54ae): undefined reference to `__compiletime_assert_62'
> x86.c:(.text+0x54dc): undefined reference to `__compiletime_assert_62'
> x86.c:(.text+0x5509): undefined reference to `__compiletime_assert_62'
> x86.c:(.text+0x5536): undefined reference to `__compiletime_assert_62'
> arch/x86/kvm/x86.o:x86.c:(.text+0x5563): more undefined references to
> `__compiletime_assert_62' follow
> make: *** [vmlinux] Error 1
>
>
> I've commented out the first BUILD_BUG_ON, and these did not cause build errors:
>
>   BUILD_BUG_ON(x86_leaf >= ARRAY_SIZE(reverse_cpuid));
>   BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);
>
> I guess clang still eliminates dead branches. Clang optimizer does
> know that these are constant, it just does not allow build
> success/failure nor runtime behavior depend on optimization level and
> compiler version. I.e. with gcc you can get build failure with only
> some compiler flags and/or compiler versions. Clang gives stable
> result. But the optimizer does use constant propagation, etc during
> optimization.


I've installed clang-3.9 (the closest version to yours my distribution
gives me) and still got the same error with it. I would expect that
4.0 should give the same result as well... Are you sure you enabled
KVM/intel/amd? (yes, I know you are maintaining KVM code :))

Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang

[Paolo Bonzini]

On 12/09/2017 17:54, Dmitry Vyukov wrote:
>> I guess clang still eliminates dead branches. Clang optimizer does
>> know that these are constant, it just does not allow build
>> success/failure nor runtime behavior depend on optimization level and
>> compiler version. I.e. with gcc you can get build failure with only
>> some compiler flags and/or compiler versions. Clang gives stable
>> result. But the optimizer does use constant propagation, etc during
>> optimization.

I can reproduce it:

$ cat f.c
int bad_code();

static inline void __attribute__((always_inline)) f(int x)
{
        if (!__builtin_constant_p(x))
                bad_code();
}

int main()
{
        f(0);
        f(1);
        f(100);
}

$ clang --version
clang version 4.0.0 (tags/RELEASE_400/final)
$ clang f.c -O2 -c -o f.o
$ nm f.o
                 U bad_code
0000000000000000 T main

$ gcc f.c -O2 -c -o f.o
$ nm f.o
0000000000000000 T main

... but I don't know, it seems very weird.  The purpose of
__builtin_constant_p is to be resolved only relatively late in the
optimization pipeline, and it has been like this for at least 15 years
in GCC.

The docs say what to expect:

  You may use this built-in function in either a macro or an inline
  function. However, if you use it in an inlined function and pass an
  argument of the function as the argument to the built-in, GCC never
  returns 1 when you call the inline function with a string constant or
  compound literal (see Compound Literals) and does not return 1 when
  you pass a constant numeric value to the inline function **unless you
  specify the -O option**.

(emphasis mine).

> I've installed clang-3.9 (the closest version to yours my distribution
> gives me) and still got the same error with it. I would expect that
> 4.0 should give the same result as well... Are you sure you enabled
> KVM/intel/amd? (yes, I know you are maintaining KVM code :))

Heh, I don't know about Radim but "^Rmake" goes straight to "make
arch/x86/kvm/" for me. :)

Paolo

Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang

[Dmitry Vyukov]

On Tue, Sep 12, 2017 at 6:03 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 12/09/2017 17:54, Dmitry Vyukov wrote:
>>> I guess clang still eliminates dead branches. Clang optimizer does
>>> know that these are constant, it just does not allow build
>>> success/failure nor runtime behavior depend on optimization level and
>>> compiler version. I.e. with gcc you can get build failure with only
>>> some compiler flags and/or compiler versions. Clang gives stable
>>> result. But the optimizer does use constant propagation, etc during
>>> optimization.
>
> I can reproduce it:
>
> $ cat f.c
> int bad_code();
>
> static inline void __attribute__((always_inline)) f(int x)
> {
>         if (!__builtin_constant_p(x))
>                 bad_code();
> }
>
> int main()
> {
>         f(0);
>         f(1);
>         f(100);
> }
>
> $ clang --version
> clang version 4.0.0 (tags/RELEASE_400/final)
> $ clang f.c -O2 -c -o f.o
> $ nm f.o
>                  U bad_code
> 0000000000000000 T main
>
> $ gcc f.c -O2 -c -o f.o
> $ nm f.o
> 0000000000000000 T main
>
> ... but I don't know, it seems very weird.  The purpose of
> __builtin_constant_p is to be resolved only relatively late in the
> optimization pipeline, and it has been like this for at least 15 years
> in GCC.
>
> The docs say what to expect:
>
>   You may use this built-in function in either a macro or an inline
>   function. However, if you use it in an inlined function and pass an
>   argument of the function as the argument to the built-in, GCC never
>   returns 1 when you call the inline function with a string constant or
>   compound literal (see Compound Literals) and does not return 1 when
>   you pass a constant numeric value to the inline function **unless you
>   specify the -O option**.
>
> (emphasis mine).


Yes, I know. This difference was surprising for me and lots of other
people as well. But this is a fundamental position for clang and is
related to some implementation choices. Namely, C/C++ frontend needs
to know values of compile-time const expressions in order to verify
correctness and generate middle-end representation. But for gcc's
meaning of __builtin_constant_p, its value only becomes known deep
inside of middle-end. Which kinda creates a cycle. In gcc it's all
somehow mixed together (front-end/middle-end) and somehow works. Can't
possibly work for clang with strict separation between front-end and
middle-end.

I proposed to introduce another builtin that returns a value that is
constant from optimizer point of view (e.g. it can eliminate dead code
on branches), but is not constant from language/front-end point of
view (e.g. you can't declare a stack array using the value as size).
It should do in such cases and should be implementable in clang. But
we don't have it yet, and again it's not __builtin_constant_p, because
gcc's __builtin_constant_p returns a compile-time constant. So we need
to deal with this situation somehow.


>> I've installed clang-3.9 (the closest version to yours my distribution
>> gives me) and still got the same error with it. I would expect that
>> 4.0 should give the same result as well... Are you sure you enabled
>> KVM/intel/amd? (yes, I know you are maintaining KVM code :))
>
> Heh, I don't know about Radim but "^Rmake" goes straight to "make
> arch/x86/kvm/" for me. :)
>
> Paolo

Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang

[Paolo Bonzini]

On 12/09/2017 18:16, Dmitry Vyukov wrote:
> On Tue, Sep 12, 2017 at 6:03 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 12/09/2017 17:54, Dmitry Vyukov wrote:
>>>> I guess clang still eliminates dead branches. Clang optimizer does
>>>> know that these are constant, it just does not allow build
>>>> success/failure nor runtime behavior depend on optimization level and
>>>> compiler version. I.e. with gcc you can get build failure with only
>>>> some compiler flags and/or compiler versions. Clang gives stable
>>>> result. But the optimizer does use constant propagation, etc during
>>>> optimization.
>>
>> I can reproduce it:
>>
>> $ cat f.c
>> int bad_code();
>>
>> static inline void __attribute__((always_inline)) f(int x)
>> {
>>         if (!__builtin_constant_p(x))
>>                 bad_code();
>> }
>>
>> int main()
>> {
>>         f(0);
>>         f(1);
>>         f(100);
>> }
>>
>> $ clang --version
>> clang version 4.0.0 (tags/RELEASE_400/final)
>> $ clang f.c -O2 -c -o f.o
>> $ nm f.o
>>                  U bad_code
>> 0000000000000000 T main
>>
>> $ gcc f.c -O2 -c -o f.o
>> $ nm f.o
>> 0000000000000000 T main
>>
>> ... but I don't know, it seems very weird.  The purpose of
>> __builtin_constant_p is to be resolved only relatively late in the
>> optimization pipeline, and it has been like this for at least 15 years
>> in GCC.
>>
>> The docs say what to expect:
>>
>>   You may use this built-in function in either a macro or an inline
>>   function. However, if you use it in an inlined function and pass an
>>   argument of the function as the argument to the built-in, GCC never
>>   returns 1 when you call the inline function with a string constant or
>>   compound literal (see Compound Literals) and does not return 1 when
>>   you pass a constant numeric value to the inline function **unless you
>>   specify the -O option**.
>>
>> (emphasis mine).
> 
> 
> Yes, I know. This difference was surprising for me and lots of other
> people as well. But this is a fundamental position for clang and is
> related to some implementation choices. Namely, C/C++ frontend needs
> to know values of compile-time const expressions in order to verify
> correctness and generate middle-end representation. But for gcc's
> meaning of __builtin_constant_p, its value only becomes known deep
> inside of middle-end. Which kinda creates a cycle. In gcc it's all
> somehow mixed together (front-end/middle-end) and somehow works. Can't
> possibly work for clang with strict separation between front-end and
> middle-end.

This is nonsense, GCC is also separating front-end and middle-end.  The
front-end only ever produces a 0 value for __builtin_constant_p if an
integer constant expression is syntactically required.

When entering the middle-end a __builtin_constant_p with non-constant
argument is lowered to a builtin function when optimization is on, or 0
when optimization is off.

The middle-end knows about __builtin_constant_p and can fold it to 1
when the argument is a constant.  At some point, GCC decides it's had
enough and changes all remaining calls to return 0.  There's no reason
why LLVM couldn't have such a builtin.

> I proposed to introduce another builtin that returns a value that is
> constant from optimizer point of view (e.g. it can eliminate dead code
> on branches), but is not constant from language/front-end point of
> view (e.g. you can't declare a stack array using the value as size).
> It should do in such cases and should be implementable in clang. But
> we don't have it yet, and again it's not __builtin_constant_p, because
> gcc's __builtin_constant_p returns a compile-time constant.

I think this has to be fixed at the include/linux/ level.  I'm okay with
warning instead of erroring, so maybe add WARN_IF_NONCONSTANT() and make
it do nothing (or live with the warning) on clang?

Paolo

Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang

[Dmitry Vyukov]

On Tue, Sep 12, 2017 at 6:33 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 12/09/2017 18:16, Dmitry Vyukov wrote:
>> On Tue, Sep 12, 2017 at 6:03 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> On 12/09/2017 17:54, Dmitry Vyukov wrote:
>>>>> I guess clang still eliminates dead branches. Clang optimizer does
>>>>> know that these are constant, it just does not allow build
>>>>> success/failure nor runtime behavior depend on optimization level and
>>>>> compiler version. I.e. with gcc you can get build failure with only
>>>>> some compiler flags and/or compiler versions. Clang gives stable
>>>>> result. But the optimizer does use constant propagation, etc during
>>>>> optimization.
>>>
>>> I can reproduce it:
>>>
>>> $ cat f.c
>>> int bad_code();
>>>
>>> static inline void __attribute__((always_inline)) f(int x)
>>> {
>>>         if (!__builtin_constant_p(x))
>>>                 bad_code();
>>> }
>>>
>>> int main()
>>> {
>>>         f(0);
>>>         f(1);
>>>         f(100);
>>> }
>>>
>>> $ clang --version
>>> clang version 4.0.0 (tags/RELEASE_400/final)
>>> $ clang f.c -O2 -c -o f.o
>>> $ nm f.o
>>>                  U bad_code
>>> 0000000000000000 T main
>>>
>>> $ gcc f.c -O2 -c -o f.o
>>> $ nm f.o
>>> 0000000000000000 T main
>>>
>>> ... but I don't know, it seems very weird.  The purpose of
>>> __builtin_constant_p is to be resolved only relatively late in the
>>> optimization pipeline, and it has been like this for at least 15 years
>>> in GCC.
>>>
>>> The docs say what to expect:
>>>
>>>   You may use this built-in function in either a macro or an inline
>>>   function. However, if you use it in an inlined function and pass an
>>>   argument of the function as the argument to the built-in, GCC never
>>>   returns 1 when you call the inline function with a string constant or
>>>   compound literal (see Compound Literals) and does not return 1 when
>>>   you pass a constant numeric value to the inline function **unless you
>>>   specify the -O option**.
>>>
>>> (emphasis mine).
>>
>>
>> Yes, I know. This difference was surprising for me and lots of other
>> people as well. But this is a fundamental position for clang and is
>> related to some implementation choices. Namely, C/C++ frontend needs
>> to know values of compile-time const expressions in order to verify
>> correctness and generate middle-end representation. But for gcc's
>> meaning of __builtin_constant_p, its value only becomes known deep
>> inside of middle-end. Which kinda creates a cycle. In gcc it's all
>> somehow mixed together (front-end/middle-end) and somehow works. Can't
>> possibly work for clang with strict separation between front-end and
>> middle-end.
>
> This is nonsense, GCC is also separating front-end and middle-end.  The
> front-end only ever produces a 0 value for __builtin_constant_p if an
> integer constant expression is syntactically required.
>
> When entering the middle-end a __builtin_constant_p with non-constant
> argument is lowered to a builtin function when optimization is on, or 0
> when optimization is off.
>
> The middle-end knows about __builtin_constant_p and can fold it to 1
> when the argument is a constant.  At some point, GCC decides it's had
> enough and changes all remaining calls to return 0.  There's no reason
> why LLVM couldn't have such a builtin.


That's still build breakages/behavior differences depending on
optimization level and compiler version. Also these funny effects:

#include <stdio.h>

void f1(int x)
{
  char a[__builtin_constant_p(x) ? 2 : 1];
  printf("%d\n", (int)sizeof(a));
}

void f2(int x)
{
  const int y = __builtin_constant_p(x) ? 2 : 1;
  char a[y];
  printf("%d\n", (int)sizeof(a));
}

void f3(int x)
{
  char a[__builtin_constant_p(x) ? 2 : 1];
  printf("%d %d\n", (int)sizeof(a), (int)__builtin_constant_p(x) ? 2 : 1);
}

void f4(int x)
{
  const int y = __builtin_constant_p(x) ? 2 : 1;
  char a[y];
  printf("%d %d\n", (int)sizeof(a), y);
}

int main()
{
  f1(1);
  f2(1);
  f3(1);
  f4(1);
  return 0;
}


$ clang-3.9 /tmp/test.c -O2 && ./a.out
1
1
1 1
1 1

$ gcc /tmp/test.c -O2 && ./a.out
2
2
1 1
2 2

Print value -- different behavior.
Print value but assign to a temp -- again different behavior.

There are merits for not doing this.

But I admit clang effectively breaks most practical uses of
__builtin_constant_p.


>> I proposed to introduce another builtin that returns a value that is
>> constant from optimizer point of view (e.g. it can eliminate dead code
>> on branches), but is not constant from language/front-end point of
>> view (e.g. you can't declare a stack array using the value as size).
>> It should do in such cases and should be implementable in clang. But
>> we don't have it yet, and again it's not __builtin_constant_p, because
>> gcc's __builtin_constant_p returns a compile-time constant.
>
> I think this has to be fixed at the include/linux/ level.  I'm okay with
> warning instead of erroring, so maybe add WARN_IF_NONCONSTANT() and make
> it do nothing (or live with the warning) on clang?
>
> Paolo

Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang

[Radim Krčmář]

2017-09-12 17:54+0200, Dmitry Vyukov:
> On Tue, Sep 12, 2017 at 5:51 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Tue, Sep 12, 2017 at 5:18 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
>>> 2017-09-12 16:42+0200, Dmitry Vyukov:
>>>> Hi Radim,
>>>>
>>>> I've just noticed that your commit "KVM: x86: generalize
>>>> guest_cpuid_has_ helpers" breaks clang build on this assert:
>>>>
>>>> static __always_inline struct cpuid_reg x86_feature_cpuid(unsigned x86_feature)
>>>> {
>>>>     unsigned x86_leaf = x86_feature / 32;
>>>>
>>>>     BUILD_BUG_ON(!__builtin_constant_p(x86_leaf));
>>>>
>>>>
>>>> In clang __builtin_constant_p is never true for function arguments,
>>>> it's true only for compile-time constants (what you can use as stack
>>>> array size, or C++ template argument). What would work is an
>>>> additional macro along the lines of:
>>>
>>> GCC optimizes it thanks to __always_inline, so the x86_feature is
>>> constant in each instance of this function ... the goal is to have
>>> compile-time input checking.
>>>
>>>> #define x86_feature_cpuid(x) (BUILD_BUG_ON(!__builtin_constant_p(x),
>>>> __x86_feature_cpuid(x))
>>>>
>>>> But again assuming that caller pass the constant directly.
>>>
>>> The __builtin_constant_p() check is just a canary, the important ones
>>> are
>>>
>>>   BUILD_BUG_ON(x86_leaf >= ARRAY_SIZE(reverse_cpuid));
>>>   BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);
>>>
>>> and these would be very awkward if moved out of the function.
>>>
>>>> Could you please fix it?
>>>
>>> Sure, I can just make them BUG_ON (or WARN_ON with error handling), but
>>> I tried with clang version 4.0.1 and got no errors -- are you using an
>>> older version?  (or a command other than `make HOSTCC=clang CC=clang`)
>>
>>
>> Interesting.
>>
>> I use clang version 6.0.0 (trunk 313027).
>>
>> I am on 8fac2f96ab86b0e14ec4e42851e21e9b518bdc55 on Linus tree. Here
>> is my config (which is basically defconfig + kvm enabled):
>> https://gist.githubusercontent.com/dvyukov/4360060ab49374b1e983312f587f1b4e/raw/2e4def6f4318bde81ab316546400513b02673bc9/gistfile1.txt
>> Build with: make CC=/build/bin/clang
>> and get:
>>
>>   MODPOST vmlinux.o
>> arch/x86/kvm/x86.o: In function `kvm_set_apic_base':
>> x86.c:(.text+0x4738): undefined reference to `__compiletime_assert_62'
>> arch/x86/kvm/x86.o: In function `kvm_set_cr4':
>> x86.c:(.text+0x54ae): undefined reference to `__compiletime_assert_62'
>> x86.c:(.text+0x54dc): undefined reference to `__compiletime_assert_62'
>> x86.c:(.text+0x5509): undefined reference to `__compiletime_assert_62'
>> x86.c:(.text+0x5536): undefined reference to `__compiletime_assert_62'
>> arch/x86/kvm/x86.o:x86.c:(.text+0x5563): more undefined references to
>> `__compiletime_assert_62' follow
>> make: *** [vmlinux] Error 1
>>
>>
>> I've commented out the first BUILD_BUG_ON, and these did not cause build errors:
>>
>>   BUILD_BUG_ON(x86_leaf >= ARRAY_SIZE(reverse_cpuid));
>>   BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);
>>
>> I guess clang still eliminates dead branches. Clang optimizer does
>> know that these are constant, it just does not allow build
>> success/failure nor runtime behavior depend on optimization level and
>> compiler version. I.e. with gcc you can get build failure with only
>> some compiler flags and/or compiler versions. Clang gives stable
>> result. But the optimizer does use constant propagation, etc during
>> optimization.

Good to know, removing the first check seems like the best option then.

> I've installed clang-3.9 (the closest version to yours my distribution
> gives me) and still got the same error with it. I would expect that
> 4.0 should give the same result as well... Are you sure you enabled
> KVM/intel/amd? (yes, I know you are maintaining KVM code :))

Well, I got warnings about KVM code and then several unrelated errors
about variable length array in structure (on torvalds/master), so I just
retried that M=arch/x86/kvm works and didn't get to the modpost phase.
I didn't realize it blows up late, sorry.