[PATCH] D82252: MachO: support `-syslibroot`

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: lld/MachO/Driver.cpp:136
+      for (auto root : roots) {
+        SmallString<261> buffer(root);
+        llvm::sys::path::append(buffer, path);
----------------
int3 wrote:
> compnerd wrote:
> > int3 wrote:
> > > any particular reason why you picked 261?
> > MAX(256, 261) - 261 is the Windows path limit, 256 is the value used on Unix.
> Ah I see. Can we make it a named constant?
Id rather do that in a follow up.  There are other places where we have file path sized buffers.


================
Comment at: lld/MachO/Driver.cpp:160
+    // NOTE: only absolute paths are re-rooted to syslibroot(s)
+    if (llvm::sys::path::is_absolute(path, llvm::sys::path::Style::posix)) {
+      for (auto root : roots) {
----------------
int3 wrote:
> Will this work on Windows given the use of `Style::posix`?
Yes, it should work on windows just as well.  I've been running the tests on Windows :)


================
Comment at: lld/MachO/Driver.cpp:161
+    if (llvm::sys::path::is_absolute(path, llvm::sys::path::Style::posix)) {
+      for (auto root : roots) {
+        SmallString<261> buffer(root);
----------------
int3 wrote:
> codebase convention is to avoid `auto` unless the type is immediately obvious
I think its obvious given that roots is just declared above, but Im fine with replacing it with `StringRef`.


================
Comment at: lld/MachO/Driver.cpp:164
+        llvm::sys::path::append(buffer, path);
+        if (isDirectory(optionLetter, buffer.str()))
+          paths.push_back(saver.save(buffer.str()));
----------------
int3 wrote:
> tested it out locally and realized that these `isDirectory` checks are now going to spew dir-not-found errors... we should probably factor out the error logging
Hmm, would you prefer to use `llvm::sys::fs::is_directory` instead?


================
Comment at: lld/MachO/Driver.cpp:423
+  std::vector<StringRef> roots;
+  for (const auto arg : args.filtered(OPT_syslibroot))
+    roots.push_back(arg->getValue());
----------------
int3 wrote:
> avoid auto
`const auto arg` is pretty common, but sure, `Arg *arg` is shorter.


================
Comment at: lld/test/MachO/syslibroot.test:21-22
+
+# NOTE: the match here is fuzzy because the default search paths exist on Linux
+# and macOS, but not on Windows.
+RUN: lld -flavor darwinnew -v -syslibroot /var/empty -syslibroot / 2>&1 | FileCheck %s -check-prefix CHECK-SYSLIBROOT-IGNORED
----------------
int3 wrote:
> which is the fuzzy match here? not sure I follow...
`/var/empty` is not being checked.


================
Comment at: lld/test/MachO/syslibroot.test:23
+# and macOS, but not on Windows.
+RUN: lld -flavor darwinnew -v -syslibroot /var/empty -syslibroot / 2>&1 | FileCheck %s -check-prefix CHECK-SYSLIBROOT-IGNORED
+
----------------
int3 wrote:
> is `/var/empty` also going to be empty on Windows? might be better to create our own empty tmp directory instead
No, `/var/empty` will not exist on Windows, but the fuzzy match takes care of that.  The intent of the `/var/empty` is ensure that a non-sysroot is not treated as a sysroot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82252