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

[John Thompson]

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


On Fri, Apr 24, 2015 at 4:06 PM, Sean Silva <chisophugis at gmail.com> wrote:

> ================
> Comment at: lib/Headers/mm_malloc.h:27-33
> @@ -26,3 +26,9 @@
>
> -#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
>
> ----------------
> rsmith wrote:
> > 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?)
> 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.
>
> ================
> Comment at: lib/Headers/stdarg.h:29
> @@ -28,1 +28,3 @@
>
> +#include <_gnuc_va_list.h>
> +
> ----------------
> rsmith wrote:
> > 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.
> 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.
>
> http://reviews.llvm.org/D9229
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>


-- 
John Thompson
John.Thompson.JTSoftware at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150424/c27259c4/attachment.html>