[lld-macho] Ensure cached objects are affected by `-object_path_lto`

[lgrey]

Summary

This refactors LTO compile to look more like COFF, where cache hits and misses are all funneled through the same code path.

Previously, cache hits were *not* being saved to -object_path_lto, which led to them sometimes falling out of the cache before dsymutil could process them. As a side effect of the refactor, cached objects are now saved with -save-temps as well, which seems desirable.

(Deleted lld/test/MachO/lto-cache-dsymutil.ll and rolled it into lld/test/MachO/lto-object-path.ll, since the cache-only, non object path approach is unreliable anyway).

[thakis]

Making this more like COFF is generally good, but isn't -cache_path_lto supposed to control where these files go in ld64 (and hence ld64.lld)?

[steven_wu]

LGTM.

For ld64, -cache_path_lto controls where the ThinLTO cache is and -object_path_lto controls where to write MachO object file from thinLTO codegen for dsymutil to use. The behavior in this patch is correct.

[steven_wu]

One thing missing is that in libLTO, the cached object is written to -object_path_lto using a hard-link. That is intentional to reduce the disk usage and it is faster than write a file buffer to output file. Might want to consider doing that in lld for future.

[thakis]

Thanks for the clarification, Steven!

[lgrey]

In D131624#3716584, @steven_wu wrote:

One thing missing is that in libLTO, the cached object is written to -object_path_lto using a hard-link. That is intentional to reduce the disk usage and it is faster than write a file buffer to output file. Might want to consider doing that in lld for future.

This makes a lot of sense, thanks! I'll do it in a follow-up for revertability.

lld/MachO/LTO.cpp

checkError(ltoObj->run(
         return std::make_unique<CachedFileStream>(
             std::make_unique<raw_svector_ostream>(buf[task]));
       },
       cache));
 
   if (!config->thinLTOCacheDir.empty())
     pruneCache(config->thinLTOCacheDir, config->thinLTOCachePolicy);
 
-  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");
-  }
-
   // 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())
+  for (unsigned i = 0; i < maxTasks; ++i) {
+    // Get the native object contents either from the cache or from memory.  Do
+    // not use the cached MemoryBuffer directly to ensure dsymutil does not
+    // race with the cache pruner.
+    StringRef objBuf;
+    if (files[i])
+      objBuf = files[i]->getBuffer();
+    else
+      objBuf = buf[i];
+    if (objBuf.empty())
       continue;
+
+    // FIXME: should `saveTemps` and `ltoObjPath` use the same file name?
+    if (config->saveTemps)
+      saveBuffer(objBuf,
+                 config->outputFile + ((i == 0) ? "" : Twine(i)) + ".lto.o");
+
     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);
+      saveBuffer(objBuf, filePath);
       modTime = getModTime(filePath);
     }
     ret.push_back(make<ObjFile>(
-        MemoryBufferRef(buf[i], saver().save(filePath.str())), modTime, ""));
+        MemoryBufferRef(objBuf, 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-cache-dsymutil.ll

-; REQUIRES: x86
-
-; RUN: rm -rf %t; mkdir %t
-; RUN: opt -module-hash -module-summary %s -o %t/foo.o
-; RUN: %lld -cache_path_lto %t/cache -o %t/test %t/foo.o
-
-;; Check that dsymutil is able to read the .o file out of the thinlto cache.
-; RUN: dsymutil -f -o - %t/test | llvm-dwarfdump - | FileCheck %s
-; CHECK: DW_AT_name ("test.cpp")
-
-
-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
-}
-
-!llvm.module.flags = !{ !0, !1 }
-!llvm.dbg.cu = !{!2}
-
-!0 = !{i32 7, !"Dwarf Version", i32 4}
-!1 = !{i32 2, !"Debug Info Version", i32 3}
-!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, emissionKind: FullDebug)
-!3 = !DIFile(filename: "test.cpp", directory: "/tmp")
-!4 = distinct !DISubprogram(name: "main", scope: !3, file: !3, line: 1, type: !5, scopeLine: 1, unit: !2)
-!5 = !DISubroutineType(types: !6)
-!6 = !{}

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: opt -thinlto-bc %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-DIR -DDIR=%t/lto-temps
 
+;; Ensure object path is still used if the cache is used
+; RUN: %lld %t/test.o -o %t/test -object_path_lto %t/lto-temps -prune_interval_lto 20 -cache_path_lto %t/cache --thinlto-cache-policy=cache_size_files=1:cache_size_bytes=10
+; RUN: llvm-nm -pa %t/test | FileCheck %s --check-prefixes CHECK,OBJPATH-DIR -DDIR=%t/lto-temps
+;; And that dsymutil can read the result
+; RUN: dsymutil -f -o - %t/test | llvm-dwarfdump - | FileCheck %s --check-prefix=DSYM
+
 ;; 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 that modTime is nonzero when `-object_path_lto` is provided
-; OBJPATH-DIR-NEXT:  {{[0-9a-f]*[1-9a-f]+[0-9a-f]*}} - 03 0001   OSO [[DIR]]/0.x86_64.lto.o
+; OBJPATH-DIR-NEXT:  {{[0-9a-f]*[1-9a-f]+[0-9a-f]*}} - 03 0001   OSO [[DIR]]/1.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
+; DSYM: DW_AT_name ("test.cpp")
 
 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
 }