Differential D107895
[runtimes] Set more paths when building runtimes standalone Authored by mstorsjo on Aug 11 2021, 4:53 AM.
Details
These paths are needed when building with per-target runtime directories. (It's possible to fix this by manually setting these when invoking Also set LLVM_TOOLS_BINARY_DIR while touching this area (as it's
Diff Detail
Event TimelineComment Actions Can you describe how you're running the build? Are you using the runtimes/ directory as the top-level directory? I don't want to support yet another way of building the runtimes, there's already too many. If we add this one, it has to be better than the other alternatives and fix some issues so that we can remove support for the other ways to build. Comment Actions I’m running the building by pointing cmake at either libcxx or the llvm-projects/runtimes directory - both of them being supported configurations. But if enabling LLVM_ENABLE_PER_TARGET_RUNTIME_DIR in these configurations, these variables need to be set. Comment Actions That's the standalone build we're trying to get rid of, yeah.
Do you mean using llvm-projects/runtimes as the top-level of the CMake invocation? The supported ways to build libc++ are https://libcxx.llvm.org/BuildingLibcxx.html#the-default-build and https://libcxx.llvm.org/BuildingLibcxx.html#bootstrapping-build (and the standalone build, which is not documented anymore). I'm not trying to push back on this change just for the sake of it, I'm really trying to understand whether this adds yet another way to build libc++ or not. Comment Actions Yes
The per-runtime target directory layout isn't so much about a new way of building things - it's a boolean flag that applies on all ways of building libcxx (so given the current N ways of building it, technically there's 2*N ways to set it up), but it doesn't affect much how things are built - only how things are installed. Also, I don't think any of the current CI even tests the installed files, as the testsuite only uses the newly built files/headers in the buildtree - so even if we'd clone all the tests for the various configurations and add copies of them with the flag enabled, the CI tests would pass - they'd just be installing files in unexpected directories. And finally: I'm not adding a new way of building libc++ - it has already been added, in D45604. It's just not very commonly used yet, but D107799 attempts to enable it by default (in some setups). Comment Actions Do I need to update the patch to make it show up properly in the review queue again to follow up on the discussion, given that this flag has been set to mark it as handled by you? Comment Actions From my perspective this addresses issues in the existing build mode that we hope to make the default one in the future rather than adding a new one so LGTM. Comment Actions What's the current status of this patch? It looks like this will help us run compiler-rt runtimes test. Currently, sanitizer tests seem to use the outdated host llvm-symbolizer when it should be using a just-built llvm-symbolizer in the binary directory, which it looks like this sets by default with this patch. Comment Actions I guess I need to reset the review status (while there's no new patch to upload yet) to continue the discussion here? Comment Actions @mstorsjo Using llvm-projects/runtimes as the top-level directory is what I referred to as "a new way of building libc++". It's not currently supported. Note that I really think this is the correct way to build libc++ (and it should be one of the two supported ways, alongside with a proper boostrapping build where we build libc++ with the just-built Clang). I'd like us to set up a CI bot that builds this way, as a first step towards removing the other ways we have of building libc++. Does that make sense to you? Comment Actions Ah right. I think that makes sense as a long term goal yes. IIRC using that build directory doesn't support actually running tests right yet though. For just building, I think that directory works well enough now that I should be able to migrate to it from the old standalone build configuration. Comment Actions We are seeing 3 buildbot failures related to this patch. all 3 buildbots are in Staging i checkout the patch just before yours, built ok, checked out yours build issue i pasted my cmake line below our two openmp amdgpu builders: FAILED: openmp/libomptarget/plugins/amdgpu/CMakeFiles/omptarget.rtl.amdgpu.dir/impl/get_elf_mach_gfx_name.cpp.o ^~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated. /usr/local/cmake/bin/cmake -DCMAKE_INSTALL_PREFIX=/home/<somewhere>/trunk_1.0 \ -DCMAKE_BUILD_TYPE=Release \ -DLLVM_ENABLE_PROJECTS="clang;lld;llvm" \ -DLLVM_LIT_ARGS="-vv --show-unsupported --show-xfail -j 32" \ -DLLVM_TARGETS_TO_BUILD="X86;AMDGPU" \ -DLLVM_ENABLE_ASSERTIONS=ON \ -DLLVM_ENABLE_RUNTIMES="openmp" \ -DCLANG_DEFAULT_LINKER=lld \ ../llvm -GNinja ninja install Comment Actions A bisect seems to show this causing some build errors for us (https://ci.chromium.org/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8836617372491315633): [1074/1207] Building CXX object compiler-rt/lib/xray/CMakeFiles/RTXrayBASIC.x86_64.dir/xray_basic_logging.cpp.o
FAILED: compiler-rt/lib/xray/CMakeFiles/RTXrayBASIC.x86_64.dir/xray_basic_logging.cpp.o
/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/./bin/clang++ --target=x86_64-unknown-linux-gnu --sysroot=/usr/local/google/home/leonardchan/sysroot/linux-amd64/ -DXRAY_HAS_EXCEPTIONS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-1/compiler-rt/lib/xray/.. -I/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-1/compiler-rt/lib/xray/../../include --target=x86_64-unknown-linux-gnu -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -ffile-prefix-map=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/runtimes/runtimes-x86_64-unknown-linux-gnu-bins=../llvm-build-1-master-fuchsia-toolchain/runtimes/runtimes-x86_64-unknown-linux-gnu-bins -ffile-prefix-map=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-1/= -no-canonical-prefixes -Wall -std=c++14 -Wno-unused-parameter -O2 -g -DNDEBUG -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -Wno-format-pedantic -Wno-format -fno-rtti -MD -MT compiler-rt/lib/xray/CMakeFiles/RTXrayBASIC.x86_64.dir/xray_basic_logging.cpp.o -MF compiler-rt/lib/xray/CMakeFiles/RTXrayBASIC.x86_64.dir/xray_basic_logging.cpp.o.d -o compiler-rt/lib/xray/CMakeFiles/RTXrayBASIC.x86_64.dir/xray_basic_logging.cpp.o -c /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-1/compiler-rt/lib/xray/xray_basic_logging.cpp
In file included from /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-1/compiler-rt/lib/xray/xray_basic_logging.cpp:30:
/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-1/compiler-rt/lib/xray/../../include/xray/xray_records.h:19:10: fatal error: 'cstdint' file not found
#include <cstdint>
^~~~~~~~~Would you be able to send out a fix or revert? Thanks. Comment Actions Building openmp via LLVM_ENABLE_RUNTIMES="openmp" broke with this change. I think in two independent ways. Example failure from the nvptx offloading buildbot http://meinersbur.de:8011/#/builders/117/builds/682 [stdout] /$HOME/openmp-offload-cuda-runtime/llvm.src/llvm/include/llvm/Support/Compiler.h:18:10: fatal error: 'llvm/Config/llvm-config.h' file not found [stdout] #include "llvm/Config/llvm-config.h" It looks like elf_common.cpp is including files from the llvm include tree, but should be including them from the build (maybe install) tree. The include path is set to $HOME/llvm-project/llvm/include;$HOME/llvm-build/llvm/include by LibomptargetGetDependencies.cmake, based on ${LLVM_MAIN_INCLUDE_DIR} ${LLVM_BINARY_DIR}/include. Also, the amdgpu devicertl now fails to build - LLVM_DIR is set, so it uses find_program(CLANG_TOOL clang PATHS ${LLVM_TOOLS_BINARY_DIR} NO_DEFAULT_PATH), which resolves to CLANG_TOOL_NOTFOUND. At a guess LLVM_TOOLS_BINARY_DIR used to resolve to a path with clang on it. Unfortunately it fails to handle this gracefully, we're missing a test for clang == clang_tool_notfound or similar, but fixing that will still leave the devicertl library no longer compiling. Comment Actions Sorry for the breakage and thanks for reverting for me (it was nighttime for me), I'll try to reproduce those failures and look into why it failed. The patch to runtimes/CMakeLists.txt was supposed to not have any effect when building from the main llvm directory by adding -DLLVM_ENABLE_RUNTIMES=..., but apparently it still did. Comment Actions I was able to easily reproduce the breakage and understood the misunderstanding; when building runtimes as part of the main llvm build, by setting LLVM_ENABLE_RUNTIMES, it does a separate sub-invocation of cmake pointing at the llvm-project/runtimes directory, which the if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR) condition considered the same as if building entirely standalone, by pointing cmake at the llvm-project/runtimes directory manually. Instead check if LLVM_BINARY_DIR was set (if invoked by the main LLVM build, this parameter is explicitly passed from there) and if find_package(LLVM) succeeded (which sets LLVM_TOOLS_BINARY_DIR and LLVM_LIBRARY_DIR). Comment Actions We're still seeing issue on our bots even after reland. This is the standalone build of libc++ on macOS when targeting Apple Silicon when we use runtimes/CMakeLists.txt as the top-level file. The error is: CMake Error: Could not open file for write in copy operation /include/c++/v1/__config_site.tmp CMake Error: : System Error: No such file or directory CMake Error at /Users/phosek/llvm/llvm-project/libcxx/include/CMakeLists.txt:436 (configure_file): configure_file Problem configuring file I debugged this and it looks the problem is that in this case LLVM_BINARY_DIR is unset and so https://github.com/llvm/llvm-project/blob/704a39569346401e96a6a3978ddc490dfa828ccc/runtimes/CMakeLists.txt#L64 fails. Replacing that check: if (NOT LLVM_BINARY_DIR) seems to be working. Comment Actions Did that setup work for you before I landed the original version of this patch (when nothing was set) or is it just that my patch didn't quite work as intended? Comment Actions It did, I looked into it and it's because before the build would take this branch https://github.com/llvm/llvm-project/blob/4f9217c5194b10f8219613506f8d701a24650bbc/libcxx/CMakeLists.txt#L439 and CMAKE_BINARY_DIR would be set whereas now it takes https://github.com/llvm/llvm-project/blob/4f9217c5194b10f8219613506f8d701a24650bbc/libcxx/CMakeLists.txt#L427 and LLVM_BINARY_DIR is unset. Comment Actions Oh, that explains it indeed. Thanks for the fix, and sorry for not getting it right on the reland! |