This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ayzhao on Feb 28 2023, 3:45 PM.

Details

Reviewers
ldionne
philnik
Group Reviewers
Restricted Project
Restricted Project
Summary

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.

Diff Detail

Event Timeline

ayzhao created this revision.Feb 28 2023, 3:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2023, 3:45 PM
Herald added a subscriber: arichardson. · View Herald Transcript
ayzhao requested review of this revision.Feb 28 2023, 3:45 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 28 2023, 3:45 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
ayzhao updated this revision to Diff 501327.Feb 28 2023, 4:02 PM

fix up the history a little bit so (hopefully) git will recognize the rename

philnik requested changes to this revision.Mar 1 2023, 7:54 AM
philnik added a subscriber: philnik.

Thanks for working on this! I think it would be quite nice to be able to build libc++ with modules enabled, but I'm not sure it will actually be possible, since we define some classes differently in the source files to provide old symbols.

libcxx/src/filesystem/operations.cpp
17

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.

libcxx/test/libcxx/transitive_includes/cxx2b.csv
165

This seems wrong. Maybe a broken rebase?

This revision now requires changes to proceed.Mar 1 2023, 7:54 AM
Mordante added inline comments.
libcxx/src/filesystem/operations.cpp
17

+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>.

ayzhao added inline comments.Mar 1 2023, 4:51 PM
libcxx/src/filesystem/operations.cpp
17

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 includes 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)

libcxx/test/libcxx/transitive_includes/cxx2b.csv
165

Everything seems to be clean, and the test did pass:

commit 1e1e4cab167a179ee5218f056bbafb017f9b4180 (HEAD -> bugfix/libcpp-includes)
Author: Alan Zhao <ayzhao@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@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
philnik added inline comments.Mar 1 2023, 5:08 PM
libcxx/src/filesystem/operations.cpp
17

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.

libcxx/test/libcxx/transitive_includes/cxx2b.csv
165

experimental/functional and experimental/algorithm don't exist anymore. The CI is currently failing for unrelated reasons though (you have to add the moved file to libcxx/utils/data/ignore_format.txt).

Mordante added inline comments.Mar 2 2023, 11:42 AM
libcxx/src/filesystem/operations.cpp
17

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.

ayzhao added inline comments.Mar 2 2023, 4:53 PM
libcxx/src/filesystem/operations.cpp
17

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
ayzhao abandoned this revision.Mar 20 2023, 4:08 PM

Abandoning this revision as right now we're working on

libcxx/include/CMakeLists.txt