DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2886981E0C
Date: Tue, 12 Sep 2017 17:18:52 +0200
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: Dmitry Vyukov <dvyukov@google.com>
Cc: David Hildenbrand <david@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>, KVM list <kvm@vger.kernel.org>,
        llvmlinux@lists.linuxfoundation.org,
        Alexander Potapenko <glider@google.com>,
        andreyknvl <andreyknvl@google.com>, Michael Davidson <md@google.com>,
        Greg Hackmann <ghackmann@google.com>,
        Nick Desaulniers <ndesaulniers@google.com>
Subject: Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang
Message-ID: <20170912151851.GA24313@flask>
References: <CACT4Y+boDPGU_02AhzUCCHfHdPaviMd5sTJTuV5RuYNor4wRpg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CACT4Y+boDPGU_02AhzUCCHfHdPaviMd5sTJTuV5RuYNor4wRpg@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1439
Lines: 42

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.