[lld-macho] Handle filename being passed in -lto_object_path

[BertalanD]

Summary

Clang passes a filename rather than a directory in -lto_object_path when
using FullLTO. Previously, it was always treated it as a directory, so
lld would crash when it attempted to create temporary files inside it.

[int3]

lld/MachO/LTO.cpp

145

create_directories is fine if the directory already exists, so this check isn't necessary

More generally, the nice way to do FS operations is to attempt things first and then handle any errors. It doesn't really matter in this case, but there's always the possibility that e.g. a directory gets removed by say tmpcleaner between your check and the actual dir creation attempt. (And it's my bad for not adding any error checking for the existing create_directories call...)

So my suggestion is to do the create_directories call first and then dispatch on its std::error_code return value to determine objPathIsDir.

150–153

hm, I wonder if there's a more elegant way of checking whether full or thinLTO is being used. Is this an idiom used elsewhere in the LLVM codebase too?

[BertalanD]

check error code after fs::create_directories call

[BertalanD]

lld/MachO/LTO.cpp

150–153

I looked through ld.lld's source code, and didn't see any full vs thin LTO checks there. libLTO does export a method named LTOModule::isThinLTO which is used by ld64, but I couldn't find a similar method in llvm::lto::LTO.

[int3]

lld/MachO/LTO.cpp

150–153

@MaskRay might you happen to know of a way to query which LTO approach was used?

[BertalanD]

lld/MachO/LTO.cpp

145

I need to backtrack on this change. fs::create_directories(path) succeeds even if path is not a directory, contrary to its documentation (mkdir returns EEXIST even if the existing path is a file, we can't differentiate between files/directories that way). Looks like we can't avoid a fs::is_directory call after all.

unfortunate :/

[BertalanD]

Re-added the fs::is_directory call; added an error message if fs::create_directories fails

[int3]

lld/MachO/LTO.cpp

146–147

can we have a test that covers this error condition + the one below? thanks!

[BertalanD]

lld/MachO/LTO.cpp

146–147

Sure!

For the "cannot create LTO object path" case, I will pass a read-only directory in -object_path_lto. For the other error, I will mix a ThinLTO and a full LTO object (which isn't handled correctly by ld64: https://openradar.appspot.com/FB10663394)

[BertalanD]

Added a test for the new errors

[BertalanD]

Would it be okay to close https://github.com/llvm/llvm-project/issues/54805 with this?

As far as I can tell, the crash described there didn't have anything to do with the (invalid) --plugin-opt=-lto-embed-bitcode=optimized argument, but was due to Clang passing a temporary file instead of a directory in -lto_object_path because Full LTO was used.

[int3]

Yeah I think so. If they need further help they can always reopen the issue

[BertalanD]

Disable error test on Windows as chmod 400 doesn't have any effect there.

lld/MachO/LTO.cpp

std::vector<ObjFile *> BitcodeCompiler::compile() {
 
   if (config->saveTemps) {
     if (!buf[0].empty())
       saveBuffer(buf[0], config->outputFile + ".lto.o");
     for (unsigned i = 1; i != maxTasks; ++i)
       saveBuffer(buf[i], config->outputFile + Twine(i) + ".lto.o");
   }
 
-  if (!config->ltoObjPath.empty())
-    fs::create_directories(config->ltoObjPath);
+  // In ThinLTO mode, Clang passes a temporary directory in -object_path_lto,
+  // while the argument is a single file in FullLTO mode.
+  bool objPathIsDir = true;
+  if (!config->ltoObjPath.empty()) {
+    if (std::error_code ec = fs::create_directories(config->ltoObjPath))
+      fatal("cannot create LTO object path " + config->ltoObjPath + ": " +
+            ec.message());
+
+    if (!fs::is_directory(config->ltoObjPath)) {
+      objPathIsDir = false;
+      unsigned objCount =
+          count_if(buf, [](const SmallString<0> &b) { return !b.empty(); });
+      if (objCount > 1)
+        fatal("-object_path_lto must specify a directory when using ThinLTO");
+    }
+  }
 
   std::vector<ObjFile *> ret;
   for (unsigned i = 0; i != maxTasks; ++i) {
     if (buf[i].empty())
       continue;
     SmallString<261> filePath("/tmp/lto.tmp");
     uint32_t modTime = 0;
     if (!config->ltoObjPath.empty()) {
       filePath = config->ltoObjPath;
+      if (objPathIsDir)
         path::append(filePath, Twine(i) + "." +
                                    getArchitectureName(config->arch()) +
                                    ".lto.o");
       saveBuffer(buf[i], filePath);
       modTime = getModTime(filePath);
     }
     ret.push_back(make<ObjFile>(
         MemoryBufferRef(buf[i], saver().save(filePath.str())), modTime, ""));
   }
   for (std::unique_ptr<MemoryBuffer> &file : files)
     if (file)
       ret.push_back(make<ObjFile>(*file, 0, ""));
   return ret;
 }

lld/test/MachO/lto-object-path.ll

 ; REQUIRES: x86
 ; UNSUPPORTED: system-windows
 
 ; RUN: rm -rf %t; mkdir %t
 ; RUN: llvm-as %s -o %t/test.o
 
 ; RUN: %lld %t/test.o -o %t/test
 ; RUN: llvm-nm -pa %t/test | FileCheck %s --check-prefixes CHECK,NOOBJPATH
 
 ; RUN: %lld %t/test.o -o %t/test -object_path_lto %t/lto-temps
-; RUN: llvm-nm -pa %t/test | FileCheck %s --check-prefixes CHECK,OBJPATH -DDIR=%t/lto-temps
+; RUN: llvm-nm -pa %t/test | FileCheck %s --check-prefixes CHECK,OBJPATH-DIR -DDIR=%t/lto-temps
+
+;; check that the object path can be an existing file
+; RUN: touch %t/lto-tmp.o
+; RUN: %lld %t/test.o -o %t/test -object_path_lto %t/lto-tmp.o
+; RUN: llvm-nm -pa %t/test | FileCheck %s --check-prefixes CHECK,OBJPATH-FILE -DFILE=%t/lto-tmp.o
+
 
-; CHECK:          0000000000000000                - 00 0000    SO /tmp/test.cpp
-; NOOBJPATH-NEXT: 0000000000000000                - 03 0001   OSO /tmp/lto.tmp
+; CHECK:             0000000000000000                - 00 0000    SO /tmp/test.cpp
+; NOOBJPATH-NEXT:    0000000000000000                - 03 0001   OSO /tmp/lto.tmp
 ;; check that modTime is nonzero when `-object_path_lto` is provided
-; OBJPATH-NEXT:   {{[0-9a-f]*[1-9a-f]+[0-9a-f]*}} - 03 0001   OSO [[DIR]]/0.x86_64.lto.o
-; CHECK-NEXT:     {{[0-9a-f]+}}                   - 01 0000   FUN _main
-; CHECK-NEXT:     0000000000000001                - 00 0000   FUN
-; CHECK-NEXT:     0000000000000000                - 01 0000    SO
-; CHECK-NEXT:     {{[0-9a-f]+}}                   T _main
+; OBJPATH-DIR-NEXT:  {{[0-9a-f]*[1-9a-f]+[0-9a-f]*}} - 03 0001   OSO [[DIR]]/0.x86_64.lto.o
+; OBJPATH-FILE-NEXT: {{[0-9a-f]*[1-9a-f]+[0-9a-f]*}} - 03 0001   OSO [[FILE]]
+; CHECK-NEXT:        {{[0-9a-f]+}}                   - 01 0000   FUN _main
+; CHECK-NEXT:        0000000000000001                - 00 0000   FUN
+; CHECK-NEXT:        0000000000000000                - 01 0000    SO
+; CHECK-NEXT:        {{[0-9a-f]+}}                   T _main
 
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 define void @main() #0 !dbg !4 {
   ret void
 }