[libcxx-commits] [PATCH] D145017: [libc++][libc++abi] Make libc++ includes more compatible with its own modulemap
Alan Zhao via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Mar 2 16:53:47 PST 2023
ayzhao added inline comments.
================
Comment at: libcxx/src/filesystem/operations.cpp:17
#include <type_traits>
+#include <utility>
#include <vector>
----------------
Mordante wrote:
> philnik wrote:
> > ayzhao wrote:
> > > 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)
> > >
> > >
> > My justification would be that the source files wouldn't have to be rebuilt nearly as often as they have to be right now. Currently it's almost always rebuilding, even if you didn't change anything near the source files.
> >
> > I think the right way to fix the problem is to remove the inline namespace. We build with `-fvisibility=hidden` anyways, so they should never be exported from the dylib. I'd suggest moving that to it's own patch, since it's a good idea either way. I suspect that clang just doesn't emit them, since the functions are never used inside `atomic_support.h`.
> My reasoning is, when they don't work properly when included in our dylib, there might be errors in our module map that users might be able to trigger. This entire module map is handcrafted, and I don't believe it's free of errors. So any error we can fix would be a win.
> My justification would be that the source files wouldn't have to be rebuilt nearly as often as they have to be right now. Currently it's almost always rebuilding, even if you didn't change anything near the source files.
I'm not sure why increased rebuilding is a problem. libc++ builds relatively quickly, especially compared to, for example, Clang. On my machine, if I restrict parallelism to `-j2`, a clean build of cxx, cxxabi, and unwind takes under 50 seconds:
```
$ perf stat ninja -j2 cxx cxxabi unwind
[1040/1040] Linking CXX static library lib/libc++.a
Performance counter stats for 'ninja -j2 cxx cxxabi unwind':
98,579.13 msec task-clock:u # 1.973 CPUs utilized
0 context-switches:u # 0.000 /sec
0 cpu-migrations:u # 0.000 /sec
2,850,617 page-faults:u # 28.917 K/sec
345,837,832,337 cycles:u # 3.508 GHz (83.68%)
22,625,691,029 stalled-cycles-frontend:u # 6.54% frontend cycles idle (83.41%)
30,348,580,219 stalled-cycles-backend:u # 8.78% backend cycles idle (83.28%)
401,110,951,625 instructions:u # 1.16 insn per cycle
# 0.08 stalled cycles per insn (83.76%)
78,341,920,950 branches:u # 794.711 M/sec (84.00%)
2,727,072,658 branch-misses:u # 3.48% of all branches (83.67%)
49.964402208 seconds time elapsed
87.267475000 seconds user
11.334668000 seconds sys
```
Also, compiling with `-fmodule-name=std` continues to add a glut of weird error messages - 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../.. -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 -c ../../buildtools/third_party/libc++/trunk/src/memory.cpp -o obj/buildtools/third_party/libc++/libc++/memory.o 2>&1 | zpastebin
In file included from ../../buildtools/third_party/libc++/trunk/src/memory.cpp:17:
In file included from ../../buildtools/third_party/libc++/trunk/include/mutex:192:
In file included from ../../buildtools/third_party/libc++/trunk/include/__mutex_base:20:
In file included from ../../buildtools/third_party/libc++/trunk/include/system_error:153:
In file included from ../../buildtools/third_party/libc++/trunk/include/ios:221:
../../buildtools/third_party/libc++/trunk/include/__locale:206:5: error: unknown type name 'once_flag'; did you mean '__once_flag'?
once_flag __flag_;
# more random errors
```
I imported `ios` into `system_error` because Clang was complaining that `io_errc` must be imported from `std.ios`. The missing `once_flag` symbol in `__locale` is defined in `mutex`, so in this case, we end up with circular includes. For some reason, this isn't an issue without `-fmodule-name=std`.
I should also add that for some reason `-fmodule-name=std` seems to completely break resolution of symbols inside anonymous namespaces, even within the same file:
```
$ /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../.. -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 -c ../../buildtools/third_party/libc++/trunk/src/string.cpp -o obj/buildtools/third_party/libc++/libc++/string.o
../../buildtools/third_party/libc++/trunk/src/string.cpp:226:12: error: use of undeclared identifier 'as_integer'
return as_integer<int>("stoi", str, idx, base);
^
../../buildtools/third_party/libc++/trunk/src/string.cpp:230:12: error: use of undeclared identifier 'as_integer'
return as_integer<long>("stol", str, idx, base);
^
../../buildtools/third_party/libc++/trunk/src/string.cpp:234:12: error: use of undeclared identifier 'as_integer'
return as_integer<unsigned long>("stoul", str, idx, base);
# plus more of the same error
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145017/new/
https://reviews.llvm.org/D145017
More information about the libcxx-commits
mailing list