Page MenuHomePhabricator

[RISCV] Improve sysroot computation if no GCC install detected
ClosedPublic

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

Details

Summary

If a GCC installed is not detected, the driver would default to the root of the filesystem. This is not ideal when this doesn't match the install directory of the toolchain and can cause undesireable behavior such as picking up system libraries or the system linker when cross-compiling.

Diff Detail

Event Timeline

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

Before falling back to returning the empty string this first looks for a directory with the triple name up one level from the driver.

This uses the Triple string in the path, however this Triple has been normalized so even if the user specifies "clang --target=riscv32-unknown-elf", it will use "riscv32-unknown-unknown-elf" when looking for the sysroot. I am wondering whether it would be sensible to do a more "intelligent" search for the sysroot here by iteratively stripping "unknown"s from the triple.

edward-jones planned changes to this revision.Oct 4 2019, 5:52 AM

I'm reworking this at the moment and adding some tests. Standby!

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.

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 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?

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?

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.

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.

lenary accepted this revision.Fri, Oct 25, 6:46 AM

LGTM.

This revision is now accepted and ready to land.Fri, Oct 25, 6:46 AM

Rebased, added a comment to explain that this is using the user provided triple instead of the canonical one, and split out the tests requiring a shell into separate files (clang/test/Driver/riscv{32,64}-toolchain-extra.c)

edward-jones requested review of this revision.Wed, Nov 6, 6:37 AM
lenary accepted this revision.Thu, Nov 7, 5:41 AM

Thanks for the rebase. It looks good, let's get it committed!

This revision is now accepted and ready to land.Thu, Nov 7, 5:41 AM
This revision was automatically updated to reflect the committed changes.

The test seems to fail on Windows, could you take a look and revert if it takes a while to fix?