On Tue, Oct 13, 2015 at 7:43 PM, Richard Smith <rich...@metafoo.co.uk>
wrote:
> On Tue, Oct 13, 2015 at 6:54 PM, Sean Silva <chisophu...@gmail.com> wrote:
>
>> On Tue, Oct 13, 2015 at 6:14 PM, Richard Smith <rich...@metafoo.co.uk>
>> wrote:
>>
>>> On Tue, Oct 13, 2015 at 5:31 PM, Sean Silva <chisophu...@gmail.com>
>>> wrote:
>>>
>>>> On Tue, Oct 13, 2015 at 3:17 PM, Richard Smith via cfe-commits <
>>>> cfe-commits@lists.llvm.org> wrote:
>>>>
>>>>> On Tue, Oct 13, 2015 at 2:10 PM, Adrian Prantl via cfe-commits <
>>>>> cfe-commits@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.
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?
-- 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@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@lists.llvm.org
>>>>>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>
>>>>>> _______________________________________________
>>>>>> cfe-commits mailing list
>>>>>> cfe-commits@lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits@lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>
>>>
>>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
