Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752309AbdIMK6h (ORCPT ); Wed, 13 Sep 2017 06:58:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41194 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751148AbdIMK6c (ORCPT ); Wed, 13 Sep 2017 06:58:32 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7B3547EA81 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=rkrcmar@redhat.com Date: Wed, 13 Sep 2017 12:58:26 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Dmitry Vyukov Cc: David Hildenbrand , Paolo Bonzini , LKML , KVM list , llvmlinux@lists.linuxfoundation.org, Alexander Potapenko , andreyknvl , Michael Davidson , Greg Hackmann , Nick Desaulniers Subject: Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang Message-ID: <20170913105826.GA18006@flask> References: <20170912151851.GA24313@flask> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Wed, 13 Sep 2017 10:58:32 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4041 Lines: 92 2017-09-12 17:54+0200, Dmitry Vyukov: > On Tue, Sep 12, 2017 at 5:51 PM, Dmitry Vyukov wrote: >> On Tue, Sep 12, 2017 at 5:18 PM, Radim Krčmář 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.