[PATCH] D92677: Provide default location of sysroot for Baremetal toolchain.
abidh added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92677/new/ https://reviews.llvm.org/D92677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92677: Provide default location of sysroot for Baremetal toolchain.
phosek added inline comments. Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:102 + SmallString<128> SysRootDir; + llvm::sys::path::append(SysRootDir, getDriver().Dir, "../lib/clang-runtimes", + getDriver().getTargetTriple()); 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92677/new/ https://reviews.llvm.org/D92677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92677: Provide default location of sysroot for Baremetal toolchain.
This revision was automatically updated to reflect the committed changes. Closed by commit rG275592e71413: Provide default location of sysroot for Baremetal toolchain. (authored by abidh). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92677/new/ https://reviews.llvm.org/D92677 Files: clang/lib/Driver/ToolChains/BareMetal.cpp clang/lib/Driver/ToolChains/BareMetal.h clang/test/Driver/baremetal-sysroot.cpp Index: clang/test/Driver/baremetal-sysroot.cpp === --- /dev/null +++ clang/test/Driver/baremetal-sysroot.cpp @@ -0,0 +1,22 @@ +// REQUIRES: shell +// UNSUPPORTED: system-windows + +// Test that when a --sysroot is not provided, driver picks the default +// location correctly if available. + +// RUN: rm -rf %T/baremetal_default_sysroot +// RUN: mkdir -p %T/baremetal_default_sysroot/bin +// RUN: mkdir -p %T/baremetal_default_sysroot/lib/clang-runtimes/armv6m-none-eabi +// RUN: ln -s %clang %T/baremetal_default_sysroot/bin/clang + +// RUN: %T/baremetal_default_sysroot/bin/clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \ +// RUN: -target armv6m-none-eabi \ +// RUN: | FileCheck --check-prefix=CHECK-V6M-C %s +// CHECK-V6M-C: "{{.*}}clang{{.*}}" "-cc1" "-triple" "thumbv6m-none-unknown-eabi" +// CHECK-V6M-C-SAME: "-internal-isystem" "{{.*}}/baremetal_default_sysroot{{[/\\]+}}bin{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+}}armv6m-none-eabi{{[/\\]+}}include{{[/\\]+}}c++{{[/\\]+}}v1" +// CHECk-V6M-C-SAME: "-internal-isystem" "{{.*}}/baremetal_default_sysroot{{[/\\]+}}bin{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+}}armv6m-none-eabi{{[/\\]+}}include" +// CHECK-V6M-C-SAME: "-x" "c++" "{{.*}}baremetal-sysroot.cpp" +// CHECK-V6M-C-NEXT: "{{[^"]*}}ld{{(\.(lld|bfd|gold))?}}{{(\.exe)?}}" "{{.*}}.o" "-Bstatic" +// CHECK-V6M-C-SAME: "-L{{.*}}/baremetal_default_sysroot{{[/\\]+}}bin{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+}}armv6m-none-eabi{{[/\\]+}}lib" +// CHECK-V6M-C-SAME: "-lc" "-lm" "-lclang_rt.builtins-armv6m" +// CHECK-V6M-C-SAME: "-o" "{{.*}}.o" Index: clang/lib/Driver/ToolChains/BareMetal.h === --- clang/lib/Driver/ToolChains/BareMetal.h +++ clang/lib/Driver/ToolChains/BareMetal.h @@ -67,6 +67,7 @@ llvm::opt::ArgStringList ) const override; void AddLinkRuntimeLib(const llvm::opt::ArgList , llvm::opt::ArgStringList ) const; + std::string computeSysRoot() const override; }; } // namespace toolchains Index: clang/lib/Driver/ToolChains/BareMetal.cpp === --- clang/lib/Driver/ToolChains/BareMetal.cpp +++ clang/lib/Driver/ToolChains/BareMetal.cpp @@ -33,7 +33,7 @@ getProgramPaths().push_back(getDriver().getInstalledDir()); if (getDriver().getInstalledDir() != getDriver().Dir) getProgramPaths().push_back(getDriver().Dir); - SmallString<128> SysRoot(getDriver().SysRoot); + SmallString<128> SysRoot(computeSysRoot()); if (!SysRoot.empty()) { llvm::sys::path::append(SysRoot, "lib"); getFilePaths().push_back(std::string(SysRoot)); @@ -94,6 +94,17 @@ return std::string(Dir.str()); } +std::string BareMetal::computeSysRoot() const { + if (!getDriver().SysRoot.empty()) +return getDriver().SysRoot; + + SmallString<128> SysRootDir; + llvm::sys::path::append(SysRootDir, getDriver().Dir, "../lib/clang-runtimes", + getDriver().getTargetTriple()); + + return std::string(SysRootDir); +} + void BareMetal::AddClangSystemIncludeArgs(const ArgList , ArgStringList ) const { if (DriverArgs.hasArg(options::OPT_nostdinc)) @@ -106,7 +117,7 @@ } if (!DriverArgs.hasArg(options::OPT_nostdlibinc)) { -SmallString<128> Dir(getDriver().SysRoot); +SmallString<128> Dir(computeSysRoot()); if (!Dir.empty()) { llvm::sys::path::append(Dir, "include"); addSystemInclude(DriverArgs, CC1Args, Dir.str()); @@ -127,7 +138,7 @@ DriverArgs.hasArg(options::OPT_nostdincxx)) return; - StringRef SysRoot = getDriver().SysRoot; + std::string SysRoot(computeSysRoot()); if (SysRoot.empty()) return; Index: clang/test/Driver/baremetal-sysroot.cpp === --- /dev/null +++ clang/test/Driver/baremetal-sysroot.cpp @@ -0,0 +1,22 @@ +// REQUIRES: shell +// UNSUPPORTED: system-windows + +// Test that when a --sysroot is not provided, driver picks the default +// location correctly if available. + +// RUN: rm -rf %T/baremetal_default_sysroot +// RUN: mkdir -p %T/baremetal_default_sysroot/bin +// RUN: mkdir -p %T/baremetal_default_sysroot/lib/clang-runtimes/armv6m-none-eabi +// RUN: ln -s %clang %T/baremetal_default_sysroot/bin/clang + +// RUN: %T/baremetal_default_sysroot/bin/clang
[PATCH] D92677: Provide default location of sysroot for Baremetal toolchain.
jroelofs added inline comments. Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:108 + + return std::string(SysRootDir.str()); +} abidh wrote: > jroelofs wrote: > > Small string has an implicit conversion to std::string, so you can just > > `return SysRootDir;` here. > I was getting a compile error on that. Looking in SmallString.h, it seems > that conversion to std::string is explicit. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92677/new/ https://reviews.llvm.org/D92677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92677: Provide default location of sysroot for Baremetal toolchain.
jroelofs added inline comments. Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:105 + + return std::string(SysRootDir); +} did `return SysRootDir;` not work? Why do you need the explicit std::string ctor call? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92677/new/ https://reviews.llvm.org/D92677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92677: Provide default location of sysroot for Baremetal toolchain.
abidh marked an inline comment as done. abidh added inline comments. Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:108 + + return std::string(SysRootDir.str()); +} jroelofs wrote: > Small string has an implicit conversion to std::string, so you can just > `return SysRootDir;` here. I was getting a compile error on that. Looking in SmallString.h, it seems that conversion to std::string is explicit. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92677/new/ https://reviews.llvm.org/D92677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92677: Provide default location of sysroot for Baremetal toolchain.
abidh updated this revision to Diff 309637. abidh added a comment. Handled review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92677/new/ https://reviews.llvm.org/D92677 Files: clang/lib/Driver/ToolChains/BareMetal.cpp clang/lib/Driver/ToolChains/BareMetal.h clang/test/Driver/baremetal-sysroot.cpp Index: clang/test/Driver/baremetal-sysroot.cpp === --- /dev/null +++ clang/test/Driver/baremetal-sysroot.cpp @@ -0,0 +1,22 @@ +// REQUIRES: shell +// UNSUPPORTED: system-windows + +// Test that when a --sysroot is not provided, driver picks the default +// location correctly if available. + +// RUN: rm -rf %T/baremetal_default_sysroot +// RUN: mkdir -p %T/baremetal_default_sysroot/bin +// RUN: mkdir -p %T/baremetal_default_sysroot/lib/clang-runtimes/armv6m-none-eabi +// RUN: ln -s %clang %T/baremetal_default_sysroot/bin/clang + +// RUN: %T/baremetal_default_sysroot/bin/clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \ +// RUN: -target armv6m-none-eabi \ +// RUN: | FileCheck --check-prefix=CHECK-V6M-C %s +// CHECK-V6M-C: "{{.*}}clang{{.*}}" "-cc1" "-triple" "thumbv6m-none-unknown-eabi" +// CHECK-V6M-C-SAME: "-internal-isystem" "{{.*}}/baremetal_default_sysroot{{[/\\]+}}bin{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+}}armv6m-none-eabi{{[/\\]+}}include{{[/\\]+}}c++{{[/\\]+}}v1" +// CHECk-V6M-C-SAME: "-internal-isystem" "{{.*}}/baremetal_default_sysroot{{[/\\]+}}bin{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+}}armv6m-none-eabi{{[/\\]+}}include" +// CHECK-V6M-C-SAME: "-x" "c++" "{{.*}}baremetal-sysroot.cpp" +// CHECK-V6M-C-NEXT: "{{[^"]*}}ld{{(\.(lld|bfd|gold))?}}{{(\.exe)?}}" "{{.*}}.o" "-Bstatic" +// CHECK-V6M-C-SAME: "-L{{.*}}/baremetal_default_sysroot{{[/\\]+}}bin{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+}}armv6m-none-eabi{{[/\\]+}}lib" +// CHECK-V6M-C-SAME: "-lc" "-lm" "-lclang_rt.builtins-armv6m" +// CHECK-V6M-C-SAME: "-o" "{{.*}}.o" Index: clang/lib/Driver/ToolChains/BareMetal.h === --- clang/lib/Driver/ToolChains/BareMetal.h +++ clang/lib/Driver/ToolChains/BareMetal.h @@ -67,6 +67,7 @@ llvm::opt::ArgStringList ) const override; void AddLinkRuntimeLib(const llvm::opt::ArgList , llvm::opt::ArgStringList ) const; + std::string computeSysRoot() const override; }; } // namespace toolchains Index: clang/lib/Driver/ToolChains/BareMetal.cpp === --- clang/lib/Driver/ToolChains/BareMetal.cpp +++ clang/lib/Driver/ToolChains/BareMetal.cpp @@ -33,7 +33,7 @@ getProgramPaths().push_back(getDriver().getInstalledDir()); if (getDriver().getInstalledDir() != getDriver().Dir) getProgramPaths().push_back(getDriver().Dir); - SmallString<128> SysRoot(getDriver().SysRoot); + SmallString<128> SysRoot(computeSysRoot()); if (!SysRoot.empty()) { llvm::sys::path::append(SysRoot, "lib"); getFilePaths().push_back(std::string(SysRoot)); @@ -94,6 +94,17 @@ return std::string(Dir.str()); } +std::string BareMetal::computeSysRoot() const { + if (!getDriver().SysRoot.empty()) +return getDriver().SysRoot; + + SmallString<128> SysRootDir; + llvm::sys::path::append(SysRootDir, getDriver().Dir, "../lib/clang-runtimes", + getDriver().getTargetTriple()); + + return std::string(SysRootDir); +} + void BareMetal::AddClangSystemIncludeArgs(const ArgList , ArgStringList ) const { if (DriverArgs.hasArg(options::OPT_nostdinc)) @@ -106,7 +117,7 @@ } if (!DriverArgs.hasArg(options::OPT_nostdlibinc)) { -SmallString<128> Dir(getDriver().SysRoot); +SmallString<128> Dir(computeSysRoot()); if (!Dir.empty()) { llvm::sys::path::append(Dir, "include"); addSystemInclude(DriverArgs, CC1Args, Dir.str()); @@ -127,7 +138,7 @@ DriverArgs.hasArg(options::OPT_nostdincxx)) return; - StringRef SysRoot = getDriver().SysRoot; + std::string SysRoot(computeSysRoot()); if (SysRoot.empty()) return; Index: clang/test/Driver/baremetal-sysroot.cpp === --- /dev/null +++ clang/test/Driver/baremetal-sysroot.cpp @@ -0,0 +1,22 @@ +// REQUIRES: shell +// UNSUPPORTED: system-windows + +// Test that when a --sysroot is not provided, driver picks the default +// location correctly if available. + +// RUN: rm -rf %T/baremetal_default_sysroot +// RUN: mkdir -p %T/baremetal_default_sysroot/bin +// RUN: mkdir -p %T/baremetal_default_sysroot/lib/clang-runtimes/armv6m-none-eabi +// RUN: ln -s %clang %T/baremetal_default_sysroot/bin/clang + +// RUN: %T/baremetal_default_sysroot/bin/clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \ +// RUN: -target armv6m-none-eabi \ +// RUN: | FileCheck --check-prefix=CHECK-V6M-C %s +//
[PATCH] D92677: Provide default location of sysroot for Baremetal toolchain.
jroelofs accepted this revision. jroelofs added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:105 + + if (!llvm::sys::fs::exists(SysRootDir)) +return std::string(); 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. Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:108 + + return std::string(SysRootDir.str()); +} Small string has an implicit conversion to std::string, so you can just `return SysRootDir;` here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92677/new/ https://reviews.llvm.org/D92677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92677: Provide default location of sysroot for Baremetal toolchain.
abidh created this revision. abidh added reviewers: mcarrasco, jroelofs. abidh added a project: clang. Herald added subscribers: s.egerton, PkmX, simoncook, kristof.beyls. abidh requested review of this revision. Herald added a subscriber: cfe-commits. 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. 2. 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. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D92677 Files: clang/lib/Driver/ToolChains/BareMetal.cpp clang/lib/Driver/ToolChains/BareMetal.h clang/test/Driver/baremetal-sysroot.cpp Index: clang/test/Driver/baremetal-sysroot.cpp === --- /dev/null +++ clang/test/Driver/baremetal-sysroot.cpp @@ -0,0 +1,22 @@ +// REQUIRES: shell +// UNSUPPORTED: system-windows + +// Test that when a --sysroot is not provided, driver picks the default +// location correctly if available. + +// RUN: rm -rf %T/baremetal_default_sysroot +// RUN: mkdir -p %T/baremetal_default_sysroot/bin +// RUN: mkdir -p %T/baremetal_default_sysroot/lib/clang-runtimes/armv6m-none-eabi +// RUN: ln -s %clang %T/baremetal_default_sysroot/bin/clang + +// RUN: %T/baremetal_default_sysroot/bin/clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \ +// RUN: -target armv6m-none-eabi \ +// RUN: | FileCheck --check-prefix=CHECK-V6M-C %s +// CHECK-V6M-C: "{{.*}}clang{{.*}}" "-cc1" "-triple" "thumbv6m-none-unknown-eabi" +// CHECK-V6M-C-SAME: "-internal-isystem" "{{.*}}/baremetal_default_sysroot{{[/\\]+}}bin{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+}}armv6m-none-eabi{{[/\\]+}}include{{[/\\]+}}c++{{[/\\]+}}v1" +// CHECk-V6M-C-SAME: "-internal-isystem" "{{.*}}/baremetal_default_sysroot{{[/\\]+}}bin{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+}}armv6m-none-eabi{{[/\\]+}}include" +// CHECK-V6M-C-SAME: "-x" "c++" "{{.*}}baremetal-sysroot.cpp" +// CHECK-V6M-C-NEXT: "{{[^"]*}}ld{{(\.(lld|bfd|gold))?}}{{(\.exe)?}}" "{{.*}}.o" "-Bstatic" +// CHECK-V6M-C-SAME: "-L{{.*}}/baremetal_default_sysroot{{[/\\]+}}bin{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+}}armv6m-none-eabi{{[/\\]+}}lib" +// CHECK-V6M-C-SAME: "-lc" "-lm" "-lclang_rt.builtins-armv6m" +// CHECK-V6M-C-SAME: "-o" "{{.*}}.o" Index: clang/lib/Driver/ToolChains/BareMetal.h === --- clang/lib/Driver/ToolChains/BareMetal.h +++ clang/lib/Driver/ToolChains/BareMetal.h @@ -67,6 +67,7 @@ llvm::opt::ArgStringList ) const override; void AddLinkRuntimeLib(const llvm::opt::ArgList , llvm::opt::ArgStringList ) const; + std::string computeSysRoot() const override; }; } // namespace toolchains Index: clang/lib/Driver/ToolChains/BareMetal.cpp === --- clang/lib/Driver/ToolChains/BareMetal.cpp +++ clang/lib/Driver/ToolChains/BareMetal.cpp @@ -33,7 +33,7 @@ getProgramPaths().push_back(getDriver().getInstalledDir()); if (getDriver().getInstalledDir() != getDriver().Dir) getProgramPaths().push_back(getDriver().Dir); - SmallString<128> SysRoot(getDriver().SysRoot); + SmallString<128> SysRoot(computeSysRoot()); if (!SysRoot.empty()) { llvm::sys::path::append(SysRoot, "lib"); getFilePaths().push_back(std::string(SysRoot)); @@ -94,6 +94,20 @@ return std::string(Dir.str()); } +std::string BareMetal::computeSysRoot() const { + if (!getDriver().SysRoot.empty()) +return getDriver().SysRoot; + + SmallString<128> SysRootDir; + llvm::sys::path::append(SysRootDir, getDriver().Dir, "../lib/clang-runtimes", + getDriver().getTargetTriple()); + + if (!llvm::sys::fs::exists(SysRootDir)) +return std::string(); + + return std::string(SysRootDir.str()); +} + void BareMetal::AddClangSystemIncludeArgs(const ArgList , ArgStringList ) const { if (DriverArgs.hasArg(options::OPT_nostdinc)) @@ -106,7 +120,7 @@ } if