Date:   Tue, 12 Sep 2017 19:33:38 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Radim Krčmář <rkrcmar@...hat.com>,
        David Hildenbrand <david@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        KVM list <kvm@...r.kernel.org>,
        llvmlinux@...ts.linuxfoundation.org,
        Alexander Potapenko <glider@...gle.com>,
        andreyknvl <andreyknvl@...gle.com>,
        Michael Davidson <md@...gle.com>,
        Greg Hackmann <ghackmann@...gle.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang

On Tue, Sep 12, 2017 at 6:33 PM, Paolo Bonzini <pbonzini@...hat.com> wrote:
> On 12/09/2017 18:16, Dmitry Vyukov wrote:
>> On Tue, Sep 12, 2017 at 6:03 PM, Paolo Bonzini <pbonzini@...hat.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

Powered by blists - more mailing lists