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

[Manman via cfe-commits]

> On Oct 17, 2016, at 2:11 PM, Bruno Cardoso Lopes via cfe-commits <cfe-commits at lists.llvm.org> wrote:
> 
> 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).

@Richard,

I wonder why (prior to your patch) we need to use a different approach for builtin headers with local submodule visibility.

Thanks,

>> 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

@Bruno,

Can you try "-fdiagnostics-show-note-include-stack” so we know the other path that leads to string.h?

Manman

> 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
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits