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@xxxxxxxxxx> wrote:
>> On Tue, Sep 12, 2017 at 5:18 PM, Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx> 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.