Page MenuHomePhabricator

[RISCV] Use compiler-rt if no GCC installation detected
Needs ReviewPublic

Authored by edward-jones on Oct 3 2019, 10:11 AM.

Details

Summary

If a GCC installation is not detected, then this attempts to use compiler-rt and the compiler-rt crtbegin/crtend implementations as a fallback.

Diff Detail

Event Timeline

edward-jones created this revision.Oct 3 2019, 10:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2019, 10:11 AM

Work in progress because this needs tests (and probably clang-format too).

My assumption is that libgcc should be used in preference if it is available. At the moment this either uses libgcc and libgcc's crtbegin/crtend, or the Clang equivalents. Potentially a user might want to mix compiler-rt builtins with libgcc's crt*.o files, but for now that's not possible.

edward-jones retitled this revision from [WIP][RISCV] Use compiler-rt if no GCC installation detected to [RISCV] Use compiler-rt if no GCC installation detected.

Rebased, updated tests from D68391 to check for existence of compiler-rt crtbegin/crtend and runtime library.

lenary requested changes to this revision.Oct 15 2019, 3:37 AM

Please can you add a test for riscv32 and riscv64 without libgcc?

I also think you want to be smarter about detecting the need/request for libgcc. Look for AddRunTimeLibs in clang/lib/Driver/ToolChains/CommonArgs.cpp - I think that will get you quite far. I think you can override getCompilerRTArgString for your toolchain, and also GetDefaultRuntimeLibType. For the correct compiler-rt stuff, you want ToolChain::getCompilerRT I think. Using all these methods should make these path additions a lot more robust.

This revision now requires changes to proceed.Oct 15 2019, 3:37 AM
edward-jones retitled this revision from [RISCV] Use compiler-rt if no GCC installation detected to [WIP][RISCV] Use compiler-rt if no GCC installation detected.
edward-jones edited the summary of this revision. (Show Details)

I've rebased, and also refactored this to use AddRunTimeLibs and GetDefaultRuntimeLibType. The tests have been updated to reflect the changes. Notable changes compared to the last version of the patch:

  • AddRunTimeLibs causes -lgcc_s to also be added to the linker command alongside -lgcc. It seems that this is added as part of AddUnwindLibrary in clang/lib/Driver/ToolChains/CommonArgs.cpp. Based on the comments for that function, I would expect to get either -lgcc_s -lgcc or -lgcc -lgcc_eh on the linker command depending on the type of unwinding supported, but this doesn't match the existing behaviour of the RISC-V driver (hence this patch breaks tests), nor does it match the behaviour of RISC-V gcc.
  • The search for compiler-rt no longer looks in <prefix>/<triple>/lib for the runtime libraries. It instead looks in the resource directory <prefix>/lib/clang/<version>/lib. This seems to be more correct; out-of-tree compiler-rt builds just need to make sure to set the install prefix to the result of clang -print-resource-dir.

Please can you add a test for riscv32 and riscv64 without libgcc?

I'm not sure what you mean here; I believe the existing tests already cover riscv{32,64} without libgcc.

This patch is looking much better, thanks for updating it.

Please may you clarify what RISC-V gcc does for -lgcc, -lgcc_s, -lgcc_eh? Is it different to what gcc does on other targets? Being closer to matching the linker arguments that gcc provides to ld seems like a good idea, IMO.

Please can you add a test for riscv32 and riscv64 without libgcc?

I'm not sure what you mean here; I believe the existing tests already cover riscv{32,64} without libgcc.

I'm not sure what I meant there either, sorry. Do disregard that question

This patch is looking much better, thanks for updating it.

Please may you clarify what RISC-V gcc does for -lgcc, -lgcc_s, -lgcc_eh? Is it different to what gcc does on other targets? Being closer to matching the linker arguments that gcc provides to ld seems like a good idea, IMO.

The default behaviour on RISC-V GCC is just -lgcc, however for Clang if I call tools::AddRunTimeLibs then by default I get -lgcc --as-needed -lgcc_s --no-as-needed in the link step.

When libgcc is needed tools::AddRunTimeLibs unconditionally calls AddUnwindLibrary, which in turn calls ToolChain::GetUnwindLibType which always returns ToolChain::UNW_Libgcc for libgcc. The code in AddUnwindLibrary will then always add either -lgcc_eh (for a static libgcc), or -lgcc_s (for a shared libgcc) to the command line. I can override this by reimplementing ToolChain::GetUnwindLibType in the target and making it return ToolChain::UNW_None, but I'm not sure whether that's the desired behavior.

I've changed this to always return ToolChain::UNW_None from RISCVToolChain::GetUnwindLibType now. As a consequence we get the original behaviour of only -lgcc being added to the link command.

LGTM.

I looked at what g++ and gcc do (my riscv64-unknown-elf toolchain):

  • gcc: Passes -lgcc --start-group -lc -lgloss --end-group -lgcc to the linker
  • g++: Passes "-lstdc++" -lm -lgcc --start-group -lc -lgloss --end-group -lgcc to the linker

This patch:

  • clang passes "--start-group" "-lc" "-lgloss" "--end-group" "-lgcc" to the linker
  • clang++ passes "-lstdc++" "--start-group" "-lc" "-lgloss" "--end-group" "-lgcc" to the linker

I think these are all compatible. I'm not sure where the -lm is coming from in the g++ invocation vs the clang++ invocation, but given we have TODOs in this driver code around C++, I don't think that needs to be fixed in this patch.

lenary accepted this revision.Mon, Nov 11, 4:26 AM
This revision is now accepted and ready to land.Mon, Nov 11, 4:26 AM
MaskRay added inline comments.Mon, Nov 11, 1:41 PM
clang/lib/Driver/ToolChains/RISCVToolchain.cpp
48

Ternary operator?

111

Why?

This revision was automatically updated to reflect the committed changes.

Hi. I think this patch is causing some test failures for us:

FAIL: Clang :: Driver/riscv64-toolchain.c (5479 of 16161)
******************** TEST 'Clang :: Driver/riscv64-toolchain.c' FAILED ********************
Script:
--
: 'RUN: at line 3';   /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c -### -no-canonical-prefixes -target riscv64 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/FileCheck -check-prefix=CC1 /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c
: 'RUN: at line 6';   /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c -### -no-canonical-prefixes    -target riscv64-unknown-elf    --gcc-toolchain=/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree    --sysroot=/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree/riscv64-unknown-elf 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/FileCheck -check-prefix=C-RV64-BAREMETAL-LP64 /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c
: 'RUN: at line 22';   /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c -### -no-canonical-prefixes    -target riscv64-unknown-elf    --sysroot=    --gcc-toolchain=/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/FileCheck -check-prefix=C-RV64-BAREMETAL-NOSYSROOT-LP64 /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c
: 'RUN: at line 37';   /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/clang --driver-mode=g++ /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c -### -no-canonical-prefixes    -target riscv64-unknown-elf -stdlib=libstdc++    --gcc-toolchain=/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree    --sysroot=/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree/riscv64-unknown-elf 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/FileCheck -check-prefix=CXX-RV64-BAREMETAL-LP64 /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c
: 'RUN: at line 54';   /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/clang --driver-mode=g++ /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c -### -no-canonical-prefixes    -target riscv64-unknown-elf -stdlib=libstdc++    --sysroot=    --gcc-toolchain=/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/FileCheck -check-prefix=CXX-RV64-BAREMETAL-NOSYSROOT-LP64 /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c
: 'RUN: at line 70';   /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c -### -no-canonical-prefixes -fuse-ld=ld    -target riscv64-unknown-linux-gnu -mabi=lp64    --gcc-toolchain=/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/multilib_riscv_linux_sdk    --sysroot=/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/multilib_riscv_linux_sdk/sysroot 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/FileCheck -check-prefix=C-RV64-LINUX-MULTI-LP64 /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c
: 'RUN: at line 86';   /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c -### -no-canonical-prefixes -fuse-ld=ld    -target riscv64-unknown-linux-gnu -march=rv64imafd    --gcc-toolchain=/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/multilib_riscv_linux_sdk    --sysroot=/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/multilib_riscv_linux_sdk/sysroot 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/FileCheck -check-prefix=C-RV64-LINUX-MULTI-LP64D /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c
: 'RUN: at line 102';   /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/clang -target riscv64 /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c -emit-llvm -S -o - | /b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c
--
Exit Code: 1

Command Output (stderr):
--
/b/s/w/ir/k/llvm-project/clang/test/Driver/riscv64-toolchain.c:16:27: error: C-RV64-BAREMETAL-LP64: expected string not found in input
// C-RV64-BAREMETAL-LP64: "{{.*}}/Inputs/basic_riscv64_tree/lib/gcc/riscv64-unknown-elf/8.0.1{{/|\\\\}}crtbegin.o"
                          ^
<stdin>:6:349: note: scanning from here
 "/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree/lib/gcc/riscv64-unknown-elf/8.0.1/../../../../bin/riscv64-unknown-elf-ld" "--sysroot=/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree/riscv64-unknown-elf" "/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree/riscv64-unknown-elf/lib/crt0.o" "/b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/lib/clang/10.0.0/lib/clang_rt.crtbegin-riscv64.o" "-L/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree/riscv64-unknown-elf/lib" "-L/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree/lib/gcc/riscv64-unknown-elf/8.0.1" "/b/s/w/ir/tmp/t/riscv64-toolchain-ae7410.o" "--start-group" "-lc" "-lgloss" "--end-group" "/b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/lib/clang/10.0.0/lib/libclang_rt.builtins-riscv64.a" "/b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/lib/clang/10.0.0/lib/clang_rt.crtend-riscv64.o" "-o" "a.out"
                                                                                                                                                                                                                                                                                                                                                            ^
<stdin>:6:591: note: possible intended match here
 "/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree/lib/gcc/riscv64-unknown-elf/8.0.1/../../../../bin/riscv64-unknown-elf-ld" "--sysroot=/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree/riscv64-unknown-elf" "/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree/riscv64-unknown-elf/lib/crt0.o" "/b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/lib/clang/10.0.0/lib/clang_rt.crtbegin-riscv64.o" "-L/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree/riscv64-unknown-elf/lib" "-L/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/basic_riscv64_tree/lib/gcc/riscv64-unknown-elf/8.0.1" "/b/s/w/ir/tmp/t/riscv64-toolchain-ae7410.o" "--start-group" "-lc" "-lgloss" "--end-group" "/b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/lib/clang/10.0.0/lib/libclang_rt.builtins-riscv64.a" "/b/s/w/ir/k/recipe_cleanup/clangeX2xRw/llvm_build_dir/lib/clang/10.0.0/lib/clang_rt.crtend-riscv64.o" "-o" "a.out"
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              
... and one more

Could you take a look? Thanks.

Link to error: https://ci.chromium.org/p/fuchsia/builders/ci/clang-linux-x64/b8896884112835419104

rsmith added a subscriber: rsmith.Wed, Nov 13, 1:22 PM

Hi. I think this patch is causing some test failures for us:

This is breaking us too, in the same way. The "problem" is that this makes the riscv64 target use the compiler-rt crtbegin / crtend files instead of the libgcc ones when configured with -DCLANG_DEFAULT_RTLIB=compiler-rt. That's probably the right behavior, but someone should check, and additional tests need to be updated to match. (It's not obvious how to even test this change in the presence of CLANG_DEFAULT_RTLIB...)

The following tests fail in the -DCLANG_DEFAULT_RTLIB=compiler-rt build configuration:

  • test/Driver/riscv32-toolchain.c
  • test/Driver/riscv32-toolchain-extra.c
  • test/Driver/riscv64-toolchain.c
  • test/Driver/riscv64-toolchain-extra.c

Reverted in aeaddf9 to unbreak things for now.

Okay. I'll see if I can find a way to test this when CLANG_DEFAULT_RTLIB is set, and then I'll resubmit

edward-jones reopened this revision.Thu, Nov 14, 4:05 AM
This revision is now accepted and ready to land.Thu, Nov 14, 4:05 AM

It seems that the option --rtlib=platform exists to force the driver to ignore the -DCLANG_DEFAULT_RTLIB for testing purposes, so I've added this option to the tests that were broken.

These tests now seem to pass regardless of whether CLANG_DEFAULT_RTLIB is set to compiler-rt or libgcc.

I'm leaving this WIP as I think it will be prudent to add some tests for --rtlib=compiler and --rtlib=libgcc.

If I set -DCLANG_DEFAULT_RTLIB=compiler-rt I see the following failure in clang/test/Driver/cross-linux.c:

/home/ed/work/proj/riscv-testing/llvm/clang/test/Driver/cross-linux.c:62:26: error: CHECK-MULTI32-X86-64: expected string not found in input
// CHECK-MULTI32-X86-64: "crti.o" "[[gcc_install:.*/Inputs/multilib_32bit_linux_tree/usr/lib/gcc/i386-unknown-linux/4.6.0]]/64{{/|\\\\}}crtbegin.o"
                         ^
<stdin>:7:327: note: scanning from here
 "/home/ed/work/proj/riscv-testing/llvm/clang/test/Driver/Inputs/multilib_32bit_linux_tree/usr/lib/gcc/i386-unknown-linux/4.6.0/../../../../i386-unknown-linux/bin/ld" "--sysroot=/home/ed/work/proj/riscv-testing/llvm/clang/test/Driver/Inputs/basic_linux_tree" "-z" "relro" "--hash-style=gnu" "--eh-frame-hdr" "-m" "elf_x86_64" "-dynamic-linker" "/lib64/ld-linux-x86-64.so.2" "-o" "/home/ed/work/proj/riscv-testing/build/llvm/tools/clang/test/Driver/Output/cross-linux.c.tmp" "crt1.o" "crti.o" "/home/ed/work/proj/riscv-testing/build/llvm/lib/clang/10.0.0/lib/linux/clang_rt.crtbegin-x86_64.o" "-L/home/ed/work/proj/riscv-testing/llvm/clang/test/Driver/Inputs/multilib_32bit_linux_tree/usr/lib/gcc/i386-unknown-linux/4.6.0/64" "-L/home/ed/work/proj/riscv-testing/llvm/clang/test/Driver/Inputs/multilib_32bit_linux_tree/usr/lib/gcc/i386-unknown-linux/4.6.0/../../../../i386-unknown-linux/lib/../lib64" "-L/home/ed/work/proj/riscv-testing/llvm/clang/test/Driver/Inputs/multilib_32bit_linux_tree/usr/lib/gcc/i386-unknown-linux/4.6.0" "-L/home/ed/work/proj/riscv-testing/llvm/clang/test/Driver/Inputs/multilib_32bit_linux_tree/usr/lib/gcc/i386-unknown-linux/4.6.0/../../../../i386-unknown-linux/lib" "-L/home/ed/work/proj/riscv-testing/llvm/clang/test/Driver/Inputs/basic_linux_tree/lib" "-L/home/ed/work/proj/riscv-testing/llvm/clang/test/Driver/Inputs/basic_linux_tree/usr/lib" "/tmp/cross-linux-bb2220.o" "/home/ed/work/proj/riscv-testing/build/llvm/lib/clang/10.0.0/lib/linux/libclang_rt.builtins-x86_64.a" "--as-needed" "-lgcc_s" "--no-as-needed" "-lc" "/home/ed/work/proj/riscv-testing/build/llvm/lib/clang/10.0.0/lib/linux/libclang_rt.builtins-x86_64.a" "--as-needed" "-lgcc_s" "--no-as-needed" "/home/ed/work/proj/riscv-testing/build/llvm/lib/clang/10.0.0/lib/linux/clang_rt.crtend-x86_64.o" "crtn.o"
                                                                                                                                                                                                                                                                                                                                      ^

And if I set -DCLANG_DEFAULT_RTLIB=libgcc then I see the following failure in clang/test/Driver/wasm-toolchain.cpp:

/home/ed/work/proj/riscv-testing/llvm/clang/test/Driver/wasm-toolchain.cpp:20:10: error: LINK: expected string not found in input
// LINK: wasm-ld{{.*}}" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc++" "-lc++abi" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"

<stdin>:6:1018: note: scanning from here
 "/home/ed/work/proj/riscv-testing/build/llvm/bin/clang" "-cc1" "-triple" "wasm32-unknown-unknown" "-emit-obj" "-mrelax-all" "-disable-free" "-main-file-name" "wasm-toolchain.cpp" "-mrelocation-model" "static" "-mthread-model" "posix" "-mframe-pointer=none" "-fno-rounding-math" "-masm-verbose" "-mconstructor-aliases" "-fuse-init-array" "-target-cpu" "generic" "-fvisibility" "hidden" "-dwarf-column-info" "-debugger-tuning=gdb" "-resource-dir" "/home/ed/work/proj/riscv-testing/build/llvm/lib/clang/10.0.0" "-isysroot" "/foo" "-internal-isystem" "/foo/include/c++/v1" "-internal-isystem" "/home/ed/work/proj/riscv-testing/build/llvm/lib/clang/10.0.0/include" "-internal-isystem" "/foo/include" "-fdeprecated-macro" "-fdebug-compilation-dir" "/home/ed/work/proj/riscv-testing/build/llvm/tools/clang/test/Driver" "-ferror-limit" "19" "-fmessage-length" "0" "-fgnuc-version=4.2.1" "-fobjc-runtime=gnustep" "-fcxx-exceptions" "-fexceptions" "-fno-common" "-fdiagnostics-show-option" "-o" "/tmp/wasm-toolchain-718d0c.o" "-x" "c++" "/home/ed/work/proj/riscv-testing/llvm/clang/test/Driver/wasm-toolchain.cpp"
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                         ^

I don't see how these tests could be affected by this patch, so I'm wondering whether these tests have always been broken?

edward-jones requested review of this revision.Thu, Nov 14, 4:34 AM
edward-jones retitled this revision from [WIP][RISCV] Use compiler-rt if no GCC installation detected to [RISCV] Use compiler-rt if no GCC installation detected.

Added tests that a user can specify a specific runtime library through --rtlib. Also rebased on master, and added a comment explaining the use of --rtlib=platform in the tests.