[PATCH] clang internal library headers changes for C++ modules

[jtsoftware]

Summary

These changes help us fix some issues related to C++ modules, and will help us diverge less from trunk in our SDK headers.

Create new file _denormals.h to contain duplicate definitions from pmmintrin.h (and xmmintrin.h, actually our version only, which also needed the denormal definitions).
Change pmmintrin.h and xmmintrin.h to include _denormals.h and delete duplicate definitions.
Change mm_malloc.h to not include stdlib.h and size_t, as this creates a circular module reference. Locally declare malloc and free with a size_t equivalent type, and replace other size_t references.
Create new file _gnu_va_list.h to define builtin_va_list. We had duplicate definitions of this locally, so we broke it out into a common header.
Change stdarg.h to include _gnu_va_list.h and delete the definition of builtin_va_list there.

[ygao]

The header of lib/Headers/_denormals.h says, "_denormals.h - SSE intrinsics", is it true? The header seems to only define a few macros.
The header of _gnuc_va_list.h maybe should include a one-line description too.

_gnuc_va_list.h uses "#pragma once" while the other header uses:

#ifndef __DENORMALS_H
#define __DENORMALS_H
...
#endif

[jtsoftware]

Hi, we've decided to back out the changes to pmmintrin.h and xmmintrin.h and not create _denormals.h.

This leaves the changes to mm_malloc.h, stdarg.h and the new file _gnu_va_list.h.

Thanks.

-John

[rsmith]

lib/Headers/mm_malloc.h

27–33

The intended layering is that Clang's _Builtin_intrinsics module is a layer on top of the C standard library module. Does that not work in your setup? (Does the module defining <stdlib.h> contain a header that includes these intrinsics headers?)

lib/Headers/stdarg.h

29

Can you say a bit more about why this split is useful to you? Do you want to include this part of <stdarg.h> and not the rest, and if so, why?

It looks to me like we have a more fundamental problem with our <stdarg.h>; like <stddef.h> it appears to be intended that glibc can request individual pieces of it with __need_* macros -- or at least, if __need___va_list is defined, we're supposed to vend a __gnuc_va_list typedef and nothing else.

So I think we should rearrange this header to properly handle __need___va_list in general, and the behavior you want here (to be able to produce __gnuc_va_list only) will naturally fall out from that.

[silvas]

lib/Headers/mm_malloc.h

27–33

I think this is a quirk of how I bootstrapped modules on PS4.

My first target for modularization (after measuring where we were spending time parsing) was a vector math library that only depended on the intrinsics (not mm_malloc though). So initially I just needed vector math lib -> _Builtin_intrinsics and this was a way to work around the issues I was seeing (and I guess we've just been doing this ever since). Going back now, it looks like there is a relatively self-contained part of the system headers that reference the intrinsics and can be split out into their own top-level module so that we can use the layering you describe.

lib/Headers/stdarg.h

29

John, what error were you seeing that necessitated this? I just did some micro-testing and it seems that clang will actually merge a duplicated definition here.

[jtsoftware]

Our process for trying to get the SDK headers in shape for modules with
respect to duplicate definitions was to mechanically break out the
definition into a separate file and include that file in the affected
header. This was done without really contemplating why there were
duplicate definitions in the fist place. In this case, the _gnuc_va_list
symbol was defined in both BSD's machine/_types.h and Eli Friedman's
stdarg.h header, and the modularize tool flagged this.

It might very well be and probably is the case that it doesn't need to be
in machine/_types.h. I can compile the headers at least with it missing,
and later we could try a full system build. I was told that a standard
header can't include other standard headers if it's not already done, so I
assumed including stdarg.h in machine/_types.h was out.

So the intrinsics are meant to be above the standard headers in layering?
I assumed that because of their low-level nature, they were at the bottom.
We actually put the intrinsics (with stdarg.h and mm_malloc.h) in a totally
separate directory (host_tools/lib/clang/include instead of
target/include), and indeed, some of the intrinsic headers are included by
headers in both of our other SDK include directories. So having
mm_malloc.h include stdlib.h was problematic, hence my changes to mm_malloc
to remove the dependence.

Any opinions on how to handle this?

-John

[jtsoftware]

Turns out we don't need this after all. Sorry.

lib/Headers/_gnuc_va_list.h

+/*===---- _gnuc_va_list.h-------------------------------------------------===
+*
+* Permission is hereby granted, free of charge, to any person obtaining a copy
+* of this software and associated documentation files (the "Software"), to deal
+* in the Software without restriction, including without limitation the rights
+* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+* copies of the Software, and to permit persons to whom the Software is
+* furnished to do so, subject to the following conditions:
+*
+* The above copyright notice and this permission notice shall be included in
+* all copies or substantial portions of the Software.
+*
+* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+* THE SOFTWARE.
+*
+*===-----------------------------------------------------------------------===
+*/
+
+#pragma once
+
+#ifndef _VA_LIST_DECLARED
+#define _VA_LIST_DECLARED
+typedef __builtin_va_list __gnuc_va_list;
+#endif

lib/Headers/mm_malloc.h

  * THE SOFTWARE.
  *
  *===-----------------------------------------------------------------------===
  */
 
 #ifndef __MM_MALLOC_H
 #define __MM_MALLOC_H
 
-#include <stdlib.h>
+#ifdef __cplusplus
+extern "C" void free(void *);
+extern "C" void *malloc(__SIZE_TYPE__);
+#else
+void free(void *);
+void *malloc(__SIZE_TYPE__);
+#endif
 
 #ifdef _WIN32
 #include <malloc.h>
 #else
 #ifndef __cplusplus
-extern int posix_memalign(void **__memptr, size_t __alignment, size_t __size);
+extern int posix_memalign(void **__memptr, __SIZE_TYPE__ __alignment, __SIZE_TYPE__ __size);
 #else
 // Some systems (e.g. those with GNU libc) declare posix_memalign with an
 // exception specifier. Via an "egregious workaround" in
 // Sema::CheckEquivalentExceptionSpec, Clang accepts the following as a valid
 // redeclaration of glibc's declaration.
-extern "C" int posix_memalign(void **__memptr, size_t __alignment, size_t __size);
+extern "C" int posix_memalign(void **__memptr, __SIZE_TYPE__ __alignment, __SIZE_TYPE__ __size);
 #endif
 #endif
 
 #if !(defined(_WIN32) && defined(_mm_malloc))
 static __inline__ void *__attribute__((__always_inline__, __nodebug__,
                                        __malloc__))
-_mm_malloc(size_t __size, size_t __align)
+_mm_malloc(__SIZE_TYPE__ __size, __SIZE_TYPE__ __align)
 {
   if (__align == 1) {
     return malloc(__size);
   }
 
   if (!(__align & (__align - 1)) && __align < sizeof(void *))
     __align = sizeof(void *);
 

lib/Headers/stdarg.h

  * THE SOFTWARE.
  *
  *===-----------------------------------------------------------------------===
  */
 
 #ifndef __STDARG_H
 #define __STDARG_H
 
+#include <_gnuc_va_list.h>
+
 #ifndef _VA_LIST
 typedef __builtin_va_list va_list;
 #define _VA_LIST
 #endif
 #define va_start(ap, param) __builtin_va_start(ap, param)
 #define va_end(ap)          __builtin_va_end(ap)
 #define va_arg(ap, type)    __builtin_va_arg(ap, type)
 
 /* GCC always defines __va_copy, but does not define va_copy unless in c99 mode
  * or -ansi is not specified, since it was not part of C90.
  */
 #define __va_copy(d,s) __builtin_va_copy(d,s)
 
 #if __STDC_VERSION__ >= 199901L || __cplusplus >= 201103L || !defined(__STRICT_ANSI__)
 #define va_copy(dest, src)  __builtin_va_copy(dest, src)
 #endif
 
-/* Hack required to make standard headers work, at least on Ubuntu */
-#ifndef __GNUC_VA_LIST
-#define __GNUC_VA_LIST 1
-#endif
-typedef __builtin_va_list __gnuc_va_list;
-
 #endif /* __STDARG_H */