[PATCH] D68391: [RISCV] Improve sysroot computation if no GCC install detected

Tue Oct 15 09:06:04 PDT 2019

lenary added a comment.

In D68391#1694622 <https://reviews.llvm.org/D68391#1694622>, @edward-jones wrote:

> Rebased and added tests
>
> I've made this use the Triple from the driver rather than the parsed LLVM triple, this means the Triple doesn't get normalized which seems like more desirable behavior.

Yeah, using the user-provided triple seems the correct choice, as the user will expect it to match the directory name. Would be useful to add a comment saying something along the same lines where you do that.

> I've added to the riscv{32,64}-toolchain.c test files, however the added tests cannot be run without a shell so I've had to disable those tests on Windows. If necessary I can split these new tests out into separate files.

I think splitting the tests might be sensible, so that we can run as many as possible on platforms without a shell/windows.

> I realize that there don't appear to be any tests for the case where no GCC install is found and no sysroot is provided to the driver. At the moment this will result in a generic linker command using the system linker, such as:
> 
> /usr/bin/ld crt0.o crtbegin.o ... -lgcc crtend.o
> 
> Is this the desired behaviour? And if so should a test be added for this too?

I think defaulting to root if no sysroot/gcc install is found is better than erroring. In all likelihood a compile/link task will fail anyway because it cannot link the results together. In the case where the compile/link does work, then there's no issue. It would be useful to have a test for this.

In D68391#1707652 <https://reviews.llvm.org/D68391#1707652>, @luismarques wrote:

> This is indeed an issue that would be nice to fix, I've often been annoyed by clang just defaulting to the root when some misconfiguration occurs. I have to wonder though, this patch only changes the clang RISC-V toolchain driver, but the problem isn't specific to RISC-V. Couldn't this tweak be generalized and made to apply to multiple/all target drivers?

I think that for baremetal risc-v, we can change this default without asking the wider community. For other targets, someone should email cfe-dev first with the proposal. I agree it makes sense, but I also imagine it could break a lot of builds unexpectedly.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68391/new/

https://reviews.llvm.org/D68391