[cfe-commits] [PATCH] Fixing include order for Windows

[Aaron Ballman]

On Sun, Feb 19, 2012 at 5:18 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Sun, Feb 19, 2012 at 3:09 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Sun, Feb 19, 2012 at 4:45 PM, Eli Friedman <eli.friedman at gmail.com>
>> wrote:
>> > On Sun, Feb 19, 2012 at 10:28 AM, Aaron Ballman <aaron at aaronballman.com>
>> > wrote:
>> >> On Windows, the default search paths for includes would put the
>> >> CMake-found headers (<build dir>\lib\clang\3.1\include) before the OS
>> >> headers.  For the most part, this is fine.  However, there are some
>> >> header files that share names with the system headers, which are
>> >> incorrect.  For instance, immintrin.h is completely different from the
>> >> SDK include.  This causes miscompilations in surprising places because
>> >> a system header will attempt to include one of these duplicates, and
>> >> find the clang header instead of the other system header.
>> >>
>> >> A simple test case of: #include <map> when building from MSVC
>> >> demonstrates the issue.
>> >>
>> >> A user can work around this behavior by specifying -nobuiltininc on
>> >> the command line, but that seems rather obtuse for such normal use
>> >> cases.
>> >>
>> >> I've fixed this by changing the order that the headers are included
>> >> from the toolchain.  The SDK headers get added *before* the clang
>> >> headers, and this resolves the issue.
>> >>
>> >> This patch does cause one regression test to fail
>> >> (Modules\compiler_builtins.m).  This is because the platform SDK
>> >> intrinsics are differently structured from the ones that come with
>> >> clang.  The OS APIs expect the SDK intrinsics, but our tests seem to
>> >> require our own intrinsics.  I'm open to suggestions.
>> >
>> > If on Windows, we specifically need the OS version of immintrin.h, we
>> > should modify our version to forward to it; completely ignoring the
>> > builtin headers is a bad idea (for example,
>> > http://llvm.org/bugs/show_bug.cgi?id=11918 is a miscompile caused by
>> > pulling in the wrong version of stdarg.h).
>>
>> The patch doesn't completely ignore the builtin headers though -- it
>> simply prefers the OS headers over the builtin headers.
>
>
> That doesn't work in the general case though -- look at all the places in
> our builtin headers where we specifically forward *to* the system headers.
> If the builtin doesn't come first, lots of things go wrong here.
>
> We should make our builtins always come first, and always be correct when
> they come first.
>
>> That being
>> said, I am not against forwarding our version.  What would be a good
>> approach to it?
>
>
> Look at the other builtin headers that use '__has_include_next' and
> 'include_next' to fold together a Clang builtin header and a system builtin
> header.
>
> I suspect you will have to essentially tack on the system 'immintrin.h'
> header to the end of the builtin, as we do expect #include <immintrin.h> to
> pull in the Clang builtin bits... Essentially, this doesn't seem like it
> should be either-or, it should be both.

So how should I handle the duplicate declarations?  For instance:

// mmintrin.h from MSVC
typedef union __declspec(intrin_type) _CRT_ALIGN(8) __m64
{
    unsigned __int64    m64_u64;
    float               m64_f32[2];
    __int8              m64_i8[8];
    __int16             m64_i16[4];
    __int32             m64_i32[2];
    __int64             m64_i64;
    unsigned __int8     m64_u8[8];
    unsigned __int16    m64_u16[4];
    unsigned __int32    m64_u32[2];
} __m64;

void  _m_empty(void);
#define _mm_empty         _m_empty

// mmintrin.h from clang
typedef long long __m64 __attribute__((__vector_size__(8)));

static __inline__ void __attribute__((__always_inline__, __nodebug__))
_mm_empty(void)
{
    __builtin_ia32_emms();
}
#define _m_empty _mm_empty

It seems to me that trying to keep *both* headers is going to be
rather challenging given that both make the same typedefs in
incompatible ways, the declarations/macros are opposite one another,
etc.

~Aaron