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

[Chandler Carruth]

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.

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?


>
> 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/728ab1de/attachment.html>