[PATCH] D142986: Enable multilib.yaml in the BareMetal ToolChain

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:180
+  SmallString<128> SysRootDir(D.Dir);
+  llvm::sys::path::append(SysRootDir, "../lib/clang-runtimes");
+
----------------
Defer to `llvm::sys::path::append` for joining the path components to ensure that the right separator is used.


================
Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:183
-  SmallString<128> SysRootDir;
-  llvm::sys::path::append(SysRootDir, getDriver().Dir, "../lib/clang-runtimes",
-                          getDriver().getTargetTriple());
----------------
I see that this is a pre-existing behavior, but I find it a unfortunate that the BareMetal driver is using its own convention. I'd prefer using `../sys-root` which the existing convention used by GCC. I'm wondering if this is something we can change as part of the transition to the new multilib implementation?


================
Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:184
+  SmallString<128> MultilibPath(SysRootDir);
+  llvm::sys::path::append(MultilibPath, MULTILIB_YAML_FILENAME);
+
----------------
michaelplatings wrote:
> phosek wrote:
> > Rather than hardcoding the filename and the location, which is inflexible, could we instead provide a command line option to specify the file to use?
> I'm not opposed to that in principle but I'd rather leave that as an option for a future change. At this point, for LLVM Embedded Toolchain for Arm our intent is that users will specify `--sysroot` if they want to use a separate set of multilibs.
Relying on `--sysroot` is fine if you're assembling the sysroot yourself and control the content, but when dealing with an existing sysroot it may not be possible to add additional files in which case having the ability to specify the `.yaml` file may be essential. Having an option also gives more flexibility, for example I could imagine having more than one `.yaml` file for the same sysroot with different subsets of multilibs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142986