Page MenuHomePhabricator

[Driver] Also search FilePaths for compiler-rt before falling back
Needs ReviewPublic

Authored by jrtc27 on Mar 6 2021, 7:35 AM.

Details

Summary

LibraryPaths is rather underused in Clang, unlike FilePaths. The latter
notably includes the sysroot for bare-metal toolchains, so this allows
compiler-rt to be found alongside the other libraries shipped as part of
a sysroot rather than requiring it to be bundled with Clang, allowing
bare-metal sysroots to behave much more like real OS sysroots.

This is particularly useful for RISC-V where there is an abundance of
possible ISA and ABI choices, but the architecture suffix added to the
library name only captures the architectural word size. Putting them in
ISA and ABI-specific sysroots is the easiest way to allow multiple
configurations to coexist on the system given that such sysroots already
must exist with the system headers and precompiled libc.

Diff Detail

Unit TestsFailed

TimeTest
120 msx64 windows > Clang.Driver::baremetal-sysroot-with-compiler-rt.c
Script: -- : 'RUN: at line 1'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\clang.exe -target riscv64 --sysroot=C:\ws\w16c2-1\llvm-project\premerge-checks\clang\test\Driver/Inputs/baremetal_sysroot_with_compiler_rt -rtlib=compiler-rt -print-libgcc-file-name | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16c2-1\llvm-project\premerge-checks\clang\test\Driver\baremetal-sysroot-with-compiler-rt.c
840 msx64 windows > Clang.Driver::print-libgcc-file-name-clangrt.c
Script: -- : 'RUN: at line 3'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\clang.exe -rtlib=compiler-rt -print-libgcc-file-name 2>&1 --target=x86_64-pc-linux -resource-dir=C:\ws\w16c2-1\llvm-project\premerge-checks\clang\test\Driver/Inputs/resource_dir_with_arch_subdir | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\filecheck.exe --check-prefix=CHECK-CLANGRT-X8664 C:\ws\w16c2-1\llvm-project\premerge-checks\clang\test\Driver\print-libgcc-file-name-clangrt.c

Event Timeline

jrtc27 created this revision.Mar 6 2021, 7:35 AM
jrtc27 requested review of this revision.Mar 6 2021, 7:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2021, 7:35 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jrtc27 updated this revision to Diff 328771.Mar 6 2021, 9:16 AM

Added missing file for test

jrtc27 updated this revision to Diff 328772.Mar 6 2021, 9:17 AM

Dropped unintended change

jrtc27 updated this revision to Diff 328773.Mar 6 2021, 9:18 AM

... and another accidental change

jrtc27 added a comment.Mar 6 2021, 9:25 AM

One thing that is currently rather ugly about BareMetal is that it ignores the AddArch argument. Once you have a sysroot, the architecture suffix is rather unnecessary, and given compiler-rt uses LLVM_ENABLE_PER_TARGET_RUNTIME_DIR to decide both whether to add a suffix and whether to add lib/${OS} to the install path, that means you can't nicely point compiler-rt at the sysroot as the install directory (well, unless you do something like make COMPILER_RT_OUTPUT_DIR point at the sysroot not the libdir and set COMPILER_RT_OS_DIR to . (or maybe the empty string also works)).

Ideally BareMetal would honour AddArch like the other toolchains, but you then have several issues:

  1. Are people relying on AddArch _not_ being honoured for things added to LibraryPaths (and therefore you need to check for both)?
  1. AddLinkRuntimeLib similarly hard-codes the fact that the architecture suffix is added. Changing it to use getCompilerRTArgString might work, but does subtly change things as then the path is provided to the linker, not a -l argument.
abidh added a comment.Mar 8 2021, 3:42 AM

One thing that is currently rather ugly about BareMetal is that it ignores the AddArch argument. Once you have a sysroot, the architecture suffix is rather unnecessary, and given compiler-rt uses LLVM_ENABLE_PER_TARGET_RUNTIME_DIR to decide both whether to add a suffix and whether to add lib/${OS} to the install path, that means you can't nicely point compiler-rt at the sysroot as the install directory (well, unless you do something like make COMPILER_RT_OUTPUT_DIR point at the sysroot not the libdir and set COMPILER_RT_OS_DIR to . (or maybe the empty string also works)).

Agree that setting the compiler-rt installation directory is not ideal. I remember I ended up doing some post installation steps to make sure that libraries were in right folder.

Regarding this patch, currently Baremetal toolchain looks for compiler-rt in a multilib specific directory. Does that not solve the problem of finding compiler-rt for any given arch/abi combination automatically?

jrtc27 added a comment.Mar 8 2021, 8:32 AM

One thing that is currently rather ugly about BareMetal is that it ignores the AddArch argument. Once you have a sysroot, the architecture suffix is rather unnecessary, and given compiler-rt uses LLVM_ENABLE_PER_TARGET_RUNTIME_DIR to decide both whether to add a suffix and whether to add lib/${OS} to the install path, that means you can't nicely point compiler-rt at the sysroot as the install directory (well, unless you do something like make COMPILER_RT_OUTPUT_DIR point at the sysroot not the libdir and set COMPILER_RT_OS_DIR to . (or maybe the empty string also works)).

Agree that setting the compiler-rt installation directory is not ideal. I remember I ended up doing some post installation steps to make sure that libraries were in right folder.

Regarding this patch, currently Baremetal toolchain looks for compiler-rt in a multilib specific directory. Does that not solve the problem of finding compiler-rt for any given arch/abi combination automatically?

So, technically yes for RISC-V I believe we can do that. However that support code only really exists for RISC-V, and we have other architectures too we need to extend. This approach is entirely general with no lists of arch-specific combinations needed, and far more flexible. I also believe that conceptually compiler-rt does belong in the sysroot, not with the compiler. The libgcc-with-gcc model kind of makes sense for GCC given you have to configure GCC for a specific architecture, but for Clang there is no such coupling. This allows you to, say, use a system-provided Clang with your own bare-metal sysroot (not that that's our use-case, but it's a sensible one). The other way to see that compiler-rt doesn't really belong with the compiler is to look at the bootstrapping process: compiler-rt depends on a libc, typically newlib, which then depends on your compiler. So you have to build Clang, then build newlib, then go back and build compiler-rt to add it to your Clang. That gets rather annoying to manage in CI jobs.

This makes sense to me but I'm not quite sure about the implications, especially when we consider compatibility. I think we need more eyes on this patch.

I just looked at this again and I don't have the full context in my mind right now but won't the test just exercise the BareMetal toolchain and not your changes?

I just looked at this again and I don't have the full context in my mind right now but won't the test just exercise the BareMetal toolchain and not your changes?

I've since lost my recollection of all the details, but BareMetal::AddLinkRuntimeLib is hard-coding -lclang_rt.builtins-$ARCH so I think we already do search in the sysroot for the library if it doesn't exist in the resources directory. What this tests is -print-libgcc-file-name (implemented in the common Driver.cpp) which is the thing that is currently "broken" for bare-metal toolchains as it will currently neglect to search the sysroot as the linker would, since it calls getCompilerRT directly (which BareMetal doesn't override).