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

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added a comment.
This revision is now accepted and ready to land.

lgtm modulo those two questions. Thanks!



================
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:
> > 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.
sgtm


================
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) {
----------------
compnerd wrote:
> 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 :)
I guess my question then is how does it work? I was under the impression that we would want the style to be `native` since the path separators are different on Windows.

(I'm working on setting up a Windows VM so I can answer these things myself in the future 😅)


================
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()));
----------------
compnerd wrote:
> 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?
seems better, thanks!


================
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
----------------
compnerd wrote:
> int3 wrote:
> > which is the fuzzy match here? not sure I follow...
> `/var/empty` is not being checked.
ah I see. Maybe make that explicit in the comment?

Also, shouldn't we be able to portably check that `/var/empty` isn't present? My understanding is that the `-syslibroot /` will make `-syslibroot /var/empty` a no-op on all platforms... is that right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82252