Enable debug fission for thinLTO linked via gold-plugin

[yunlian]

Summary

This enables debug fission on implicit ThinLTO when linked with gold. It will put the .dwo files in a directory specified by user.

[pcc]

Please upload your patch with context.

include/llvm/LTO/Config.h

77

Rename to match coding conventions: DwoDir.

include/llvm/LTO/LTO.h

169

Remove this field.

lib/LTO/LTOBackend.cpp

439

The module identifier is not enough to uniquely identify a .dwo file, as there could be multiple inputs with the same identifier. It could also contain slashes (which would require creating parent directories) or other characters which are invalid to use in file names. Probably the simplest thing to do is use the task identifier.

488

Move all of this logic into codegen; that way, you don't need to pass a file name back. If fission is enabled we should call addPassesToEmitFile on a temporary file, and then have objcopy operate on that file and finally open it and write its contents to Stream->OS.

[yunlian]

lib/LTO/LTOBackend.cpp

439

I think it should be good enough in normal cases.
Below are some example identifier when I link chrome.

./obj/device/bluetooth/public/mojom/mojom/uuid.mojom.o
./obj/device/bluetooth/libbluetooth.a.llvm.19039330.bluetooth_adapter_client.cc

For .o, it is the name (path included) of the input bitecode. For archives, it is the name of the archive appended by offset and the source file name.

The only thing that it does not work is that, the same input files are shared by different binaries and the binaries are linked in the same time.

The module name implies that we may need to create sub directories to store the .dwo files. I think that should be fine.

[pcc]

lib/LTO/LTOBackend.cpp

439

I think it should be good enough in normal cases.

Yes, but the task identifier is good enough in all cases, and it requires less code.

[yunlian]

lib/LTO/LTOBackend.cpp

439

I use the module identifier without directory information and the Task as the .dwo file name.

[pcc]

A reminder to add context (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

Test cases please. Also I think you will need to add the dwo path and the task id to the cache key if split dwarf is enabled.

include/llvm/LTO/Config.h

76

Drop the "when using ThinLTO" part because this should also work with regular LTO.

lib/LTO/LTOBackend.cpp

292

if (auto EC = sys::...

305

Indentation

308

Do you need this line?

310

Remove braces and if (auto EC = sys::...

311

Include EC.message() in error.

323–324

Add error handling.

327

Add error handling.

[pcc]

In D44792#1047010, @pcc wrote:

A reminder to add context (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

Test cases please. Also I think you will need to add the dwo path and the task id to the cache key if split dwarf is enabled.

Please address.

lib/LTO/LTOBackend.cpp

294

This can just be llvm::raw_fd_ostream OS(FD, true); because you aren't transferring the ownership anywhere.

328

I wouldn't bother with the arguments, I'd probably say something like Failed to extract dwo: exit code foo. Same below.

[yunlian]

In D44792#1047010, @pcc wrote:

A reminder to add context (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

Test cases please. Also I think you will need to add the dwo path and the task id to the cache key if split dwarf is enabled.

I added the dwo dir to cache key.
About the test cases, I am not sure how to do that in my case.
Ideally, it should build a binary and I can check the output of readelf.
But this needs clang. Can we call clang in a unittest of llvm?

[pcc]

In D44792#1050454, @yunlian wrote:

In D44792#1047010, @pcc wrote:

A reminder to add context (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

Test cases please. Also I think you will need to add the dwo path and the task id to the cache key if split dwarf is enabled.

I added the dwo dir to cache key.
About the test cases, I am not sure how to do that in my case.
Ideally, it should build a binary and I can check the output of readelf.
But this needs clang. Can we call clang in a unittest of llvm?

You shouldn't need clang to test ThinLTO. See for example any of the tests in test/tools/gold/X86/thinlto*.ll.

[yunlian]

You shouldn't need clang to test ThinLTO. See for example any of the tests in test/tools/gold/X86/thinlto*.ll.

Thanks, It seems I can to pass -shared to gold to avoid invoking clang. A test case was uploaded.

[pcc]

lib/LTO/LTO.cpp

139 ↗

(On Diff #140854)

You don't need the if condition here.

test/tools/gold/X86/split-dwarf.ll

1 ↗

(On Diff #140854)

I don't think this is necessary, as long as you use LLVM tools.

3 ↗

(On Diff #140854)

Prefer to specify absolute paths to commands to avoid depending on the current directory.

7 ↗

(On Diff #140854)

We generally don't depend on GNU binutils in our tests. You probably want to pass the path to the just-built llvm-objcopy here.

9 ↗

(On Diff #140854)

This test should start with textual IR instead of a .bc file. Use the opt command to create a .bc file from your test input and then pass that to gold.

15 ↗

(On Diff #140854)

Similarly, don't depend on binutils here. Can this be tested with llvm-dwarfdump?

[yunlian]

lib/LTO/LTO.cpp

139 ↗

(On Diff #140854)

Will do

test/tools/gold/X86/split-dwarf.ll

1 ↗

(On Diff #140854)

The reason I want to exclude windows platform is I want to use 'which' command line, if I can remove that, I will remove this line too.

7 ↗

(On Diff #140854)

How can I get the path of the llvm-objcopy? I can run llvm-* command line inside llvm-lit without specifying the path. But this is one is a little bit tricky, it needs the full path of the objcopy.

9 ↗

(On Diff #140854)

If I use llvm-as or opt to create a .bc file from .ll and do the same test, the .dwo file name changes from split-dwarf.dwo to ld-temp.dwo. Is that expected?

15 ↗

(On Diff #140854)

llvm-dwarfdump works here.
Will do.

[pcc]

test/tools/gold/X86/split-dwarf.ll

7 ↗

(On Diff #140854)

Create a ToolSubst for %llvm-objcopy in llvm/test/lit.cfg.py and use it here.

9 ↗

(On Diff #140854)

Your .bc files don't have a summary, so they are being compiled with full LTO instead of ThinLTO. Pass -module-summary to opt to get ThinLTO object files.

Come to think of it, we probably want to test both full and ThinLTO. Can you construct a test with one full LTO input file and one ThinLTO?

[yunlian]

test/tools/gold/X86/split-dwarf.ll

3 ↗

(On Diff #140854)

It seems that %t is an absolute path already.

[pcc]

test/tools/gold/X86/split-dwarf.ll

3 ↗

(On Diff #140854)

Yes but you are passing a relative path as dwo_dir. Pass %t/dwo_dir instead and remove this line.

[pcc]

test/lit.cfg.py

139 ↗

(On Diff #140999)

%llvm-objcopy (and elsewhere)

Can you use FindTool here?

[pcc]

test/lit.cfg.py

139 ↗

(On Diff #140999)

Looks like the %llvm-objcopy part isn't done.

[yunlian]

test/lit.cfg.py

139 ↗

(On Diff #140999)

I did not use %llvm-objcopy, I used %objcopy in my test instead. Do I still need that?

[pcc]

test/lit.cfg.py

139 ↗

(On Diff #140999)

Yes because we want to be clear that this is llvm-objcopy and not some other objcopy.

[pcc]

LGTM

tools/gold/gold-plugin.cpp

811–819

You should be able to set all of these without the if.

[yunlian]

rebase to trunk.

[yunlian]

Fix an indent error during rebase.

include/llvm/LTO/Config.h

   /// Sample PGO profile path.
   std::string SampleProfile;
 
+  /// The directory to store .dwo files when using ThinLTO.
+  std::string DWO_Dir;
+
+  /// The objcopy binary used to extract dwo files.
+  std::string Objcopy;
+
   /// Optimization remarks file path.
   std::string RemarksFilename = "";
 

include/llvm/LTO/LTO.h

 /// destructor.
 class NativeObjectStream {
 public:
-  NativeObjectStream(std::unique_ptr<raw_pwrite_stream> OS) : OS(std::move(OS)) {}
+  NativeObjectStream(std::unique_ptr<raw_pwrite_stream> OS,
+                     std::string FileName=std::string())
+      : OS(std::move(OS)), FileName(FileName) {}
   std::unique_ptr<raw_pwrite_stream> OS;
+  std::string FileName;
   virtual ~NativeObjectStream() = default;
 };
 

lib/LTO/LTOBackend.cpp

 #include "llvm/Passes/PassBuilder.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Program.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Target/TargetMachine.h"

   return !Conf.PostOptModuleHook || Conf.PostOptModuleHook(Task, Mod);
 }
 
-void codegen(Config &Conf, TargetMachine *TM, AddStreamFn AddStream,
+std::string codegen(Config &Conf, TargetMachine *TM, AddStreamFn AddStream,
              unsigned Task, Module &Mod) {
   if (Conf.PreCodeGenModuleHook && !Conf.PreCodeGenModuleHook(Task, Mod))
-    return;
+    return std::string();
 
   auto Stream = AddStream(Task);
   legacy::PassManager CodeGenPasses;
   if (TM->addPassesToEmitFile(CodeGenPasses, *Stream->OS, Conf.CGFileType))
     report_fatal_error("Failed to setup codegen");
   CodeGenPasses.run(Mod);
+  return Stream->FileName;
 }
 
 void splitCodeGen(Config &C, TargetMachine *TM, AddStreamFn AddStream,

 
   std::unique_ptr<TargetMachine> TM = createTargetMachine(Conf, *TOrErr, Mod);
 
+  if (!Conf.DWO_Dir.empty()) {
+    SmallString<1024> DwarfFile(Conf.DWO_Dir);
+    llvm::sys::path::append(DwarfFile, Mod.getModuleIdentifier() + ".dwo");
+    TM->Options.MCOptions.SplitDwarfFile = DwarfFile.str().str();
+  }
   if (Conf.CodeGenOnly) {
     codegen(Conf, TM.get(), AddStream, Task, Mod);
     return Error::success();

            /*ExportSummary=*/nullptr, /*ImportSummary=*/&CombinedIndex))
     return Error::success();
 
-  codegen(Conf, TM.get(), AddStream, Task, Mod);
+  std::string output_file = codegen(Conf, TM.get(), AddStream, Task, Mod);
+  if (!Conf.DWO_Dir.empty()) {
+    SmallString<1024> DwarfFile(Conf.DWO_Dir);
+    sys::path::append(DwarfFile, Mod.getModuleIdentifier() + ".dwo");
+    sys::path::remove_filename(DwarfFile);
+    std::error_code EC = llvm::sys::fs::create_directories(DwarfFile.str());
+    if (EC) {
+      errs() << "Failed to create directory " << DwarfFile.c_str() << "\n";
+      return errorCodeToError(EC);
+    }
+    SmallVector<const char*, 8> ExtractArgs, StripArgs;
+    ExtractArgs.push_back(Conf.Objcopy.c_str());
+    ExtractArgs.push_back("--extract-dwo");
+    ExtractArgs.push_back(output_file.c_str());
+    ExtractArgs.push_back(TM->Options.MCOptions.SplitDwarfFile.c_str());
+    ExtractArgs.push_back(nullptr);
+    StripArgs.push_back(Conf.Objcopy.c_str());
+    StripArgs.push_back("--strip-dwo");
+    StripArgs.push_back(output_file.c_str());
+    StripArgs.push_back(nullptr);
+    sys::ExecuteAndWait(Conf.Objcopy, ExtractArgs.data());
+    sys::ExecuteAndWait(Conf.Objcopy, StripArgs.data());
+  }
   return Error::success();
 }

tools/gold/gold-plugin.cpp

   static std::string sample_profile;
   // New pass manager
   static bool new_pass_manager = false;
+  // Objcopy to debug fission.
+  static std::string objcopy;
+  // Directory to store the .dwo files.
+  static std::string dwo_dir;
 
   static void process_plugin_option(const char *opt_)
   {

     } else if (opt == "disable-verify") {
       DisableVerify = true;
     } else if (opt.startswith("sample-profile=")) {
-      sample_profile= opt.substr(strlen("sample-profile="));
+      sample_profile = opt.substr(strlen("sample-profile="));
     } else if (opt == "new-pass-manager") {
       new_pass_manager = true;
-    } else {
+    } else if (opt.startswith("objcopy=")) {
+      objcopy = opt.substr(strlen("objcopy="));
+    } else if (opt.startswith("dwo_dir=")) {
+      dwo_dir = opt.substr(strlen("dwo_dir="));
+    }else {
       // Save this option to pass to the code generator.
       // ParseCommandLineOptions() expects argv[0] to be program name. Lazily
       // add that.

   if (!options::sample_profile.empty())
     Conf.SampleProfile = options::sample_profile;
 
+  if (!options::dwo_dir.empty())
+    Conf.DWO_Dir = options::dwo_dir;
+
+  if (!options::objcopy.empty())
+    Conf.Objcopy = options::objcopy;
+
   // Use new pass manager if set in driver
   Conf.UseNewPM = options::new_pass_manager;
 

     int FD = getOutputFileName(Filename, /* TempOutFile */ !SaveTemps,
                                Files[Task].first, Task);
     return llvm::make_unique<lto::NativeObjectStream>(
-        llvm::make_unique<llvm::raw_fd_ostream>(FD, true));
+        llvm::make_unique<llvm::raw_fd_ostream>(FD, true),
+        Files[Task].first.str().str());
   };
 
   auto AddBuffer = [&](size_t Task, std::unique_ptr<MemoryBuffer> MB) {