<div class="gmail_quote">On Sun, Feb 19, 2012 at 3:09 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Sun, Feb 19, 2012 at 4:45 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>> wrote:<br>
> On Sun, Feb 19, 2012 at 10:28 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
>> On Windows, the default search paths for includes would put the<br>
>> CMake-found headers (<build dir>\lib\clang\3.1\include) before the OS<br>
>> headers.  For the most part, this is fine.  However, there are some<br>
>> header files that share names with the system headers, which are<br>
>> incorrect.  For instance, immintrin.h is completely different from the<br>
>> SDK include.  This causes miscompilations in surprising places because<br>
>> a system header will attempt to include one of these duplicates, and<br>
>> find the clang header instead of the other system header.<br>
>><br>
>> A simple test case of: #include <map> when building from MSVC<br>
>> demonstrates the issue.<br>
>><br>
>> A user can work around this behavior by specifying -nobuiltininc on<br>
>> the command line, but that seems rather obtuse for such normal use<br>
>> cases.<br>
>><br>
>> I've fixed this by changing the order that the headers are included<br>
>> from the toolchain.  The SDK headers get added *before* the clang<br>
>> headers, and this resolves the issue.<br>
>><br>
>> This patch does cause one regression test to fail<br>
>> (Modules\compiler_builtins.m).  This is because the platform SDK<br>
>> intrinsics are differently structured from the ones that come with<br>
>> clang.  The OS APIs expect the SDK intrinsics, but our tests seem to<br>
>> require our own intrinsics.  I'm open to suggestions.<br>
><br>
> If on Windows, we specifically need the OS version of immintrin.h, we<br>
> should modify our version to forward to it; completely ignoring the<br>
> builtin headers is a bad idea (for example,<br>
> <a href="http://llvm.org/bugs/show_bug.cgi?id=11918" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=11918</a> is a miscompile caused by<br>
> pulling in the wrong version of stdarg.h).<br>
<br>
</div></div>The patch doesn't completely ignore the builtin headers though -- it<br>
simply prefers the OS headers over the builtin headers.</blockquote><div><br></div><div>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.</div>
<div><br></div><div>We should make our builtins always come first, and always be correct when they come first.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
That being<br>
said, I am not against forwarding our version.  What would be a good<br>
approach to it?<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
</div>