[libcxx] r249738 - Split <ctype.h> out of <cctype>.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 14 17:55:55 PDT 2015
On Wed, Oct 14, 2015 at 5:22 PM, Sean Silva <chisophugis at gmail.com> wrote:
> On Tue, Oct 13, 2015 at 7:43 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>
>> On Tue, Oct 13, 2015 at 6:54 PM, Sean Silva <chisophugis at gmail.com>
>> wrote:
>>
>>> On Tue, Oct 13, 2015 at 6:14 PM, Richard Smith <richard at metafoo.co.uk>
>>> wrote:
>>>
>>>> On Tue, Oct 13, 2015 at 5:31 PM, Sean Silva <chisophugis at gmail.com>
>>>> wrote:
>>>>
>>>>> On Tue, Oct 13, 2015 at 3:17 PM, Richard Smith via cfe-commits <
>>>>> cfe-commits at lists.llvm.org> wrote:
>>>>>
>>>>>> On Tue, Oct 13, 2015 at 2:10 PM, Adrian Prantl via cfe-commits <
>>>>>> cfe-commits at lists.llvm.org> wrote:
>>>>>>
>>>>>>> Hi Richard,
>>>>>>>
>>>>>>> this commit appears to break the module self-host on Darwin.
>>>>>>>
>>>>>>> When compiling the following program generated by clang’s own cmake
>>>>>>> script:
>>>>>>>
>>>>>>> > #undef NDEBUG
>>>>>>> > #include <cassert>
>>>>>>> > #define NDEBUG
>>>>>>> > #include <cassert>
>>>>>>> > int main() { assert(this code is not compiled); }
>>>>>>>
>>>>>>> with clang++ -std=c++11 -fmodules -fcxx-modules test.cpp
>>>>>>>
>>>>>>
>>>>>> (You don't need -fcxx-modules here.)
>>>>>>
>>>>>>
>>>>>>> I get
>>>>>>>
>>>>>>> > While building module 'std' imported from /Users/buildslave/adrian/
>>>>>>> llvm.org/_build.ninja.release/bin/../include/c++/v1/cassert:20:
>>>>>>> > While building module 'Darwin' imported from
>>>>>>> /Users/buildslave/adrian/
>>>>>>> llvm.org/_build.ninja.release/bin/../include/c++/v1/cstddef:39:
>>>>>>> > In file included from <module-includes>:98:
>>>>>>> > In file included from
>>>>>>> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/usr/include/wchar.h:92:
>>>>>>> > In file included from
>>>>>>> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/usr/include/_wctype.h:57:
>>>>>>
>>>>>> > /Users/buildslave/adrian/
>>>>>>> llvm.org/_build.ninja.release/bin/../include/c++/v1/ctype.h:33:10:
>>>>>>> fatal error: cyclic dependency in module 'std': std -> Darwin -> std
>>>>>>> > #include <__config>
>>>>>>> > ^
>>>>>>> > While building module 'std' imported from /Users/buildslave/adrian/
>>>>>>> llvm.org/_build.ninja.release/bin/../include/c++/v1/cassert:20:
>>>>>>> > In file included from <module-includes>:1:
>>>>>>> > In file included from /Users/buildslave/adrian/
>>>>>>> llvm.org/_build.ninja.release/bin/../include/c++/v1/algorithm:624:
>>>>>>> > In file included from /Users/buildslave/adrian/
>>>>>>> llvm.org/_build.ninja.release/bin/../include/c++/v1/initializer_list:47
>>>>>>> :
>>>>>>> > /Users/buildslave/adrian/
>>>>>>> llvm.org/_build.ninja.release/bin/../include/c++/v1/cstddef:39:10:
>>>>>>> fatal error: could not build module 'Darwin'
>>>>>>> > #include <stddef.h>
>>>>>>> > ~~~~~~~~^
>>>>>>> > In file included from test.cpp:2:
>>>>>>> > /Users/buildslave/adrian/
>>>>>>> llvm.org/_build.ninja.release/bin/../include/c++/v1/cassert:20:10:
>>>>>>> fatal error: could not build module 'std'
>>>>>>> > #include <__config>
>>>>>>> > ~~~~~~~~^
>>>>>>> > 3 errors generated.
>>>>>>>
>>>>>>> Let me know how I can help in diagnosing what’s going on here.
>>>>>>>
>>>>>>
>>>>>> OK, I see what's wrong. Is this working any better for you in r250236?
>>>>>>
>>>>>
>>>>> We're still seeing:
>>>>>
>>>>> While building module 'Darwin' imported from /usr/include/assert.h:42:
>>>>> While building module 'std' imported from
>>>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/math.h:309:
>>>>>
>>>>
>>>> Argh, this is including <type_traits>, which is in the std module. Can
>>>> you try removing the header "type_traits" line from the libc++ module map
>>>> and see if that helps?
>>>>
>>>
>>> I've run into this issue in the past, and I don't think that will fix it
>>> (see below). Just to be sure, here is the output with type_traits removed
>>> from the module map::
>>>
>>> While building module 'Darwin' imported from /usr/include/assert.h:42:
>>> While building module 'std' imported from
>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/type_traits:211:
>>>
>>
>> That's an include of <cstddef>. We'd need to apply this workaround to
>> that header too (but I think the buck stops there).
>>
>
>> In file included from <module-includes>:1:
>>> In file included from
>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/algorithm:624:
>>> In file included from
>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/initializer_list:47:
>>> In file included from
>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/cstddef:43:
>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/stddef.h:46:15: fatal
>>> error: cyclic dependency in module 'Darwin': Darwin -> std -> Darwin
>>> #include_next <stddef.h>
>>> ^
>>> While building module 'Darwin' imported from /usr/include/assert.h:42:
>>> In file included from <module-includes>:80:
>>> In file included from
>>> /path/to/build_cmake/stage1/bin/../lib/clang/3.8.0/include/tgmath.h:29:
>>> In file included from
>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/math.h:309:
>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/type_traits:211:10:
>>> fatal error: could not build module 'std'
>>> #include <cstddef>
>>> ~~~~~~~~^
>>> In file included from modules.cpp:2:
>>> In file included from
>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/cassert:21:
>>> /usr/include/assert.h:42:10: fatal error: could not build module 'Darwin'
>>> #include <sys/cdefs.h>
>>> ~~~~~~~~^
>>> 3 errors generated.
>>>
>>>
>>>
>>>> What we really need here is a way to get the Darwin blah.h headers to
>>>> only include each other, and not find the libc++ headers. Do you need
>>>> something that works with new libc++ and old Clang, or would a Clang
>>>> feature to prevent the Darwin module from finding the std module's headers
>>>> work for you?
>>>>
>>>
>>> I've run into this issue in the wild with the modularized PS4 SDK, so I
>>> think that a clang feature that prevents this is the best fit. I think I
>>> may have already mentioned this to you at one of the socials but just to
>>> have it in writing here for everybody else, the root cause of the issue is
>>> that `-Imylib` will cause mylib/assert.h to be selected for `#include
>>> <assert.h>`, even when building the system module. Hence some random user
>>> header will end up as being part of the system module and the seed for
>>> problems has been planted.
>>>
>>> If the random user header then ends up including code from a module that
>>> depends on the system module, then clang will go off and build this other
>>> module, which itself (say) depends on the system module, which triggers the
>>> recursive dependency error.
>>>
>>> There's nothing special about system headers for this issue, but due to
>>> their position at the root of the dependency tree they seem to be the ones
>>> affected in practice. The fundamental problem is that some random header
>>> outside the module is interposing for the header listed in the module map.
>>> I think the feature basically needs to be about making the module more
>>> "hermetically sealed" in this scenario. Off the top of my head, maybe
>>> something like putting the module map's directory at the front of the
>>> search path for includes? That seems sort of hacky. Any ideas?
>>>
>>
>> I was thinking: if we found the module map for a module in some include
>> path, then we should build that module with only the directories from the
>> header search path at or before that directory in the list of include dirs.
>> That's probably a fairly simple change.
>>
>
> The intuition being that headers in earlier-listed include paths shouldn't
> depend on later-listed ones? I'm not sure how that will hold up in
> practice. It may work for the immediate issue of libc++, but it makes me
> uneasy.
>
It also makes me uneasy that we're making what was once a mostly-incidental
property (order of -I flags) be more fundamental. One way around this would
be to put something in the relevant module map file to opt it into this
behavior (and we could infer this flag for the Darwin module as a
compatibility hack).
Also, it's worth considering the long-term future for this. Are we forever
> going to rely on this strange header-forwarding arrangement forever?
> Looking at D12747 it just seems like the C++ and C standards are out of
> sync as far as their requirements; is that ever going to be fixed?
>
It seems unlikely that it will ever be completely fixed. The C++ forms of
these headers provide an overload set whereas C can't do so, the C++ forms
require functions to be used and not macros whereas the C forms allow
either, and so on; it seems unlikely that either the C or C++ committees
will budge on these questions. Implementing the C++ headers in terms of the
C headers seems like an arrangement we need to support in the long term.
(Another option would be to avoid using the C library headers entirely, but
that means we lose all the extensions that the C library provides, which
makes that approach very hard and bordering on infeasible.)
-- Sean Silva
>
>
>>
>>
>>> -- Sean Silva
>>>
>>>
>>>
>>>>
>>>>
>>>>> In file included from <module-includes>:1:
>>>>> In file included from
>>>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/algorithm:624:
>>>>> In file included from
>>>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/initializer_list:47:
>>>>> In file included from
>>>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/cstddef:43:
>>>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/stddef.h:46:15:
>>>>> fatal error: cyclic dependency in module 'Darwin': Darwin -> std -> Darwin
>>>>> #include_next <stddef.h>
>>>>> ^
>>>>> While building module 'Darwin' imported from /usr/include/assert.h:42:
>>>>> In file included from <module-includes>:80:
>>>>> In file included from
>>>>> /path/to/build_cmake/stage1/bin/../lib/clang/3.8.0/include/tgmath.h:29:
>>>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/math.h:309:10: fatal
>>>>> error: could not build module 'std'
>>>>> #include <type_traits>
>>>>> ~~~~~~~~^
>>>>> In file included from modules.cpp:2:
>>>>> In file included from
>>>>> /path/to/build_cmake/stage1/bin/../include/c++/v1/cassert:21:
>>>>> /usr/include/assert.h:42:10: fatal error: could not build module
>>>>> 'Darwin'
>>>>> #include <sys/cdefs.h>
>>>>> ~~~~~~~~^
>>>>> 3 errors generated.
>>>>>
>>>>>
>>>>> Looks like the system headers are being interposed.
>>>>>
>>>>> -- Sean Silva
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> Once this works, I’d like to to set up a green dragon bot that
>>>>>>> builds clang with LLVM_ENABLE_MODULES to catch similar problems earlier.
>>>>>>>
>>>>>>> -- adrian
>>>>>>>
>>>>>>> > On Oct 8, 2015, at 1:36 PM, Richard Smith via cfe-commits <
>>>>>>> cfe-commits at lists.llvm.org> wrote:
>>>>>>> >
>>>>>>> > Author: rsmith
>>>>>>> > Date: Thu Oct 8 15:36:30 2015
>>>>>>> > New Revision: 249738
>>>>>>> >
>>>>>>> > URL: http://llvm.org/viewvc/llvm-project?rev=249738&view=rev
>>>>>>> > Log:
>>>>>>> > Split <ctype.h> out of <cctype>.
>>>>>>> >
>>>>>>> > Added:
>>>>>>> > libcxx/trunk/include/ctype.h
>>>>>>> > - copied, changed from r249736, libcxx/trunk/include/cctype
>>>>>>> > Modified:
>>>>>>> > libcxx/trunk/include/cctype
>>>>>>> > libcxx/trunk/test/std/strings/c.strings/cctype.pass.cpp
>>>>>>> >
>>>>>>> > Modified: libcxx/trunk/include/cctype
>>>>>>> > URL:
>>>>>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/cctype?rev=249738&r1=249737&r2=249738&view=diff
>>>>>>> >
>>>>>>> ==============================================================================
>>>>>>> > --- libcxx/trunk/include/cctype (original)
>>>>>>> > +++ libcxx/trunk/include/cctype Thu Oct 8 15:36:30 2015
>>>>>>> > @@ -37,10 +37,6 @@ int toupper(int c);
>>>>>>> >
>>>>>>> > #include <__config>
>>>>>>> > #include <ctype.h>
>>>>>>> > -#if defined(_LIBCPP_MSVCRT)
>>>>>>> > -#include "support/win32/support.h"
>>>>>>> > -#include "support/win32/locale_win32.h"
>>>>>>> > -#endif // _LIBCPP_MSVCRT
>>>>>>> >
>>>>>>> > #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
>>>>>>> > #pragma GCC system_header
>>>>>>> > @@ -48,33 +44,19 @@ int toupper(int c);
>>>>>>> >
>>>>>>> > _LIBCPP_BEGIN_NAMESPACE_STD
>>>>>>> >
>>>>>>> > -#undef isalnum
>>>>>>> > using ::isalnum;
>>>>>>> > -#undef isalpha
>>>>>>> > using ::isalpha;
>>>>>>> > -#undef isblank
>>>>>>> > using ::isblank;
>>>>>>> > -#undef iscntrl
>>>>>>> > using ::iscntrl;
>>>>>>> > -#undef isdigit
>>>>>>> > using ::isdigit;
>>>>>>> > -#undef isgraph
>>>>>>> > using ::isgraph;
>>>>>>> > -#undef islower
>>>>>>> > using ::islower;
>>>>>>> > -#undef isprint
>>>>>>> > using ::isprint;
>>>>>>> > -#undef ispunct
>>>>>>> > using ::ispunct;
>>>>>>> > -#undef isspace
>>>>>>> > using ::isspace;
>>>>>>> > -#undef isupper
>>>>>>> > using ::isupper;
>>>>>>> > -#undef isxdigit
>>>>>>> > using ::isxdigit;
>>>>>>> > -#undef tolower
>>>>>>> > using ::tolower;
>>>>>>> > -#undef toupper
>>>>>>> > using ::toupper;
>>>>>>> >
>>>>>>> > _LIBCPP_END_NAMESPACE_STD
>>>>>>> >
>>>>>>> > Copied: libcxx/trunk/include/ctype.h (from r249736,
>>>>>>> libcxx/trunk/include/cctype)
>>>>>>> > URL:
>>>>>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/ctype.h?p2=libcxx/trunk/include/ctype.h&p1=libcxx/trunk/include/cctype&r1=249736&r2=249738&rev=249738&view=diff
>>>>>>> >
>>>>>>> ==============================================================================
>>>>>>> > --- libcxx/trunk/include/cctype (original)
>>>>>>> > +++ libcxx/trunk/include/ctype.h Thu Oct 8 15:36:30 2015
>>>>>>> > @@ -1,5 +1,5 @@
>>>>>>> > // -*- C++ -*-
>>>>>>> > -//===---------------------------- cctype
>>>>>>> ----------------------------------===//
>>>>>>> > +//===---------------------------- ctype.h
>>>>>>> ---------------------------------===//
>>>>>>> > //
>>>>>>> > // The LLVM Compiler Infrastructure
>>>>>>> > //
>>>>>>> > @@ -8,14 +8,11 @@
>>>>>>> > //
>>>>>>> >
>>>>>>> //===----------------------------------------------------------------------===//
>>>>>>> >
>>>>>>> > -#ifndef _LIBCPP_CCTYPE
>>>>>>> > -#define _LIBCPP_CCTYPE
>>>>>>> > +#ifndef _LIBCPP_CTYPE_H
>>>>>>> > +#define _LIBCPP_CTYPE_H
>>>>>>> >
>>>>>>> > /*
>>>>>>> > - cctype synopsis
>>>>>>> > -
>>>>>>> > -namespace std
>>>>>>> > -{
>>>>>>> > + ctype.h synopsis
>>>>>>> >
>>>>>>> > int isalnum(int c);
>>>>>>> > int isalpha(int c);
>>>>>>> > @@ -31,52 +28,41 @@ int isupper(int c);
>>>>>>> > int isxdigit(int c);
>>>>>>> > int tolower(int c);
>>>>>>> > int toupper(int c);
>>>>>>> > -
>>>>>>> > -} // std
>>>>>>> > */
>>>>>>> >
>>>>>>> > #include <__config>
>>>>>>> > -#include <ctype.h>
>>>>>>> > -#if defined(_LIBCPP_MSVCRT)
>>>>>>> > -#include "support/win32/support.h"
>>>>>>> > -#include "support/win32/locale_win32.h"
>>>>>>> > -#endif // _LIBCPP_MSVCRT
>>>>>>> > +#include_next <ctype.h>
>>>>>>> >
>>>>>>> > #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
>>>>>>> > #pragma GCC system_header
>>>>>>> > #endif
>>>>>>> >
>>>>>>> > -_LIBCPP_BEGIN_NAMESPACE_STD
>>>>>>> > +#ifdef __cplusplus
>>>>>>> > +
>>>>>>> > +#if defined(_LIBCPP_MSVCRT)
>>>>>>> > +// We support including .h headers inside 'extern "C"' contexts,
>>>>>>> so switch
>>>>>>> > +// back to C++ linkage before including these C++ headers.
>>>>>>> > +extern "C++" {
>>>>>>> > + #include "support/win32/support.h"
>>>>>>> > + #include "support/win32/locale_win32.h"
>>>>>>> > +}
>>>>>>> > +#endif // _LIBCPP_MSVCRT
>>>>>>> >
>>>>>>> > #undef isalnum
>>>>>>> > -using ::isalnum;
>>>>>>> > #undef isalpha
>>>>>>> > -using ::isalpha;
>>>>>>> > #undef isblank
>>>>>>> > -using ::isblank;
>>>>>>> > #undef iscntrl
>>>>>>> > -using ::iscntrl;
>>>>>>> > #undef isdigit
>>>>>>> > -using ::isdigit;
>>>>>>> > #undef isgraph
>>>>>>> > -using ::isgraph;
>>>>>>> > #undef islower
>>>>>>> > -using ::islower;
>>>>>>> > #undef isprint
>>>>>>> > -using ::isprint;
>>>>>>> > #undef ispunct
>>>>>>> > -using ::ispunct;
>>>>>>> > #undef isspace
>>>>>>> > -using ::isspace;
>>>>>>> > #undef isupper
>>>>>>> > -using ::isupper;
>>>>>>> > #undef isxdigit
>>>>>>> > -using ::isxdigit;
>>>>>>> > #undef tolower
>>>>>>> > -using ::tolower;
>>>>>>> > #undef toupper
>>>>>>> > -using ::toupper;
>>>>>>> >
>>>>>>> > -_LIBCPP_END_NAMESPACE_STD
>>>>>>> > +#endif
>>>>>>> >
>>>>>>> > -#endif // _LIBCPP_CCTYPE
>>>>>>> > +#endif // _LIBCPP_CTYPE_H
>>>>>>> >
>>>>>>> > Modified: libcxx/trunk/test/std/strings/c.strings/cctype.pass.cpp
>>>>>>> > URL:
>>>>>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/strings/c.strings/cctype.pass.cpp?rev=249738&r1=249737&r2=249738&view=diff
>>>>>>> >
>>>>>>> ==============================================================================
>>>>>>> > --- libcxx/trunk/test/std/strings/c.strings/cctype.pass.cpp
>>>>>>> (original)
>>>>>>> > +++ libcxx/trunk/test/std/strings/c.strings/cctype.pass.cpp Thu
>>>>>>> Oct 8 15:36:30 2015
>>>>>>> > @@ -86,18 +86,18 @@ int main()
>>>>>>> > static_assert((std::is_same<decltype(std::tolower(0)),
>>>>>>> int>::value), "");
>>>>>>> > static_assert((std::is_same<decltype(std::toupper(0)),
>>>>>>> int>::value), "");
>>>>>>> >
>>>>>>> > - assert(isalnum('a'));
>>>>>>> > - assert(isalpha('a'));
>>>>>>> > - assert(isblank(' '));
>>>>>>> > - assert(!iscntrl(' '));
>>>>>>> > - assert(!isdigit('a'));
>>>>>>> > - assert(isgraph('a'));
>>>>>>> > - assert(islower('a'));
>>>>>>> > - assert(isprint('a'));
>>>>>>> > - assert(!ispunct('a'));
>>>>>>> > - assert(!isspace('a'));
>>>>>>> > - assert(!isupper('a'));
>>>>>>> > - assert(isxdigit('a'));
>>>>>>> > - assert(tolower('A') == 'a');
>>>>>>> > - assert(toupper('a') == 'A');
>>>>>>> > + assert(std::isalnum('a'));
>>>>>>> > + assert(std::isalpha('a'));
>>>>>>> > + assert(std::isblank(' '));
>>>>>>> > + assert(!std::iscntrl(' '));
>>>>>>> > + assert(!std::isdigit('a'));
>>>>>>> > + assert(std::isgraph('a'));
>>>>>>> > + assert(std::islower('a'));
>>>>>>> > + assert(std::isprint('a'));
>>>>>>> > + assert(!std::ispunct('a'));
>>>>>>> > + assert(!std::isspace('a'));
>>>>>>> > + assert(!std::isupper('a'));
>>>>>>> > + assert(std::isxdigit('a'));
>>>>>>> > + assert(std::tolower('A') == 'a');
>>>>>>> > + assert(std::toupper('a') == 'A');
>>>>>>> > }
>>>>>>> >
>>>>>>> >
>>>>>>> > _______________________________________________
>>>>>>> > cfe-commits mailing list
>>>>>>> > cfe-commits at lists.llvm.org
>>>>>>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> cfe-commits mailing list
>>>>>>> cfe-commits at lists.llvm.org
>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> cfe-commits mailing list
>>>>>> cfe-commits at lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151014/dedee2f3/attachment-0001.html>
More information about the cfe-commits
mailing list