[libcxx-commits] [PATCH] D145017: [libc++][libc++abi] Make libc++ includes more compatible with its own modulemap

[Alan Zhao via Phabricator via libcxx-commits]

ayzhao added inline comments.


================
Comment at: libcxx/src/filesystem/operations.cpp:17
 #include <type_traits>
+#include <utility>
 #include <vector>
----------------
Mordante wrote:
> philnik wrote:
> > I don't think we want to do this. In fact, we should go the other way and use private headers instead IMO. Instead, we should pass `-fmodule-name=std` to let clang know that these files implement the module.
> +1 I think we either need to add the missing granularized headers or the module map is misses exports for the granularized headers. I've seen the latter several times while using granularized headers in `<format>`.
I'm not sure what the justification is for including internal private header files in source files.

The original reason for splitting up the header files was to decouple the entities within each header to prevent cyclic dependencies and transitive includes. However, source files don't suffer from this problem because users don't include them.

As an aside, I tried @philnik's suggestion to use `-fmodule-name=absl` and I ended up with some bizarre failures - for example

```
/path/to/llvm-project/build/bin/clang++ -DLIBCXXABI_SILENT_TERMINATE -DDCHECK_ALWAYS_ON=1  -D_LIBCPP_ENABLE_ASSERTIONS=1 -D_LIBCPP_BUILDING_LIBRARY -I../../buildtools/third_party/libc++/trunk/src -I../.. -Igen -I../../buildtools/third_party/libc++  -fmodules -fbuiltin-module-map -fmodules-cache-path=module_cache -Xclang -fmodules-local-submodule-visibility -O2 -fmodule-name=std -fvisibility=default -std=c++20 -nostdinc++ -isystem../../buildtools/third_party/libc++/trunk/include -isystem../../buildtools/third_party/libc++abi/trunk/include -std=c++20 -c ../../buildtools/third_party/libc++abi/trunk/src/cxa_exception.cpp -o obj/buildtools/third_party/libc++abi/libc++abi/cxa_exception.o
../../buildtools/third_party/libc++abi/trunk/src/cxa_exception.cpp:655:14: error: no member named '__libcpp_atomic_add' in namespace 'std'
        std::__libcpp_atomic_add(&exception_header->referenceCount, size_t(1));
        ~~~~~^
../../buildtools/third_party/libc++abi/trunk/src/cxa_exception.cpp:672:18: error: no member named '__libcpp_atomic_add' in namespace 'std'
        if (std::__libcpp_atomic_add(&exception_header->referenceCount, size_t(-1)) == 0)
            ~~~~~^
2 errors generated.
```

[cxa_exception.cpp](https://github.com/llvm/llvm-project/blob/main/libcxxabi/src/cxa_exception.cpp) includes [atomic_support.h](https://github.com/llvm/llvm-project/blob/main/libcxx/src/include/atomic_support.h), which defines the missing symbols in question (albeit in an anonymous namespace). The above compile command runs without error if I either remove `-fmodule-name=std` or the anonymous namespace in atomic_support.h.

(note: I was hacking in Chrome's source tree, and buildtools/third_party/libc++/trunk and buildtools/third_party/libc++/trunk corresponds to llvm-project/libcxx and llvm-project/libcxxabi respectively. There are no patches to libc++, and we follow upstream very closely)




================
Comment at: libcxx/test/libcxx/transitive_includes/cxx2b.csv:165
 expected version
+experimental/algorithm algorithm
+experimental/algorithm cstddef
----------------
philnik wrote:
> This seems wrong. Maybe a broken rebase?
Everything seems to be clean, and the test did pass:

```
commit 1e1e4cab167a179ee5218f056bbafb017f9b4180 (HEAD -> bugfix/libcpp-includes)
Author: Alan Zhao <ayzhao at google.com>
Date:   Tue Feb 28 14:56:01 2023 -0800

    [libc++][libc++abi] Make libc++ includes more compatible with its own modulemap

    Currently, libc++ itself cannot be built with its own modulemap file.
    This patch fixes includes in the implementation source files as follows:
    * Replace includes of private headers with their public counterparts.
    * Fix several missing imports.
    * Move __std_stream out of the modulemap and into a header file in
      src/include as it's only used in iostream.cpp

    Note that this patch isn't sufficient to allow compiling libc++ with
    it's own modulemap - right now, clang still complains that iostream.cpp
    is redefining several symbols in the iostream header with different
    types.

    Differential Revision: https://reviews.llvm.org/D145017

commit 0963833a194331a8b8d6775dcd1c3025a8154751 (origin/main, origin/HEAD, main)
Author: Lang Hames <lhames at gmail.com>
Date:   Wed Mar 1 15:34:23 2023 -0800

    [ExecutionEngine] Silence warnings about sprintf use in interpreter.

    We should review memory safety in the interpreter
    (https://github.com/llvm/llvm-project/issues/58086), but for now just silence
    the warnings to reduce noise.

    rdar://100555195

```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145017/new/

https://reviews.llvm.org/D145017