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

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:183
-  SmallString<128> SysRootDir;
-  llvm::sys::path::append(SysRootDir, getDriver().Dir, "../lib/clang-runtimes",
-                          getDriver().getTargetTriple());
----------------
phosek wrote:
> 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?
If I remember correctly @abidh did it this way specifically to avoid conflicting with the contents of GCC's `sys-root` directory.


================
Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:184
+  SmallString<128> MultilibPath(SysRootDir);
+  llvm::sys::path::append(MultilibPath, MULTILIB_YAML_FILENAME);
+
----------------
phosek wrote:
> 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.
Sounds good, let's do it in a later patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142986