[ATfL][build] Add the ability to bypass the bootstrap building with a provided bootstrap compiler - #941
Conversation
| if [[ -e "${BOOTSTRAP_COMPILER_DIR}/bin/clang++" ]]; then | ||
| echo "Using the existing bootstrap compiler." | ||
| export PATH="${BOOTSTRAP_COMPILER_DIR}/bin:${PATH}" | ||
| bootstrap_compiler_default_config |
There was a problem hiding this comment.
I don't think it is good practice to modify the configuration file of an externally provided toolchain. Also USE_BOOTSTRAP_CMAKE_FLAGS already uses the CMake setting -DLLVM_ENABLE_LLD=ON to ensure that the lld linker is used by the subsequent stages. So I think bootstrap_compiler_default_config is unnecessary and we can completely remove bootstrap_compiler_default_config from this script. That also will remove the requirement of the bootstrap compiler location having to be writable.
There was a problem hiding this comment.
Sadly, this git log does not go that far back so there's no log of the justification behind forcing to use LLD the most brutal way ever. We may first try to introduce an PR which lifts that and see if anything fails (could be the reason behind it was to workaround some upstream issue haunting the building process at that time) and if it is not failing, we would effectively get rid of it.
There was a problem hiding this comment.
Yeah that makes sense. Let's test that cleanup separately ASAP.
There was a problem hiding this comment.
BTW, this bootstrap compiler directory still needs to be writable, there's no way to pass LD_LIBRARY_PATH to lit tests executed by check-all anymore (there was an long an annoying discussion why there is no going back to this), so forcing the use of Rpath in the compiler's config file is the only way to do it:
echo "-Wl,-rpath=${ATFL_DIR}/lib" >> "${BUILD_DIR}"/bootstrap_compiler/bin/clang++.cfg
run_test_command "${LOGS_DIR}/product_check_all.xml" "${LOGS_DIR}/product.txt" check-all
There was a problem hiding this comment.
Yeah that makes sense. Let's test that cleanup separately ASAP.
There was a problem hiding this comment.
Apparently, -DLLVM_ENABLE_LLD=ON applies only to build time, and the default linker (GNU ld) is being used when testing. And this reveals two problems in RHEL8: one is that GNU ld needs explicit -ldl for many of the libcxx test files, and second one is more worrying, the code linked with GNU ld fails with exceptions. And the second issue cannot be solved by adding any more flags, it just needs to be linked with lld to make it pass on RHEL8.
I'm not sure how much we want to invest in solving it. If not much, then we'd better revert it.
| export PATH="${BUILD_DIR}/bootstrap_compiler/bin:${PATH}" | ||
| export PATH="${BOOTSTRAP_COMPILER_DIR}/bin:${PATH}" | ||
| bootstrap_compiler_default_config | ||
| run_test_command "${LOGS_DIR}/bootstrap_check_all.xml" "${LOGS_DIR}/bootstrap_compiler.txt" check-all |
There was a problem hiding this comment.
On early return when an external bootstrap compiler is used, "${LOGS_DIR}/bootstrap_check_all.xml" will not be created. However check_lit_xml_results unconditionally expects "${LOGS_DIR}/bootstrap_check_all.xml" to be there and fails if it does not exist.
There was a problem hiding this comment.
I've added a line for XML file creation, and also rebased this PR so it could be merged cleanly.
There was a problem hiding this comment.
Well, not rebased to the top, as the topmost change needs to be verified first.
… provided bootstrap compiler This is to avoid the issues caused by the aging GCC versions by removing the GCC compiler from the picture.
b24956a to
ff30a97
Compare
2 participants
This is to avoid the issues caused by the aging GCC versions by removing the GCC compiler from the picture.