C++23 stl modules#27065
Conversation
sbc100
left a comment
There was a problem hiding this comment.
Nice!
To be clear are these files taken from the emscripten-libs-21 branch?
We normally use the system/lib/update_libcxx.py script to update these files. Perhaps you could try using that (maybe it needs modifying to include the modules directory somehow?)
Also made copy_tree conditionally exclude files because we need the CMakeLists.txt for the modules
sbc100
left a comment
There was a problem hiding this comment.
lgtm!
But maybe we could add a test for this ?
Might need to get Clang-CXX-CXXImportStd working if the test environment doesn't support cmake 4.2 for CMAKE_CXX_STDLIB_MODULES_JSON . In this case we'd need to patch emcc.py to allow using a relative search path for -print-file-name with CMAKE_CXX_COMPILER_ID_ARG1 |
This is explicitly used by CMake's Clang-CXX-CXXImportStd.cmake
If we can get cmake 4.2 installed on at least one of the CI machines would that address the issue? I would be fine landing this change standalone and following up with a test too. |
| set(LIBCXX_INSTALL_LIBRARY_DIR "${LIBCXX_LIBRARY_DIR}") | ||
| set(LIBCXX_INSTALL_MODULES_DIR "${LIBCXX_GENERATED_MODULE_DIR}") | ||
|
|
||
| add_subdirectory("${EMSCRIPTEN_ROOT_PATH}/system/lib/libcxx/modules") |
There was a problem hiding this comment.
I think this should be referring to the installed version in the emscripten cache/sysroot not the version in source control.
There was a problem hiding this comment.
Yeah this implies relative paths to CMAKE_INSTALL_PREFIX = cache/sysroot.
There was a problem hiding this comment.
I think -print-file-name is already sysroot relative.
It looks like cmake expects clang -print-file-name=libc++.modules.json to print the file within the sysroot, like it does for libs.
It looks like libc++.modules.json is supposed to live in cache/sysroot/lib/wasm32-emscripten/ .. and the current code should then find it.
You might need to update install_system_headers so that the json files go in the right place.
There was a problem hiding this comment.
Yes it prints the correct path when installed in cache/sysroot/lib/wasm32-emscripten but CMake seems not to invoke the underlying function from Clang-CXX-CXXImportStd automatically for emscripten:
[cmake] CMake Error in CMakeLists.txt:
[cmake] "The "CXX_MODULE_STD" property on target "HelloWorld" requires
[cmake] CMAKE_CXX_STDLIB_MODULES_JSON be set, but it was not provided by the
[cmake] toolchain.So I guess the only alternative for CMake>3.30 would be using our own copy of the older Clang-CXX-CXXImportStd since they changed the underlying function.
There was a problem hiding this comment.
I'm not sure how Clang-CXX-CXXImportStd is supposed to be called.
Why don't we land this change (to add the new files) and then followup with another change to add test and figure out the Clang-CXX-CXXImportStd thing?
There was a problem hiding this comment.
I'm not sure how Clang-CXX-CXXImportStd is supposed to be called.
It can be explicitly called like:
include(Compiler/Clang-CXX-CXXImportStd)
_cmake_cxx_find_modules_json() # >4.2 Sets CMAKE_CXX_STDLIB_MODULES_JSON
_cmake_cxx_import_std(23 to_eval) # <4.2 Sets targets
But yeah lets figure that out later
| PROPERTY | ||
| COMPILE_FLAGS -Wno-reserved-module-identifier | ||
| ) | ||
| set(CMAKE_CXX_STDLIB_MODULES_JSON "${LIBCXX_LIBRARY_DIR}/libc++.modules.json") |
There was a problem hiding this comment.
I think if you install libc++.modules.json in the correct place in the sysroot then CMake should be able to find it without setting CMAKE_CXX_STDLIB_MODULES_JSON like this ?
…o CMAKE_INSTALL_PREFIX
This reverts commit 8b5827f.
needed for out of tree builds i.e. tests
| set(LIBCXX_LIBRARY_DIR "${CMAKE_BINARY_DIR}/${LIBCXX_BINARY_DIR}/${LIBCXX_INSTALL_LIBRARY_DIR}") | ||
| set(LIBCXX_GENERATED_MODULE_DIR "${CMAKE_BINARY_DIR}/${LIBCXX_BINARY_DIR}/${LIBCXX_INSTALL_MODULES_DIR}") | ||
|
|
||
| add_subdirectory("${EMSCRIPTEN_ROOT_PATH}/system/lib/libcxx/modules" ${LIBCXX_BINARY_DIR}) |
There was a problem hiding this comment.
I would imagine we can avoid referencing system/lib/ completely no? Shouldn't we just refer to the installed files in the sysroot? What is this line required for?
There was a problem hiding this comment.
Can we install them there some other way? These lines are meant to generate the libc++.modules.json and the module interfaces from there in the first place.
There was a problem hiding this comment.
Oh I see.. so the libc++.modules.json is not checked in but generated automatically on first use?
How does this work in normal/desktop systems?
I would have through we would install the modules directory as part of the install_system_headers step and then point to them there? (install_system_headers is a little misnames since it installs all kind of things really).
There was a problem hiding this comment.
Right, so that means we should "install" them into the sysroot, no? Since that is our version of "shipping" here I think.
There was a problem hiding this comment.
(We don't use libc++'s install logic but reproduce it instead in install_system_headers)
There was a problem hiding this comment.
So to build them in the purview of install_system_headers instead of Emscripten.cmake we'd need to add a little cmake shim project I suppose.
For the cmake side of things this wouldn't strictly be necessary though since we can just set CMAKE_CXX_STDLIB_MODULES_JSON for CMake > 4.2
This adds the libcxx/modules folder from https://github.com/emscripten-core/llvm-project needed for CMake to have the std and std.compat module interfaces generated during configuration.
At the moment emscripten only needs to roll the updated llvm headers to successfully compile those for c++23.
For c++26 we need to implement std::ranges::views::indices or comment out
https://github.com/Diyou/emscripten/blob/922e9aa8b84613afbb1651459054b1c60b55e69b/system/lib/libcxx/modules/std/ranges.inc#L120
Resolves #21143