[cfe-dev] [llvm-testresults] Red bots at night, buildczar's delight?

[dalej]

On Nov 18, 2010, at 12:57 PM, Chandler Carruth wrote:

> On Thu, Nov 18, 2010 at 10:51 AM, dalej <dalej at apple.com> wrote:
> OK, I guess no one else wants to express an opinion on this, so i went with this one.  Real-world code may have some problems down the road, but nothing that can't be fixed.
> 
> Sadly, the world is not so simple it seems. Let me tell you a bitter tale of sorrow and tears.
> 
> We started deploying this internally, and found code that broke. That wasn't *too* surprising. Then we found out why. The code  was "correct". It included <stdlib.h> just like it should:
> 
> #include <pmmintrin.h>
> #include <stdlib.h>
> 
> or some such. Seems fine? It would be on a mac. But not on Linux. Oh no. Our fearless leaders have declared that they don't see any problem defining libc functions like this:
> 
> #ifdef __cplusplus__
> void *malloc(size_t size) throw();
> #else
> ...
> #endif
> 
> Yes, that's an exception specifier on a C malloc function. Apparently this optimizes something for GCC, but sadly it's utterly and completely non-conforming.
> 
> Now we get really interesting, because of course C allows (and many people do) just declaring the malloc function directly rather than adding a header file. That's great, but then you have to some how intuit whether or not *your* favorite standard library has broken their declaration in this way, or you get conflicting declarations of the function.
> 
> Ok, we can deal with this much. Clang has an egregious hack that will merge these specifiers and accept re-declarations missing them when the original declaration comes from within a system header. So we *should* be good. Look back up at the includes I mentioned, now back to me. The includes pull in mm_malloc.h first, then stdlib.h. So the first declaration doesn't have the specifier, and the second one does.

Hmmm, I guess this explains why the testcase changes I checked in didn't actually fix anything...

> I have no idea what the best way to solve this is. We could extend the egregious hack to allow the reverse ordering, or we could add the stdlib.h include in mm_malloc only wwhen in a hosted environment. Thoughts?

Given that we're hacking around this header file bug in the compiler, don't we have to allow the reverse ordering anyway?
#include <stddef.h>
extern void* malloc(size_t);
#include <stdlib.h>

As for the freestanding issue, the gcc version has #if __STDC_HOSTED in mmintrin.h, around the (only) include for mm_malloc.h.  It seems to me we might as well do that; the mm_malloc functions call malloc and free, so they won't work in a freestanding environment.

>  On Nov 17, 2010, at 4:35 PM, Chandler Carruth wrote:
> 
>> On Wed, Nov 17, 2010 at 11:33 AM, dalej <dalej at apple.com> wrote:
>> 
>> On Nov 17, 2010, at 10:39 AM, dalej wrote:
>> >>>> clang-i386-darwin10 running the gcc testsuite developed 5 new failures (there are others, claimed by Rafael but not fixed) in the range 119343-119345 (build 1625).  That's Chandler and John; all the patches look unrelated to the failures to me, but obviously one of them isn't.
>> 
>> John analyzed this as:
>> 
>> >>> gcc.apple/default-x86_64-sse3.c has:
>> >>> #include <pmmintrin.h>
>> 
>> ...which eventually includes <mm_malloc.h>, a gcc-supplied header....
>> 
>> >>> gcc's mm_malloc.h has:
>> >>> #include <stdlib.h>
>> >>>
>> >>> After r119343, Clang's mm_malloc.h has:
>> >>> #include <stddef.h>
>> >>>
>> >>> The test case contains a (spurious) call to exit().  Cue clang warning about exit() not being declared and dejagnu complaining about extra "errors".
>> 
>> I said:
>> 
>> > there's no reason <pmmintrin.h> should declare exit.  Test is broken, I'll fix.
>> 
>> 
>> but on further consideration I'm not so sure.  gcc's *mmintrin.h files have been (indirectly) including <stdlib.h> forever, at least since 2004, and use of those headers is widespread in X86 code.  Removing <stdlib.h> breaks these 5 tests, and may well break user code.   I don't think these headers are covered by any standard, but I'm not seeing a good reason to break compatibility with gcc in this way.  Chandler, why did you do this?  Other opinions?
>> 
>> Sorry this broke stuff. =/
>> 
>> My motivation for the change was quite simple: this is a builtin header file, and as such a minimal set of system header includes is preferable. None is even more preferable as it provides for the use of the header in a freestanding context. When I've discussed this header in the past, the sentiment from Chris and others was that there was a general trend in both GCC and Clang to reduce the number of system headers included by builtin headers.
>> 
>> I'd vote for continuing the trend. This isn't the first time it has happened (GCC dropped errno.h from mm_malloc.h not too long ago) and I can't imagine it will be the last. It's hard to argue that we should match GCC on this front when in fact, GCC is a moving target here, and moving in the same direction if at a different speed.
>> 
>> I also cannot come up with a principled reason why any code should expect an *mmintrin.h header to provide the definitions within stdlib.h. 'exit()' in particular just doesn't make sense. I think finding this bad code and fixing it is the correct approach.
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20101118/b4aec385/attachment.html>