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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
