Page MenuHomePhabricator

Provide default location of sysroot for Baremetal toolchain.
ClosedPublic

Authored by abidh on Dec 4 2020, 11:44 AM.

Details

Summary

Currently, Baremetal toolchain requires user to pass a sysroot location
using a --sysroot flag. This is not very convenient for the user. It also
creates problem for toolchain vendors who don't have a fixed location to
put the sysroot bits.

Clang does provide 'DEFAULT_SYSROOT' which can be used by the toolchain
builder to provide the default location. But it does not work if toolchain
is targeting multiple targets e.g. arm-none-eabi/riscv64-unknown-elf which
clang is capable of doing.

This patch tries to solve this problem by providing a default location of
the toolchain if user does not explicitly provides --sysroot. The exact
location and name can be different but it should fulfill these conditions:

  1. The sysroot path should have a target triple element so that multi-target

toolchain problem (as I described above) could be addressed.

  1. The location should not be $TOP/$Triple as this is used by gcc generally

and will be a problem for installing both gcc and clang based toolchain at
the same location.

Diff Detail

Event Timeline

abidh created this revision.Dec 4 2020, 11:44 AM
abidh requested review of this revision.Dec 4 2020, 11:44 AM
jroelofs accepted this revision.Dec 4 2020, 12:35 PM

LGTM

clang/lib/Driver/ToolChains/BareMetal.cpp
105

Why not always add it, even if it doesn't exist? That's an easier failure mode to diagnose as a user, since you'll see the compiler appending paths that aren't there, rather than having magic behavior.

108

Small string has an implicit conversion to std::string, so you can just return SysRootDir; here.

This revision is now accepted and ready to land.Dec 4 2020, 12:35 PM
abidh updated this revision to Diff 309637.Dec 4 2020, 1:29 PM

Handled review comments.

abidh marked an inline comment as done.Dec 4 2020, 1:31 PM
abidh added inline comments.
clang/lib/Driver/ToolChains/BareMetal.cpp
108

I was getting a compile error on that. Looking in SmallString.h, it seems that conversion to std::string is explicit.

jroelofs added inline comments.Dec 4 2020, 1:34 PM
clang/lib/Driver/ToolChains/BareMetal.cpp
105

did return SysRootDir; not work? Why do you need the explicit std::string ctor call?

jroelofs added inline comments.Dec 4 2020, 1:37 PM
clang/lib/Driver/ToolChains/BareMetal.cpp
108

ohh, I see what's going on. doxygen doesn't list the explicit keyword in the html page, so I thought it was implicit. sorry.

This revision was automatically updated to reflect the committed changes.
phosek added a subscriber: phosek.May 6 2021, 10:29 PM
phosek added inline comments.
clang/lib/Driver/ToolChains/BareMetal.cpp
102

I apologize for raising a comment on an older change but I ran into this recently, GCC uses ../sys-root as the default sysroot location and I think it'd be useful for Clang to be consistent.

abidh added a comment.May 7 2021, 2:40 AM

If gcc and clang based toolchains are installed in the same prefix then having sysroot at same location can cause one installation to overwrite parts of other.