[lld-macho] Re-root absolute input file paths if -syslibroot is specified

[int3]

update

[int3]

lld/MachO/Driver.cpp

121

Since we may return path; from this function, having it return an StringRef instead of an std::string is natural. I therefore modified findPathCombination to return an Optional<StringRef> as well.

For uniformity, findFramework should be modified similarly; I'll do it in a separate commit to avoid cluttering this diff.

lld/test/MachO/dependency-info.s

23

we now attempt to search for a bunch of rerooted paths, so there are more not-found entries

[int3]

try to get test passing on windows

[int3]

retry

[int3]

[int3]

[int3]

[int3]

[int3]

[int3]

[int3]

I give up; disabling the test on Windows

[gkm]

lld/MachO/Driver.cpp

80

What does \p mean here in this comment?

lld/test/MachO/reroot-path.s

27–28

These don't test what you want, since they succeed even without -syslibroot:

• PASS without -syslibroot %t, since %t/%:t/bar.a is a real path, so the syslibroot is unused
• PASS with -syslibroot %t and s/%t/%:t/bar.a/%t/bar.a/ for a syslibroot-relative path
• FAIL (as expected) without -syslibroot %t and s/%t/%:t/bar.a/%t/bar.a/, since syslibroot is needed to reach the syslibroot-relative path

29–30

Same behavior here for libbar.dylib vs. bar.a

31–33

Same

[gkm]

lld/test/MachO/reroot-path.s

27–28

I believe the second case is what you want: with -syslibroot %t and s/%t/%:t/bar.a/%t/bar.a/

[int3]

lld/MachO/Driver.cpp

80

Parameter. It's used by Doxygen: https://www.doxygen.nl/manual/commands.html#cmdp

lld/test/MachO/reroot-path.s

27–28

yea good catch. I'll check the actual path accessed with -t.

[int3]

update

[gkm]

lld/test/MachO/reroot-path.s

27–30

Don't we also want to test with %t/bar.a and %t/libbar.dylib to demonstrate that -syslibroot %t was honored %t prepended to find the file?

[int3]

lld/test/MachO/reroot-path.s

27–30

oops, yeah. This should just be "%t/libbar.dylib", testing "%t/:%t/libbar.dylib" is unnecessary... I had this right in an earlier version of the diff :p

[int3]

fix

[gkm]

lld/test/MachO/reroot-path.s

29–30

Almost there! These also need to drop the %:t/ component and become %t/libbar.dylib on the lld command line.

[int3]

lld/test/MachO/reroot-path.s

29–30

It looks right to me as-is. Are you looking at the right version of the diff?

[gkm]

lld/test/MachO/reroot-path.s

29–30

Line 29 is good now with %t/libbar.dylib, but 28 still contains %t/%:t/libbar.dylib

[int3]

lld/test/MachO/reroot-path.s

29–30

ugh, yes. Sorry, not sure how I missed that

[int3]

fix

[gkm]

😄 😄 😄

[thakis]

Does this do the right thing with --reproduce? (cf createResponseFile() in DriverUtils.cpp)

[int3]

Does this do the right thing with --reproduce? (cf createResponseFile() in DriverUtils.cpp)

... kinda. It mostly works, except when there's another file at the non-rerooted absolute path, in which case we emit the wrong path. Fix coming up in a bit.

lld/MachO/Driver.cpp

case OPT_dylib:
     return MH_DYLIB;
   case OPT_execute:
     return MH_EXECUTE;
   default:
     llvm_unreachable("internal error");
   }
 }
 
-static Optional<std::string>
-findAlongPathsWithExtensions(StringRef name, ArrayRef<StringRef> extensions) {
+// Search for all possible combinations of `{root}/{name}.{extension}`.
+// If \p extensions are not specified, then just search for `{root}/{name}`.
+static Optional<StringRef>
+findPathCombination(const Twine &name, const std::vector<StringRef> &roots,
+                    ArrayRef<StringRef> extensions = {""}) {
   SmallString<261> base;
-  for (StringRef dir : config->librarySearchPaths) {
+  for (StringRef dir : roots) {
     base = dir;
-    path::append(base, Twine("lib") + name);
+    path::append(base, name);
     for (StringRef ext : extensions) {
       Twine location = base + ext;
       if (fs::exists(location))
-        return location.str();
+        return saver.save(location.str());
       else
         depTracker->logFileNotFound(location);
     }
   }
   return {};
 }
 
-static Optional<std::string> findLibrary(StringRef name) {
+static Optional<StringRef> findLibrary(StringRef name) {
   if (config->searchDylibsFirst) {
-    if (Optional<std::string> path =
-            findAlongPathsWithExtensions(name, {".tbd", ".dylib"}))
+    if (Optional<StringRef> path = findPathCombination(
+            "lib" + name, config->librarySearchPaths, {".tbd", ".dylib"}))
       return path;
-    return findAlongPathsWithExtensions(name, {".a"});
+    return findPathCombination("lib" + name, config->librarySearchPaths,
+                               {".a"});
   }
-  return findAlongPathsWithExtensions(name, {".tbd", ".dylib", ".a"});
+  return findPathCombination("lib" + name, config->librarySearchPaths,
+                             {".tbd", ".dylib", ".a"});
+}
+
+// If -syslibroot is specified, absolute paths to non-object files may be
+// rerooted.
+static StringRef rerootPath(StringRef path) {
+  if (!path::is_absolute(path, path::Style::posix) || path.endswith(".o"))
+    return path;
+
+  if (Optional<StringRef> rerootedPath =
+          findPathCombination(path, config->systemLibraryRoots))
+    return *rerootedPath;
+
+  return path;
 }
 
 static Optional<std::string> findFramework(StringRef name) {
   SmallString<260> symlink;
   StringRef suffix;
   std::tie(name, suffix) = name.split(",");
   for (StringRef dir : config->frameworkSearchPaths) {
     symlink = dir;
if (newFile) {
     if (config->printEachFile && magic != file_magic::archive)
       message(toString(newFile));
     inputFiles.insert(newFile);
   }
   return newFile;
 }
 
 static void addLibrary(StringRef name, bool isWeak) {
-  if (Optional<std::string> path = findLibrary(name)) {
+  if (Optional<StringRef> path = findLibrary(name)) {
     auto *dylibFile = dyn_cast_or_null<DylibFile>(addFile(*path, false));
     if (isWeak && dylibFile)
       dylibFile->forceWeakImport = true;
     return;
   }
   error("library not found for -l" + name);
 }
 

 }
 
 static void addFileList(StringRef path) {
   Optional<MemoryBufferRef> buffer = readFile(path);
   if (!buffer)
     return;
   MemoryBufferRef mbref = *buffer;
   for (StringRef path : args::getLines(mbref))
-    addFile(path, false);
+    addFile(rerootPath(path), false);
 }
 
 // An order file has one entry per line, in the following format:
 //
 //   <cpu>:<object file>:<symbol name>
 //
 // <cpu> and <object file> are optional. If not specified, then that entry
 // matches any symbol of that name. Parsing this format is not quite
void createFiles(const InputArgList &args) {
   // Other options should be handled via filtered() and/or getLastArg().
   for (const Arg *arg : args) {
     const Option &opt = arg->getOption();
     warnIfDeprecatedOption(opt);
     warnIfUnimplementedOption(opt);
 
     switch (opt.getID()) {
     case OPT_INPUT:
-      addFile(arg->getValue(), false);
+      addFile(rerootPath(arg->getValue()), false);
       break;
     case OPT_weak_library:
-      if (auto *dylibFile =
-              dyn_cast_or_null<DylibFile>(addFile(arg->getValue(), false)))
+      if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
+              addFile(rerootPath(arg->getValue()), false)))
         dylibFile->forceWeakImport = true;
       break;
     case OPT_filelist:
       addFileList(arg->getValue());
       break;
     case OPT_force_load:
-      addFile(arg->getValue(), true);
+      addFile(rerootPath(arg->getValue()), true);
       break;
     case OPT_l:
     case OPT_weak_l:
       addLibrary(arg->getValue(), opt.getID() == OPT_weak_l);
       break;
     case OPT_framework:
     case OPT_weak_framework:
       addFramework(arg->getValue(), opt.getID() == OPT_weak_framework);

lld/test/MachO/dependency-info.s

 # CHECK: lld-version: {{.*}}LLD {{.*}}
 
 # CHECK-DAG: input-file: {{.*}}{{[/\]}}bar.a
 # CHECK-DAG: input-file: {{.*}}{{[/\]}}libfoo.dylib
 # CHECK-DAG: input-file: {{.*}}{{[/\]}}libSystem.tbd
 # CHECK-DAG: input-file: {{.*}}{{[/\]}}main.o
 # CHECK-DAG: input-file: {{.*}}bar.o
 
-# CHECK-NEXT: not-found: {{.*}}{{[/\]}}libdyld.{{.*}}
+# CHECK: not-found: {{.*}}{{[/\]}}libdyld.{{.*}}
 ## There could be more not-found here but we are not checking those because it's brittle.
 
 # CHECK: output-file: {{.*}}{{[/\]}}test.out
 
 #--- foo.s
 .globl __Z3foo
 __Z3foo:
   ret

lld/test/MachO/reroot-path.s

+# REQUIRES: x86
+# UNSUPPORTED: system-windows
+## FIXME: In principle this test should pass on Windows
+# RUN: rm -rf %t; split-file %s %t
+
+## This test verifies that we attempt to re-root absolute paths if -syslibroot
+## is specified. Therefore we would like to be able to specify an absolute path
+## without worrying that it may match an actual file on the system outside the
+## syslibroot. `chroot` would do the job but isn't cross-platform, so I've used
+## this %t/%:t hack instead.
+# RUN: mkdir -p %t/%:t
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/foo.s -o %t/foo.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/bar.s -o %t/bar.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/test.s -o %t/test.o
+
+## bar.a is under %t/%:t, and so verifies that rerooting happens. foo.a isn't,
+## and therefore verifies that we still fall back to the original path if no
+## file exists at the rerooted path.
+# RUN: llvm-ar rcs %t/foo.a %t/foo.o
+# RUN: %lld -dylib %t/foo.o -o %t/libfoo.dylib
+# RUN: llvm-ar rcs %t/%:t/bar.a %t/bar.o
+# RUN: %lld -dylib %t/bar.o -o %t/%:t/libbar.dylib
+
+## Test our various file-loading flags to make sure all bases are covered.
+# RUN: %lld -lSystem -syslibroot %t %t/foo.a %t/bar.a %t/test.o -o /dev/null -t | FileCheck %s -DDIR="%t/%:t"
+# RUN: %lld -lSystem -syslibroot %t -force_load %t/foo.a -force_load %t/bar.a %t/test.o -o /dev/null -t | FileCheck %s -DDIR="%t/%:t"
+# RUN: %lld -lSystem -syslibroot %t %t/libfoo.dylib %t/libbar.dylib %t/test.o -o /dev/null -t | FileCheck %s -DDIR="%t/%:t"
+# RUN: %lld -lSystem -syslibroot %t -weak_library %t/libfoo.dylib -weak_library %t/libbar.dylib %t/test.o -o /dev/null -t | FileCheck %s -DDIR="%t/%:t"
+# RUN: echo "%t/libfoo.dylib" > %t/filelist
+# RUN: echo "%t/libbar.dylib" >> %t/filelist
+# RUN: %lld -lSystem -syslibroot %t -filelist %t/filelist %t/test.o -o /dev/null -t | FileCheck %s -DDIR="%t/%:t"
+# CHECK: [[DIR]]/{{(lib)?}}bar
+
+## Paths to object files don't get rerooted.
+# RUN: mv %t/bar.o %t/%:t/bar.o
+# RUN: not %lld -lSystem -syslibroot %t %t/foo.o %t/bar.o %t/test.o -o \
+# RUN:   /dev/null 2>&1 | FileCheck %s --check-prefix=OBJ
+# OBJ: error: cannot open {{.*[\\/]}}bar.o: {{[Nn]}}o such file or directory
+
+## Now create a "decoy" libfoo.dylib under %t/%:t to demonstrate that the
+## rerooted path takes precedence over the original path. We will get an
+## undefined symbol error since we aren't loading %t/libfoo.dylib.
+# RUN: cp %t/%:t/libbar.dylib %t/%:t/libfoo.dylib
+# RUN: not %lld -lSystem -syslibroot %t %t/libfoo.dylib %t/libbar.dylib %t/test.o \
+# RUN:   -o /dev/null 2>&1 | FileCheck %s --check-prefix=UNDEF
+# UNDEF: error: undefined symbol: _foo
+
+#--- foo.s
+.globl _foo
+_foo:
+
+#--- bar.s
+.globl _bar
+_bar:
+
+#--- test.s
+.text
+.globl _main
+
+_main:
+  callq _foo
+  callq _bar
+  ret