Page MenuHomePhabricator

[runtimes] Set more paths when building runtimes standalone
ClosedPublic

Authored by mstorsjo on Aug 11 2021, 4:53 AM.

Details

Summary

These paths are needed when building with per-target runtime directories.

(It's possible to fix this by manually setting these when invoking
cmake, but one isn't supposed to need to do that.)

Also set LLVM_TOOLS_BINARY_DIR while touching this area (as it's
also unset in this case) even if it isn't specifically needed by the
per-target runtime configuration.

Diff Detail

Event Timeline

mstorsjo created this revision.Aug 11 2021, 4:53 AM
mstorsjo requested review of this revision.Aug 11 2021, 4:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2021, 4:53 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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.

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.

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.

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.

I’m running the building by pointing cmake at either libcxx

That's the standalone build we're trying to get rid of, yeah.

or the llvm-projects/runtimes directory

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.

ldionne requested changes to this revision.Aug 24 2021, 1:37 PM

Requesting changes so it shows up properly in the review queue.

This revision now requires changes to proceed.Aug 24 2021, 1:37 PM

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.

I’m running the building by pointing cmake at either libcxx

That's the standalone build we're trying to get rid of, yeah.

or the llvm-projects/runtimes directory

Do you mean using llvm-projects/runtimes as the top-level of the CMake invocation?

Yes

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.

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

Requesting changes so it shows up properly in the review queue.

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?

phosek accepted this revision.Aug 24 2021, 6:22 PM

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.

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.

mstorsjo requested review of this revision.Sep 8 2021, 5:46 AM

Requesting changes so it shows up properly in the review queue.

I guess I need to reset the review status (while there's no new patch to upload yet) to continue the discussion here?

ldionne accepted this revision.Sep 8 2021, 8:45 AM

@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?

This revision is now accepted and ready to land.Sep 8 2021, 8:45 AM

@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?

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.

ronlieb added a subscriber: ronlieb.Sep 8 2021, 4:18 PM

We are seeing 3 buildbot failures related to this patch. all 3 buildbots are in Staging
unfortunately bots down last 4 days , so this may not be as timely as you might hope for

i checkout the patch just before yours, built ok, checked out yours build issue
i also checked out main latest, and reverted and it builds ok

i pasted my cmake line below

our two openmp amdgpu builders:
https://lab.llvm.org/staging/#/builders/183/builds/1057
https://lab.llvm.org/staging/#/builders/182/builds/1264
and Michael Kruse's builder
https://lab.llvm.org/staging/#/builders/182/builds/1264

FAILED: openmp/libomptarget/plugins/amdgpu/CMakeFiles/omptarget.rtl.amdgpu.dir/impl/get_elf_mach_gfx_name.cpp.o
/home/rlieberm/mono-repo/llvm-project/build/./bin/clang++ --target=x86_64-unknown-linux-gnu -DTARGET_NAME=AMDGPU -D_DEBUG -DSTDC_CONSTANT_MACROS -DSTDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Domptarget_rtl_amdgpu_EXPORTS -I/home/rlieberm/mono-repo/llvm-project/openmp/libomptarget/include -I/home/rlieberm/mono-repo/llvm-project/openmp/libomptarget/plugins/amdgpu/impl -I/home/rlieberm/mono-repo/llvm-project/llvm/include -Iinclude -I/home/rlieberm/mono-repo/llvm-project/openmp/libomptarget/plugins/common/elf_common -isystem /opt/rocm/include/hsa -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 -Wall -Wcast-qual -Wformat-pedantic -Wimplicit-fallthrough -Wsign-compare -Wno-extra -Wno-pedantic -std=c++14 -O3 -DNDEBUG -fPIC -UNDEBUG -MD -MT openmp/libomptarget/plugins/amdgpu/CMakeFiles/omptarget.rtl.amdgpu.dir/impl/get_elf_mach_gfx_name.cpp.o -MF openmp/libomptarget/plugins/amdgpu/CMakeFiles/omptarget.rtl.amdgpu.dir/impl/get_elf_mach_gfx_name.cpp.o.d -o openmp/libomptarget/plugins/amdgpu/CMakeFiles/omptarget.rtl.amdgpu.dir/impl/get_elf_mach_gfx_name.cpp.o -c /home/rlieberm/mono-repo/llvm-project/openmp/libomptarget/plugins/amdgpu/impl/get_elf_mach_gfx_name.cpp
In file included from /home/rlieberm/mono-repo/llvm-project/openmp/libomptarget/plugins/amdgpu/impl/get_elf_mach_gfx_name.cpp:13:
In file included from /home/rlieberm/mono-repo/llvm-project/llvm/include/llvm/BinaryFormat/ELF.h:22:
In file included from /home/rlieberm/mono-repo/llvm-project/llvm/include/llvm/ADT/StringRef.h:12:
In file included from /home/rlieberm/mono-repo/llvm-project/llvm/include/llvm/ADT/STLExtras.h:19:
In file included from /home/rlieberm/mono-repo/llvm-project/llvm/include/llvm/ADT/Optional.h:18:
In file included from /home/rlieberm/mono-repo/llvm-project/llvm/include/llvm/ADT/Hashing.h:48:
In file included from /home/rlieberm/mono-repo/llvm-project/llvm/include/llvm/Support/ErrorHandling.h:17:
/home/rlieberm/mono-repo/llvm-project/llvm/include/llvm/Support/Compiler.h:18:10: fatal error: 'llvm/Config/llvm-config.h' file not found
#include "llvm/Config/llvm-config.h"

^~~~~~~~~~~~~~~~~~~~~~~~~~~

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

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.

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.
I'm not confident that LLVM_MAIN_INCLUDE_DIR is a good thing to be passing as an include path, so might be able to fix it by changing that, but the interaction with this patch is unclear.

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.

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.

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

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.

I have uploaded D109570 for review with the proposed fix.

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.

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?

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?

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.

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?

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.

Oh, that explains it indeed. Thanks for the fix, and sorry for not getting it right on the reland!