* "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang @ 2017-09-12 14:42 Dmitry Vyukov 2017-09-12 15:18 ` Radim Krčmář 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Vyukov @ 2017-09-12 14:42 UTC (permalink / raw) To: Radim Krčmář, David Hildenbrand, Paolo Bonzini, LKML, KVM list Cc: llvmlinux, Alexander Potapenko, andreyknvl, Michael Davidson, Greg Hackmann, Nick Desaulniers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang 2017-09-12 14:42 "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang Dmitry Vyukov @ 2017-09-12 15:18 ` Radim Krčmář 2017-09-12 15:51 ` Dmitry Vyukov 0 siblings, 1 reply; 9+ messages in thread From: Radim Krčmář @ 2017-09-12 15:18 UTC (permalink / raw) To: Dmitry Vyukov Cc: David Hildenbrand, Paolo Bonzini, LKML, KVM list, llvmlinux, Alexander Potapenko, andreyknvl, Michael Davidson, Greg Hackmann, Nick Desaulniers 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang 2017-09-12 15:18 ` Radim Krčmář @ 2017-09-12 15:51 ` Dmitry Vyukov 2017-09-12 15:54 ` Dmitry Vyukov 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Vyukov @ 2017-09-12 15:51 UTC (permalink / raw) To: Radim Krčmář Cc: David Hildenbrand, Paolo Bonzini, LKML, KVM list, llvmlinux, Alexander Potapenko, andreyknvl, Michael Davidson, Greg Hackmann, Nick Desaulniers 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang 2017-09-12 15:51 ` Dmitry Vyukov @ 2017-09-12 15:54 ` Dmitry Vyukov 2017-09-12 16:03 ` Paolo Bonzini 2017-09-13 10:58 ` Radim Krčmář 0 siblings, 2 replies; 9+ messages in thread From: Dmitry Vyukov @ 2017-09-12 15:54 UTC (permalink / raw) To: Radim Krčmář Cc: David Hildenbrand, Paolo Bonzini, LKML, KVM list, llvmlinux, Alexander Potapenko, andreyknvl, Michael Davidson, Greg Hackmann, Nick Desaulniers 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 :)) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang 2017-09-12 15:54 ` Dmitry Vyukov @ 2017-09-12 16:03 ` Paolo Bonzini 2017-09-12 16:16 ` Dmitry Vyukov 2017-09-13 10:58 ` Radim Krčmář 1 sibling, 1 reply; 9+ messages in thread From: Paolo Bonzini @ 2017-09-12 16:03 UTC (permalink / raw) To: Dmitry Vyukov, Radim Krčmář Cc: David Hildenbrand, LKML, KVM list, llvmlinux, Alexander Potapenko, andreyknvl, Michael Davidson, Greg Hackmann, Nick Desaulniers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang 2017-09-12 16:03 ` Paolo Bonzini @ 2017-09-12 16:16 ` Dmitry Vyukov 2017-09-12 16:33 ` Paolo Bonzini 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Vyukov @ 2017-09-12 16:16 UTC (permalink / raw) To: Paolo Bonzini Cc: Radim Krčmář, David Hildenbrand, LKML, KVM list, llvmlinux, Alexander Potapenko, andreyknvl, Michael Davidson, Greg Hackmann, Nick Desaulniers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang 2017-09-12 16:16 ` Dmitry Vyukov @ 2017-09-12 16:33 ` Paolo Bonzini 2017-09-12 17:33 ` Dmitry Vyukov 0 siblings, 1 reply; 9+ messages in thread From: Paolo Bonzini @ 2017-09-12 16:33 UTC (permalink / raw) To: Dmitry Vyukov Cc: Radim Krčmář, David Hildenbrand, LKML, KVM list, llvmlinux, Alexander Potapenko, andreyknvl, Michael Davidson, Greg Hackmann, Nick Desaulniers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang 2017-09-12 16:33 ` Paolo Bonzini @ 2017-09-12 17:33 ` Dmitry Vyukov 0 siblings, 0 replies; 9+ messages in thread From: Dmitry Vyukov @ 2017-09-12 17:33 UTC (permalink / raw) To: Paolo Bonzini Cc: Radim Krčmář, David Hildenbrand, LKML, KVM list, llvmlinux, Alexander Potapenko, andreyknvl, Michael Davidson, Greg Hackmann, Nick Desaulniers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang 2017-09-12 15:54 ` Dmitry Vyukov 2017-09-12 16:03 ` Paolo Bonzini @ 2017-09-13 10:58 ` Radim Krčmář 1 sibling, 0 replies; 9+ messages in thread From: Radim Krčmář @ 2017-09-13 10:58 UTC (permalink / raw) To: Dmitry Vyukov Cc: David Hildenbrand, Paolo Bonzini, LKML, KVM list, llvmlinux, Alexander Potapenko, andreyknvl, Michael Davidson, Greg Hackmann, Nick Desaulniers 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-09-13 10:58 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-09-12 14:42 "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang Dmitry Vyukov 2017-09-12 15:18 ` Radim Krčmář 2017-09-12 15:51 ` Dmitry Vyukov 2017-09-12 15:54 ` Dmitry Vyukov 2017-09-12 16:03 ` Paolo Bonzini 2017-09-12 16:16 ` Dmitry Vyukov 2017-09-12 16:33 ` Paolo Bonzini 2017-09-12 17:33 ` Dmitry Vyukov 2017-09-13 10:58 ` Radim Krčmář
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.