Page MenuHomePhabricator

[RISCV] Pick the correct RISCV linker instead of calling riscv-gcc to link
AbandonedPublic

Authored by mgrang on Jan 29 2018, 5:54 PM.

Details

Reviewers
asb
apazos
Summary

Currently we are invoking riscv-gcc to link. This causes all flags passed to clang to be passed down to gcc as well.
As a result we run into errors for flags not recognzied by gcc.

Diff Detail

Repository
rC Clang

Event Timeline

mgrang created this revision.Jan 29 2018, 5:54 PM
asb added a comment.EditedJan 30 2018, 10:26 AM

My main concern with this patch is that the description doesn't really match what it does. The current in-tree code _doesn't_ call gcc to link for the tested configuration (a multilib toolchain), and this is verified with the tests in test/Driver/riscv32-toolchain.c. D39963 originally added support for a baremetal toolchain, but was changed to focus on the multilib Linux toolchain for a couple of reasons:

  1. This was a more straight-forward change
  2. Downstream users such as yourself had a higher priority for building for a Linux target
  3. It looks like we'd want to discuss the possibility of adding RISC-V support to lib/Driver/ToolChains/BareMetal rather than adding yet another target-specific toolchain

This patch seems identical to my changes to add a baremetal toolchain (https://github.com/lowRISC/riscv-llvm/blob/master/clang/0003-RISCV-Implement-clang-driver-for-the-baremetal-RISCV.patch which was previously submitted as part of D39963), with a line changed (setting a different name for the linker). Was it meant to be a one-line patch on top of that work?

Are you compiling for a Linux target or for bare metal? One problem with the multilib detection code is that if it fails it tends to do so silently. I see two paths forward depending on the immediate problem you're seeing:

  1. If you're seeing an issue with the correct linker being selected when compiling using a Linux toolchain, the correct fix would be to modify that detection logic committed in rL322276 and add new tests
  2. If the issue is that you're now trying to use Clang for a bare-metal target, we should discuss whether to move forwards with my baremetal patch as-is or to try to modify lib/Driver/ToolChains/BareMetal - probably worth discussing this on cfe-dev.

The fact you've hardcoded "riscv32-unknown-linux-gnu-ld" as the linker name makes me wonder if the current issue you're seeing is 1)?

In D42673#992056, @asb wrote:

My main concern with this patch is that the description doesn't really match what it does. The current in-tree code _doesn't_ call gcc to link for the tested configuration (a multilib toolchain), and this is verified with the tests in test/Driver/riscv32-toolchain.c. D39963 originally added support for a baremetal toolchain, but was changed to focus on the multilib Linux toolchain for a couple of reasons:

  1. This was a more straight-forward change
  2. Downstream users such as yourself had a higher priority for building for a Linux target
  3. It looks like we'd want to discuss the possibility of adding RISC-V support to lib/Driver/ToolChains/BareMetal rather than adding yet another target-specific toolchain

This patch seems identical to my changes to add a baremetal toolchain (https://github.com/lowRISC/riscv-llvm/blob/master/clang/0003-RISCV-Implement-clang-driver-for-the-baremetal-RISCV.patch which was previously submitted as part of D39963), with a line changed (setting a different name for the linker). Was it meant to be a one-line patch on top of that work?

Are you compiling for a Linux target or for bare metal? One problem with the multilib detection code is that if it fails it tends to do so silently. I see two paths forward depending on the immediate problem you're seeing:

  1. If you're seeing an issue with the correct linker being selected when compiling using a Linux toolchain, the correct fix would be to modify that detection logic committed in rL322276 and add new tests
  2. If the issue is that you're now trying to use Clang for a bare-metal target, we should discuss whether to move forwards with my baremetal patch as-is or to try to modify lib/Driver/ToolChains/BareMetal - probably worth discussing this on cfe-dev.

The fact you've hardcoded "riscv32-unknown-linux-gnu-ld" as the linker name makes me wonder if the current issue you're seeing is 1)?

Alex,
This patch is based on your previously submitted patch as part of D39963. The issue is that if I pass -target riscv32 then it invokes riscv32-gcc but if I pass -target riscv32-unknown-linux-gnu then is correctly invokes riscv32-unknown-linux-gnu-ld. So what we need to sort out is the behavior we want for the -target option. We should let it fail to find the linker if the complete triple is not passed rather than invoking the gcc driver (that's the behavior on ARM).

mgrang abandoned this revision.Feb 1 2018, 12:15 PM

For now, by setting the full triple (riscv-unknown-linux-gnu) we are able to invoke the correct gnu linker. So will abandon this patch.