[ThinLTO] Adding architecture name into saved object filename

[steven_wu]

Summary

For ThinLTOCodegenerator, it has an option to save the object file
outputs into a directory which is essential for debug info. Tools like lldb
and dsymutil will look for these object files for debug info.

On Darwin platform, you can link fat binaries with one single clang
driver invocation like:
$ clang -arch x86_64 -arch i386 -Wl,-object_path_lto,$TMPDIR ...
Unfornately, the output object files for one architecture is going to
overwrite the previous ones and one architecture slice will end up with
no debug info. One example for this is to turn on ThinLTO for sanitizer
dylibs in compiler-rt project.

To fix the issue, add the name for the architecture into the name of the
output object file.

rdar://problem/35482935

[dexonsmith]

Is there a good way to test this?

[steven_wu]

In D60924#1473135, @dexonsmith wrote:

Is there a good way to test this?

Not currently in LLVM. It might be possible to add a test in clang repo which requires DARWIN and ld64.

[dexonsmith]

In D60924#1473161, @steven_wu wrote:

In D60924#1473135, @dexonsmith wrote:

Is there a good way to test this?

Not currently in LLVM. It might be possible to add a test in clang repo which requires DARWIN and ld64.

Maybe we can split out the function to generate the filename, and add a unit test for that?

[steven_wu]

In D60924#1473166, @dexonsmith wrote:

In D60924#1473161, @steven_wu wrote:

In D60924#1473135, @dexonsmith wrote:

Is there a good way to test this?

Not currently in LLVM. It might be possible to add a test in clang repo which requires DARWIN and ld64.

Maybe we can split out the function to generate the filename, and add a unit test for that?

That almost looks like a test for path::append. Not sure if there is much value to add that.

[dexonsmith]

In D60924#1473176, @steven_wu wrote:

In D60924#1473166, @dexonsmith wrote:

In D60924#1473161, @steven_wu wrote:

In D60924#1473135, @dexonsmith wrote:

Is there a good way to test this?

Not currently in LLVM. It might be possible to add a test in clang repo which requires DARWIN and ld64.

Maybe we can split out the function to generate the filename, and add a unit test for that?

That almost looks like a test for path::append. Not sure if there is much value to add that.

What about an llvm-lto test? Is there a way to make it trigger this case?

My concern is that this is a subtle requirement, and it would be easy to regress without a test.

[steven_wu]

Add tests for the change. There is acutally an easy to test with llvm-lto

Also clean up the interface.

[steven_wu]

ping

[dexonsmith]

LGTM.

llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

public:
   void internalize(Module &Module, ModuleSummaryIndex &Index,
                    const lto::InputFile &File);
 
   /**
    * Perform post-importing ThinLTO optimizations.
    */
   void optimize(Module &Module);
 
+  /**
+   * Write temporary object file to SavedObjectDirectoryPath, write symlink
+   * to Cache directory if needed. Returns the path to the generated file in
+   * SavedObjectsDirectoryPath.
+   */
+  std::string writeGeneratedObject(int count, StringRef CacheEntryPath,
+                                   const MemoryBuffer &OutputBuffer);
   /**@}*/
 
 private:
   /// Helper factory to build a TargetMachine
   TargetMachineBuilder TMBuilder;
 
   /// Vector holding the in-memory buffer containing the produced binaries, when
   /// SavedObjectsDirectoryPath isn't set.

llvm/lib/LTO/ThinLTOCodeGenerator.cpp

void ThinLTOCodeGenerator::optimize(Module &TheModule) {
 
   // Optimize now
   optimizeModule(TheModule, *TMBuilder.create(), OptLevel, Freestanding);
 }
 
 /// Write out the generated object file, either from CacheEntryPath or from
 /// OutputBuffer, preferring hard-link when possible.
 /// Returns the path to the generated file in SavedObjectsDirectoryPath.
-static std::string writeGeneratedObject(int count, StringRef CacheEntryPath,
-                                        StringRef SavedObjectsDirectoryPath,
+std::string
+ThinLTOCodeGenerator::writeGeneratedObject(int count, StringRef CacheEntryPath,
                                            const MemoryBuffer &OutputBuffer) {
+  auto ArchName = TMBuilder.TheTriple.getArchName();
   SmallString<128> OutputPath(SavedObjectsDirectoryPath);
-  llvm::sys::path::append(OutputPath, Twine(count) + ".thinlto.o");
+  llvm::sys::path::append(OutputPath,
+                          Twine(count) + "." + ArchName + ".thinlto.o");
   OutputPath.c_str(); // Ensure the string is null terminated.
   if (sys::fs::exists(OutputPath))
     sys::fs::remove(OutputPath);
 
   // We don't return a memory buffer to the linker, just a list of files.
   if (!CacheEntryPath.empty()) {
     // Cache is enabled, hard-link the entry (or copy if hard-link fails).
     auto Err = sys::fs::create_hard_link(CacheEntryPath, OutputPath);
for (auto &Mod : Modules) {
         auto TheModule = loadModuleFromInput(Mod.get(), Context, false,
                                              /*IsImporting*/ false);
 
         // CodeGen
         auto OutputBuffer = codegenModule(*TheModule, *TMBuilder.create());
         if (SavedObjectsDirectoryPath.empty())
           ProducedBinaries[count] = std::move(OutputBuffer);
         else
-          ProducedBinaryFiles[count] = writeGeneratedObject(
-              count, "", SavedObjectsDirectoryPath, *OutputBuffer);
+          ProducedBinaryFiles[count] =
+              writeGeneratedObject(count, "", *OutputBuffer);
       }, count++);
     }
 
     return;
   }
 
   // Sequential linking phase
   auto Index = linkCombinedIndex();
for (auto IndexCount : ModulesOrdering) {
                             << count << " " << ModuleIdentifier << "\n");
 
           if (ErrOrBuffer) {
             // Cache Hit!
             if (SavedObjectsDirectoryPath.empty())
               ProducedBinaries[count] = std::move(ErrOrBuffer.get());
             else
               ProducedBinaryFiles[count] = writeGeneratedObject(
-                  count, CacheEntryPath, SavedObjectsDirectoryPath,
-                  *ErrOrBuffer.get());
+                  count, CacheEntryPath, *ErrOrBuffer.get());
             return;
           }
         }
 
         LLVMContext Context;
         Context.setDiscardValueNames(LTODiscardValueNames);
         Context.enableDebugTypeODRUniquing();
         auto DiagFileOrErr = lto::setupOptimizationRemarks(
for (auto IndexCount : ModulesOrdering) {
             } else {
               OutputBuffer = std::move(*ReloadedBufferOrErr);
             }
           }
           ProducedBinaries[count] = std::move(OutputBuffer);
           return;
         }
         ProducedBinaryFiles[count] = writeGeneratedObject(
-            count, CacheEntryPath, SavedObjectsDirectoryPath, *OutputBuffer);
+            count, CacheEntryPath, *OutputBuffer);
       }, IndexCount);
     }
   }
 
   pruneCache(CacheOptions.Path, CacheOptions.Policy);
 
   // If statistics were requested, print them out now.
   if (llvm::AreStatisticsEnabled())
     llvm::PrintStatistics();
   reportAndResetTimings();
 }

llvm/test/ThinLTO/X86/save_objects.ll

 
 ; Same with hot cache
 ; RUN: rm -Rf %t.thin.out
 ; RUN: rm -Rf %t.cache && mkdir %t.cache
 ; RUN: llvm-lto -thinlto-save-objects=%t.thin.out -thinlto-action=run %t2.bc  %t.bc -exported-symbol=main -thinlto-cache-dir %t.cache 
 ; RUN: ls %t.thin.out | count 2
 ; RUN: ls %t.cache | count 3
 
+; Check the name of object in directory has arch name included.
+; RUN: ls %t.thin.out | grep x86_64.thinlto.o | count 2
+
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"
 
 define void @globalfunc() #0 {
 entry:
   ret void
 }