Page MenuHomePhabricator

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

Authored by jtsoftware on Apr 23 2015, 12:09 PM.

Details

Reviewers
rsmith
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.

Diff Detail

Event Timeline

jtsoftware retitled this revision from to [PATCH] clang internal library headers changes for C++ modules.
jtsoftware updated this object.
jtsoftware edited the test plan for this revision. (Show Details)
jtsoftware added a reviewer: rsmith.
jtsoftware added a subscriber: Unknown Object (MLST).
ygao added a subscriber: ygao.Apr 23 2015, 12:29 PM

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

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 added inline comments.Apr 24 2015, 1:24 PM
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 added a subscriber: silvas.Apr 24 2015, 4:06 PM
silvas added inline comments.
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.

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 abandoned this revision.May 4 2015, 6:34 PM

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