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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

Oh yeah, don't forget to remove the `HelpHidden` tag from syslibroot's definition in Options.td



================
Comment at: lld/MachO/Driver.cpp:136
+      for (auto root : roots) {
+        SmallString<261> buffer(root);
+        llvm::sys::path::append(buffer, path);
----------------
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?


================
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) {
----------------
Will this work on Windows given the use of `Style::posix`?


================
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
----------------
which is the fuzzy match here? not sure I follow...


================
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
+
----------------
is `/var/empty` also going to be empty on Windows? might be better to create our own empty tmp directory instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82252