[PATCH] D93138: Add initial support for multilibs in Baremetal toolchain.

[Hafiz Abid Qadeer via Phabricator via cfe-commits]

abidh added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:177
   if (!getDriver().SysRoot.empty())
-    return getDriver().SysRoot;
+    return getDriver().SysRoot + SelectedMultilib.osSuffix();
 
----------------
zixuan-wu wrote:
> I think the multilib osSuffix added here will make every multilib contain include dir, and those include dirs are the same. For lib, there should be different multilib dir. 
> 
> So it's better to add osSuffix at construction function like following I think instead of be part of sysroot.
> 
> ```
> if (!SysRoot.empty()) {
>     llvm::sys::path::append(SysRoot, "lib", SelectedMultilib.osSuffix());
>     getFilePaths().push_back(std::string(SysRoot));
>   }
> ```
> 
> @abidh 
The multilib include directories are not necessarily the same.  In my opinion, having a separate include and lib directory for every multilib is much cleaner then having all multilib share an include directory and then you put multilib specific directories inside that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93138