[libcxx] r249738 - Split <ctype.h> out of <cctype>.

[Bruno Cardoso Lopes via cfe-commits]

Hi,

On Fri, Oct 14, 2016 at 3:09 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Fri, Oct 14, 2016 at 11:44 AM, Bruno Cardoso Lopes
> <bruno.cardoso at gmail.com> wrote:
>>
>> Hi Richard,
>>
>> I have a patch on top of your suggested patch from a year ago, that
>> break the cyclic dependency we're seeing, with this (and a few changes
>> to the SDK) we can bootstrap clang with submodule local visibility on
>> darwin. I've attached the patch with a reduced, standalone testcase
>> that doesn't depend on the SDK. The issues that are not covered by
>> your patch, that I cover in mine, are related to built-in and textual
>> headers: they can be found in paths where they don't map to any
>> modules, triggering other cycles. I fix that by looking further to
>> find a matching module for the header in question, instead the first
>> found header in header search.
>
>
> It looks like the 0002 patch is working around a bug in the 0001 patch: with
> 0001 applied, a module with [no_undeclared_includes] can still include a
> textual header from another module on which it doesn't declare a dependency
> (in the testcase, the libc module is incorrectly permitted to include the
> textual header <stdio.h> from libc++). Rather than preferring non-modular
> headers over modular headers as the 0002 patch does, I wonder if the issue
> could instead be resolved by fixing that apparent bug.

Thanks for the fast answer and for the new patch :-)
My intend with 0002 was actually to prefer modular headers instead of
non-modular, but fallback to the later in case none is found.

> I gave that a go; the result is attached. I also had to change the way we
> handle builtin headers -- instead of implicitly including (for instance) the
> builtin <stddef.h> as a modular header in any module that provides its own
> <stddef.h>, I now only include it as a textual header (this also lets us use
> the same approach for this case whether or not we're using local submodule
> visibility). That exposed a couple of testcases that were (unreasonably, in
> my opinion) failing to include_next the real builtin header from their
> wrapper header.

I'm curious about these, are they from clang tests?

> The attached patch causes your testcase to pass; I'd be interested to know
> if it works in practice on Darwin.

It works for building the Darwin module, but failed for "#include
<algorithm>" on darwin (which was working under 0001+0002 patch):

While building module 'std' imported from all-headers.cpp:1:
While building module 'Darwin' imported from
/clang-install/include/c++/v1/string.h:61:
In file included from <module-includes>:422:
In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/mach.h:67:
In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/mach_interface.h:42:
In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/clock_priv.h:6:
/clang-install/include/c++/v1/string.h:74:7: error: redefinition of
'__libcpp_strchr'
char* __libcpp_strchr(const char* __s, int __c) {return
(char*)strchr(__s, __c);}
      ^
/clang-install/include/c++/v1/string.h:74:7: note: previous definition is here
char* __libcpp_strchr(const char* __s, int __c) {return
(char*)strchr(__s, __c);}
      ^
While building module 'std' imported from all-headers.cpp:1:
While building module 'Darwin' imported from
/clang-install/include/c++/v1/string.h:61:
In file included from <module-includes>:422:
In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/mach.h:67:
In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/mach_interface.h:42:
In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/clock_priv.h:6:
/clang-install/include/c++/v1/string.h:76:13: error: redefinition of 'strchr'
const char* strchr(const char* __s, int __c) {return __libcpp_strchr(__s, __c);}
            ^
/clang-install/include/c++/v1/string.h:76:13: note: previous definition is here
const char* strchr(const char* __s, int __c) {return __libcpp_strchr(__s, __c);}
            ^
<...>
While building module 'std' imported from all-headers.cpp:1:
In file included from <module-includes>:1:
In file included from /clang-install/include/c++/v1/algorithm:638:
In file included from /clang-install/include/c++/v1/cstring:61:
/clang-install/include/c++/v1/string.h:61:15: fatal error: could not
build module 'Darwin'
#include_next <string.h>
 ~~~~~~~~~~~~~^
all-headers.cpp:1:10: fatal error: could not build module 'std'
#include <algorithm>
 ~~~~~~~~^
17 errors generated.

It looks like "#include_next <string.h>" is poiting back to
/clang-install/include/c++/v1/string.h
FWIW, string.h is also in SDK/usr/include/string.h, under
Darwin.C.string module.
I'll get back to you with a small testcase once I'm able to reduce this.

Thanks!


-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc