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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to