[libcxx][module-map] creates submodules for private headers

[cjdb]

Summary

Most of our private headers need to be treated as submodules so that
Clang modules can export things correctly. Previous commits that split
monolithic headers into smaller chunks were unaware of this requirement,
and so this is being addressed in one fell swoop. Moving forward, most
new headers will need to have their own submodule (anything that's
conditionally included is exempt from this rule, which means __support
headers aren't made into submodules).

This hasn't been marked NFC, since I'm not 100% sure that's the case.

[cjdb]

fixes benign typo

[• Quuxplusone]

Seems surprisingly painless.
You did miss the other "libc++ classic style" helper headers, e.g. __functional_03; did you want to get those in this same PR?

libcxx/include/module.modulemap

297–298

requires cplusplus20? or what's the rationale for adding that on various headers?
E.g. I see <latch> is marked with requires cplusplus14, even though it's also new-in-C++20.

460–467

I like this one-line-per-header formatting. Please also use it for __utility and __format and __algorithm.

[ldionne]

This LGTM if CI passes, and with Arthur's comments. Thanks a lot for fixing this. As we just discussed, I will add a CI configuration that runs the test suite and used -fmodules -fcxx-modules, which should allow us to catch even more issues, and catch them before they break other people in the future.

[cjdb]

• removes __algorithm/unwrap_iter.h
• modularises most global __headers

[cjdb]

rebases

[cjdb]

In D103551#2794554, @Quuxplusone wrote:

Seems surprisingly painless.
You did miss the other "libc++ classic style" helper headers, e.g. __functional_03; did you want to get those in this same PR?

Done for all but __functional_(base_)?03 since they're designed to be textual includes.
I think adjusting those deserves its own patch.

libcxx/include/module.modulemap

297–298

Nice catch. For latch specifically, see D68480. Looks like libc++ supports it from C++14 onward.

460–467

Thanks, didn't notice I missed a few.

[cjdb]

rebases to reactivate CI

[Mordante]

Thanks a lot for working on this! And thanks @ldionne for setting up a CI to catch these issues earlier!

libcxx/include/module.modulemap

368

Just for my understanding. This header is C++20. Do headers in the internal module not set their own requires?

[cjdb]

rebases to activate CI

[cjdb]

Answered @Mordante's question (I forgot to press submit down here).

libcxx/include/module.modulemap

368

It's unclear to me whether or not we can put requires cplusplus20 on because we unconditionally include these headers.

[phosek]

After this change landed, we started seeing a number of crashes in our CI when using the just built Clang to run libc++ tests (that is using the runtimes build):

Failed Tests (6):
  libc++ :: libcxx/modules/cinttypes_exports.compile.pass.cpp
  libc++ :: libcxx/modules/clocale_exports.compile.pass.cpp
  libc++ :: libcxx/modules/cstdint_exports.compile.pass.cpp
  libc++ :: libcxx/modules/inttypes_h_exports.compile.pass.cpp
  libc++ :: libcxx/modules/stdint_h_exports.compile.pass.cpp
  libc++ :: libcxx/modules/stds_include.sh.cpp

They all fail with the same error:

clang++: clang/lib/Serialization/ASTWriter.cpp:2502: unsigned int clang::ASTWriter::getSubmoduleID(clang::Module *): Assertion `(ID || !Mod) && "asked for module ID for non-local, non-imported module"' failed.
clang++: llvm/include/llvm/Bitstream/BitstreamWriter.h:124: llvm::BitstreamWriter::~BitstreamWriter(): Assertion `BlockScope.empty() && CurAbbrevs.empty() && "Block imbalance"' failed.
clang-13: error: clang frontend command failed with exit code 134 (use -v to see invocation)

[cjdb]

In D103551#2801442, @phosek wrote:

After this change landed, we started seeing a number of crashes in our CI when using the just built Clang to run libc++ tests (that is using the runtimes build):

Failed Tests (6):
  libc++ :: libcxx/modules/cinttypes_exports.compile.pass.cpp
  libc++ :: libcxx/modules/clocale_exports.compile.pass.cpp
  libc++ :: libcxx/modules/cstdint_exports.compile.pass.cpp
  libc++ :: libcxx/modules/inttypes_h_exports.compile.pass.cpp
  libc++ :: libcxx/modules/stdint_h_exports.compile.pass.cpp
  libc++ :: libcxx/modules/stds_include.sh.cpp

They all fail with the same error:

clang++: clang/lib/Serialization/ASTWriter.cpp:2502: unsigned int clang::ASTWriter::getSubmoduleID(clang::Module *): Assertion `(ID || !Mod) && "asked for module ID for non-local, non-imported module"' failed.
clang++: llvm/include/llvm/Bitstream/BitstreamWriter.h:124: llvm::BitstreamWriter::~BitstreamWriter(): Assertion `BlockScope.empty() && CurAbbrevs.empty() && "Block imbalance"' failed.
clang-13: error: clang frontend command failed with exit code 134 (use -v to see invocation)

Interesting. Our scheduled Runtimes build is passing, so I don't know what's up there. Could you please link to one of the failing builds? I'd like to read the diagnostics.

[phosek]

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8845421670067038960/+/u/clang/test/stdout is the full log.

[cjdb]

In D103551#2801496, @phosek wrote:

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8845421670067038960/+/u/clang/test/stdout is the full log.

Is this for Chromium or Fuchsia? Either way, I'll take a look at it tomorrow morning.

[phosek]

In D103551#2801497, @cjdb wrote:

In D103551#2801496, @phosek wrote:

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8845421670067038960/+/u/clang/test/stdout is the full log.

Is this for Chromium or Fuchsia? Either way, I'll take a look at it tomorrow morning.

It's for Fuchsia (we use Chromium infrastructure hence the URL).

libcxx/include/__iterator/advance.h

 #ifndef _LIBCPP___ITERATOR_ADVANCE_H
 #define _LIBCPP___ITERATOR_ADVANCE_H
 
 #include <__config>
 #include <__debug>
 #include <__function_like.h>
 #include <__iterator/concepts.h>
 #include <__iterator/incrementable_traits.h>
+#include <__iterator/iterator_traits.h>
 #include <concepts>
 #include <limits>
 #include <type_traits>
 #include <utility>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif

libcxx/include/__iterator/indirect_concepts.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #ifndef _LIBCPP___ITERATOR_INDIRECT_CONCEPTS_H
 #define _LIBCPP___ITERATOR_INDIRECT_CONCEPTS_H
 
 #include <__config>
 #include <__iterator/concepts.h>
 #include <__iterator/incrementable_traits.h>
+#include <__iterator/iterator_traits.h>
+#include <__iterator/readable_traits.h>
 #include <concepts>
 #include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS

libcxx/include/__iterator/next.h

 #ifndef _LIBCPP___ITERATOR_NEXT_H
 #define _LIBCPP___ITERATOR_NEXT_H
 
 #include <__config>
 #include <__function_like.h>
 #include <__iterator/advance.h>
 #include <__iterator/concepts.h>
 #include <__iterator/incrementable_traits.h>
+#include <__iterator/iterator_traits.h>
+#include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 

libcxx/include/__iterator/prev.h

 #ifndef _LIBCPP___ITERATOR_PREV_H
 #define _LIBCPP___ITERATOR_PREV_H
 
 #include <__config>
 #include <__function_like.h>
 #include <__iterator/advance.h>
 #include <__iterator/concepts.h>
 #include <__iterator/incrementable_traits.h>
+#include <__iterator/iterator_traits.h>
+#include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 

libcxx/include/__memory/allocation_guard.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___MEMORY_ALLOCATION_GUARD_H
 #define _LIBCPP___MEMORY_ALLOCATION_GUARD_H
 
 #include <__config>
 #include <__memory/allocator_traits.h>
 #include <cstddef>
+#include <utility>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 

libcxx/include/__memory/allocator.h

 template <class _Tp, class _Up>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
 bool operator==(const allocator<_Tp>&, const allocator<_Up>&) _NOEXCEPT {return true;}
 
 template <class _Tp, class _Up>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
 bool operator!=(const allocator<_Tp>&, const allocator<_Up>&) _NOEXCEPT {return false;}
 
+template <class _Tp>
+struct __is_default_allocator<allocator<_Tp> > : true_type { };
+
 _LIBCPP_END_NAMESPACE_STD
 
 _LIBCPP_POP_MACROS
 
 #endif // _LIBCPP___MEMORY_ALLOCATOR_H

libcxx/include/__memory/allocator_traits.h

#else
     using type = typename _Traits::template rebind_alloc<_Tp>::other;
 #endif
 };
 
 // __is_default_allocator
 template <class _Tp>
 struct __is_default_allocator : false_type { };
 
-template <class _Tp>
-struct __is_default_allocator<allocator<_Tp> > : true_type { };
-
 // __is_cpp17_move_insertable
 template <class _Alloc, class = void>
 struct __is_cpp17_move_insertable
     : is_move_constructible<typename _Alloc::value_type>
 { };
 
 template <class _Alloc>
 struct __is_cpp17_move_insertable<_Alloc, _EnableIf<

libcxx/include/__memory/pointer_safety.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___MEMORY_POINTER_SAFETY_H
 #define _LIBCPP___MEMORY_POINTER_SAFETY_H
 
 #include <__config>
+#include <cstddef>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 

libcxx/include/__memory/pointer_traits.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___MEMORY_POINTER_TRAITS_H
 #define _LIBCPP___MEMORY_POINTER_TRAITS_H
 
 #include <__config>
+#include <__memory/addressof.h>
 #include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>

libcxx/include/module.modulemap

module chrono {
     header "chrono"
     export *
   }
   module codecvt {
     header "codecvt"
     export *
   }
   module compare {
+    requires cplusplus20
     header "compare"
     export *
   }
   module complex {
     header "complex"
     export *
   }
   module concepts {
+    requires cplusplus20
     header "concepts"
     export *
   }
   module condition_variable {
     header "condition_variable"
     export *
   }
   module deque {
     header "deque"
     export initializer_list
     export *
   }
   module exception {
     header "exception"
     export *
   }
   module execution {
     header "execution"
     export *
   }
   module filesystem {
     header "filesystem"
     export *
   }
   module format {
+    requires cplusplus20
     header "format"
     export *
+
+    module __format {
+      module format_error         { header "__format/format_error.h"         }
+      module format_parse_context { header "__format/format_parse_context.h" }
+    }
   }
   module forward_list {
     header "forward_list"
     export initializer_list
     export *
   }
   module fstream {
     header "fstream"
module std [system] {
   module istream {
     header "istream"
     // FIXME: should re-export ios, streambuf?
     export *
   }
   module iterator {
     header "iterator"
     export *
+
+    module __iterator {
+      module advance              { header "__iterator/advance.h"              }
+      module concepts             { header "__iterator/concepts.h"             }
+      module default_sentinel     { header "__iterator/default_sentinel.h"     }
+      module incrementable_traits { header "__iterator/incrementable_traits.h" }
+      module indirect_concepts    { header "__iterator/indirect_concepts.h"    }
+      module iter_move            { header "__iterator/iter_move.h"            }
+      module iterator_traits      { header "__iterator/iterator_traits.h"      }
+      module next                 { header "__iterator/next.h"                 }
+      module prev                 { header "__iterator/prev.h"                 }
+      module projected            { header "__iterator/projected.h"            }
+      module readable_traits      { header "__iterator/readable_traits.h"      }
+    }
   }
   module latch {
     requires cplusplus14
     header "latch"
     export *
   }
   module limits {
     header "limits"
module std [system] {
   module map {
     header "map"
     export initializer_list
     export *
   }
   module memory {
     header "memory"
     export *
+
+    module __memory {
+      module addressof                { header "__memory/addressof.h"                }
+      module allocation_guard         { header "__memory/allocation_guard.h"         }
+      module allocator_traits         { header "__memory/allocator_traits.h"         }
+      module allocator                { header "__memory/allocator.h"                }
+      module auto_ptr                 { header "__memory/auto_ptr.h"                 }
+      module compressed_pair          { header "__memory/compressed_pair.h"          }
+      module construct_at             { header "__memory/construct_at.h"             }
+      module pointer_safety           { header "__memory/pointer_safety.h"           }
+      module pointer_traits           { header "__memory/pointer_traits.h"           }
+      module raw_storage_iterator     { header "__memory/raw_storage_iterator.h"     }
+      module shared_ptr               { header "__memory/shared_ptr.h"               }
+      module temporary_buffer         { header "__memory/temporary_buffer.h"         }
+      module uninitialized_algorithms { header "__memory/uninitialized_algorithms.h" }
+      module unique_ptr               { header "__memory/unique_ptr.h"               }
+    }
   }
   module mutex {
     header "mutex"
     export *
   }
   module new {
     header "new"
     export *
module queue {
     export *
   }
   module random {
     header "random"
     export initializer_list
     export *
   }
   module ranges {
+    requires cplusplus20
     header "ranges"
     export compare
     export initializer_list
     export iterator
     export *
+
+    module __ranges {
+      module access                 { header "__ranges/access.h"                 }
+      module concepts               { header "__ranges/concepts.h"               }
+      module data                   { header "__ranges/data.h"                   }
+      module empty                  { header "__ranges/empty.h"                  }
+      module enabled_borrowed_range { header "__ranges/enabled_borrowed_range.h" }
+      module size                   { header "__ranges/size.h"                   }
+      module view_interface         { header "__ranges/view_interface.h"         }
+      module view                   { header "__ranges/view.h"                   }
+    }
   }
   module ratio {
     header "ratio"
     export *
   }
   module regex {
     header "regex"
     export initializer_list
module unordered_set {
     header "unordered_set"
     export initializer_list
     export *
   }
   module utility {
     header "utility"
     export initializer_list
     export *
+
+    module __utility {
+      module to_underlying {
+        header "__utility/to_underlying.h"
+      }
+    }
   }
   module valarray {
     header "valarray"
     export initializer_list
     export *
   }
   module variant {
     header "variant"
     export *
   }
   module vector {
     header "vector"
     export initializer_list
     export *
   }
   module version {
     header "version"
     export *
   }
 
   // FIXME: These should be private.
-  module __bit_reference { header "__bit_reference" export * }
-  module __bits { header "__bits" export * }
-  module __debug { header "__debug" export * }
-  module __errc { header "__errc" export * }
-  module __functional_base { header "__functional_base" export * }
-  module __hash_table { header "__hash_table" export * }
-  module __locale { header "__locale" export * }
-  module __mutex_base { header "__mutex_base" export * }
-  module __node_handle { header "__node_handle" export * }
-  module __split_buffer { header "__split_buffer" export * }
-  module __std_stream { header "__std_stream" export * }
-  module __string { header "__string" export * }
-  module __threading_support { header "__threading_support" export * }
-  module __tree { header "__tree" export * }
-  module __tuple { header "__tuple" export * }
-  module __undef_macros { header "__undef_macros" export * }
+  module __availability       { header "__availability"       export * }
+  module __bit_reference      { header "__bit_reference"      export * }
+  module __bits               { header "__bits"               export * }
+  module __config             { header "__config"             export * }
+  module __debug              { header "__debug"              export * }
+  module __errc               { header "__errc"               export * }
+  module __function_like      { header "__function_like.h"    export * }
+  module __functional_base    { header "__functional_base" export * }
+  // TODO: modularise __functional_(base_)?03
+  module __hash_table         { header "__hash_table"         export * }
+  // libcpp_version not modularised since it's just a version number
+  module __locale             { header "__locale"             export * }
+  module __mutex_base         { header "__mutex_base"         export * }
+  module __node_handle        { header "__node_handle"        export * }
+  module __nullptr            { header "__nullptr"            export * }
+  module __split_buffer       { header "__split_buffer"       export * }
+  module __std_stream         { header "__std_stream"         export * }
+  module __string             { header "__string"             export * }
+  module __threading_support  { header "__threading_support"  export * }
+  module __tree               { header "__tree"               export * }
+  module __tuple              { header "__tuple"              export * }
+  module __undef_macros       { header "__undef_macros"       export * }
 
   module experimental {
     requires cplusplus11
 
     module algorithm {
       header "experimental/algorithm"
       export *
     }