[llvm-objcopy][NFC] remove split dwo file creation from executeObjcopyOnBinary.

[avl]

Summary

This patch removes creation of the resulting file from the
executeObjcopyOnBinary() function. For the most use cases, the
executeObjcopyOnBinary receives output file as a parameter

• raw_ostream &Out. The splitting .dwo file is implemented differently:

file containg .dwo tables is created inside executeObjcopyOnBinary().
When objcopy functionality would be moved into separate library,
current implementation will become inconvenient. The goal of that
refactoring is to separate concerns: It might be convenient to
to do dwo tables splitting but to create resulting file differently.

[avl]

fix a mistake.

[MaskRay]

LGTM.

[jhenderson]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

329–332

You don't need the = nullptr here, I believe.

347–351

This code duplication is a real step backwards from what was there before. I think it would be far better to share the code for ihex and binary, with only a very localised if, probably inside the lambda, to choose the right function to run. Possible example:

if (Config.InputFormat == FileFormat::Binary || Config.InputFormat == FileFormat::IHex) {
    ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
        MemoryBuffer::getFileOrSTDIN(Config.InputFilename);
    if (!BufOrErr)
      return createFileError(Config.InputFilename, BufOrErr.getError());
    MemoryBufferHolder = std::move(*BufOrErr);
    ObjcopyFunc = [&](raw_ostream &OutFile) -> Error {
      switch(Config.InputFormat) {
      case FileFormat::Binary:
        return executeObjcopyOnRawBinary(Config, *MemoryBufferHolder, OutFile);
      case FileFormat::IHex:
        return executeObjcopyOnIHex(Config, *MemoryBufferHolder, OutFile);
    };
} else {
  ...
}

380–381

I think this should be pulled out into a helper variable, since it's repeated in each instance.

385

A pair of short comments at each of this and the following stage would be helpful to show which part is doing what.

[avl]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

380–381

I left this code assuming it would be deleted by by D98511.

[avl]

addressed comments.

[jhenderson]

LGTM.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

384–385

A bit more verbose than I had in mind, but it's fine.

[avl]

Thank you for the review.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

 
 template <class... Ts>
 static Error makeStringError(std::error_code EC, const Twine &Msg,
                              Ts &&... Args) {
   std::string FullMsg = (EC.message() + ": " + Msg).str();
   return createStringError(EC, FullMsg.c_str(), std::forward<Ts>(Args)...);
 }
 
-static Error splitDWOToFile(const CopyConfig &Config, const Reader &Reader,
-                            StringRef File, ElfType OutputElfType) {
-  Expected<std::unique_ptr<Object>> DWOFile = Reader.create(false);
-  if (!DWOFile)
-    return DWOFile.takeError();
-
-  auto OnlyKeepDWOPred = [&DWOFile](const SectionBase &Sec) {
-    return onlyKeepDWOPred(**DWOFile, Sec);
-  };
-  if (Error E =
-          (*DWOFile)->removeSections(Config.AllowBrokenLinks, OnlyKeepDWOPred))
-    return E;
-  if (Config.OutputArch) {
-    (*DWOFile)->Machine = Config.OutputArch.getValue().EMachine;
-    (*DWOFile)->OSABI = Config.OutputArch.getValue().OSABI;
-  }
-
-  return writeToFile(File, [&](raw_ostream &OutFile) -> Error {
-    std::unique_ptr<Writer> Writer =
-        createWriter(Config, **DWOFile, OutFile, OutputElfType);
-    if (Error E = Writer->finalize())
-      return E;
-    return Writer->write();
-  });
-}
-
 static Error dumpSectionToFile(StringRef SecName, StringRef Filename,
                                Object &Obj) {
   for (auto &Sec : Obj.sections()) {
     if (Sec.Name == SecName) {
       if (Sec.Type == SHT_NOBITS)
         return createStringError(object_error::parse_failed,
                                  "cannot dump section '%s': it has no contents",
                                  SecName.str().c_str());
if (!Config.OnlySection.empty() && !Sym.Referenced &&
       return true;
 
     return false;
   };
 
   return Obj.removeSymbols(RemoveSymbolsPred);
 }
 
+static Error keepOrRemoveDWOSections(const CopyConfig &Config, Object &Obj) {
+  assert(!Config.SplitDWO.empty());
+  assert(Config.ExtractDWO != Config.StripDWO);
+
+  if (Config.ExtractDWO) {
+    return Obj.removeSections(
+        Config.AllowBrokenLinks,
+        [&Obj](const SectionBase &Sec) { return onlyKeepDWOPred(Obj, Sec); });
+  }
+
+  if (Config.StripDWO) {
+    return Obj.removeSections(
+        Config.AllowBrokenLinks,
+        [](const SectionBase &Sec) { return isDWOSection(Sec); });
+  }
+
+  llvm_unreachable("Ambigous SplitDWO option");
+}
+
 static Error replaceAndRemoveSections(const CopyConfig &Config, Object &Obj) {
   SectionPred RemovePred = [](const SectionBase &) { return false; };
 
   // Removes:
   if (!Config.ToRemove.empty()) {
     RemovePred = [&Config](const SectionBase &Sec) {
       return Config.ToRemove.matches(Sec.Name);
     };
   }
 
-  if (Config.StripDWO || !Config.SplitDWO.empty())
+  if (Config.StripDWO)
     RemovePred = [RemovePred](const SectionBase &Sec) {
       return isDWOSection(Sec) || RemovePred(Sec);
     };
 
   if (Config.ExtractDWO)
     RemovePred = [RemovePred, &Obj](const SectionBase &Sec) {
       return onlyKeepDWOPred(Obj, Sec) || RemovePred(Sec);
     };

 
 // This function handles the high level operations of GNU objcopy including
 // handling command line options. It's important to outline certain properties
 // we expect to hold of the command line operations. Any operation that "keeps"
 // should keep regardless of a remove. Additionally any removal should respect
 // any previous removals. Lastly whether or not something is removed shouldn't
 // depend a) on the order the options occur in or b) on some opaque priority
 // system. The only priority is that keeps/copies overrule removes.
-static Error handleArgs(const CopyConfig &Config, Object &Obj,
-                        const Reader &Reader, ElfType OutputElfType) {
+static Error handleArgs(const CopyConfig &Config, Object &Obj) {
   if (Config.StripSwiftSymbols || Config.KeepUndefined)
     return createStringError(llvm::errc::invalid_argument,
                              "option not supported by llvm-objcopy for ELF");
-  if (!Config.SplitDWO.empty())
-    if (Error E =
-            splitDWOToFile(Config, Reader, Config.SplitDWO, OutputElfType))
-      return E;
 
   if (Config.OutputArch) {
     Obj.Machine = Config.OutputArch.getValue().EMachine;
     Obj.OSABI = Config.OutputArch.getValue().OSABI;
   }
 
+  if (!Config.SplitDWO.empty())
+    return keepOrRemoveDWOSections(Config, Obj);
+
   // Dump sections before add/remove for compatibility with GNU objcopy.
   for (StringRef Flag : Config.DumpSection) {
     StringRef SectionName;
     StringRef FileName;
     std::tie(SectionName, FileName) = Flag.split('=');
     if (Error E = dumpSectionToFile(SectionName, FileName, Obj))
       return E;
   }
Error executeObjcopyOnIHex(const CopyConfig &Config, MemoryBuffer &In,
                            raw_ostream &Out) {
   IHexReader Reader(&In);
   Expected<std::unique_ptr<Object>> Obj = Reader.create(true);
   if (!Obj)
     return Obj.takeError();
 
   const ElfType OutputElfType =
       getOutputElfType(Config.OutputArch.getValueOr(MachineInfo()));
-  if (Error E = handleArgs(Config, **Obj, Reader, OutputElfType))
+  if (Error E = handleArgs(Config, **Obj))
     return E;
   return writeOutput(Config, **Obj, Out, OutputElfType);
 }
 
 Error executeObjcopyOnRawBinary(const CopyConfig &Config, MemoryBuffer &In,
                                 raw_ostream &Out) {
   uint8_t NewSymbolVisibility =
       Config.ELF->NewSymbolVisibility.getValueOr((uint8_t)ELF::STV_DEFAULT);
   BinaryReader Reader(&In, NewSymbolVisibility);
   Expected<std::unique_ptr<Object>> Obj = Reader.create(true);
   if (!Obj)
     return Obj.takeError();
 
   // Prefer OutputArch (-O<format>) if set, otherwise fallback to BinaryArch
   // (-B<arch>).
   const ElfType OutputElfType =
       getOutputElfType(Config.OutputArch.getValueOr(MachineInfo()));
-  if (Error E = handleArgs(Config, **Obj, Reader, OutputElfType))
+  if (Error E = handleArgs(Config, **Obj))
     return E;
   return writeOutput(Config, **Obj, Out, OutputElfType);
 }
 
 Error executeObjcopyOnBinary(const CopyConfig &Config,
                              object::ELFObjectFileBase &In, raw_ostream &Out) {
   ELFReader Reader(&In, Config.ExtractPartition);
   Expected<std::unique_ptr<Object>> Obj =
       Reader.create(!Config.SymbolsToAdd.empty());
   if (!Obj)
     return Obj.takeError();
   // Prefer OutputArch (-O<format>) if set, otherwise infer it from the input.
   const ElfType OutputElfType =
       Config.OutputArch ? getOutputElfType(Config.OutputArch.getValue())
                         : getOutputElfType(In);
 
-  if (Error E = handleArgs(Config, **Obj, Reader, OutputElfType))
+  if (Error E = handleArgs(Config, **Obj))
     return createFileError(Config.InputFilename, std::move(E));
 
   if (Error E = writeOutput(Config, **Obj, Out, OutputElfType))
     return createFileError(Config.InputFilename, std::move(E));
 
   return Error::success();
 }
 
 } // end namespace elf
 } // end namespace objcopy
 } // end namespace llvm

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

static Error executeObjcopy(CopyConfig &Config) {
   sys::fs::file_status Stat;
   if (Config.InputFilename != "-") {
     if (auto EC = sys::fs::status(Config.InputFilename, Stat))
       return createFileError(Config.InputFilename, EC);
   } else {
     Stat.permissions(static_cast<sys::fs::perms>(0777));
   }
 
-  using ProcessRawFn = Error (*)(CopyConfig &, MemoryBuffer &, raw_ostream &);
-  ProcessRawFn ProcessRaw;
-  switch (Config.InputFormat) {
-  case FileFormat::Binary:
-    ProcessRaw = executeObjcopyOnRawBinary;
-    break;
-  case FileFormat::IHex:
-    ProcessRaw = executeObjcopyOnIHex;
-    break;
-  default:
-    ProcessRaw = nullptr;
-  }
+  std::function<Error(raw_ostream & OutFile)> ObjcopyFunc = nullptr;
 
-  if (ProcessRaw) {
-    auto BufOrErr = MemoryBuffer::getFileOrSTDIN(Config.InputFilename);
+  OwningBinary<llvm::object::Binary> BinaryHolder;
+  std::unique_ptr<MemoryBuffer> MemoryBufferHolder;
+
+  if (Config.InputFormat == FileFormat::Binary) {
+    // Handle FileFormat::Binary.
+    ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
+        MemoryBuffer::getFileOrSTDIN(Config.InputFilename);
     if (!BufOrErr)
       return createFileError(Config.InputFilename, BufOrErr.getError());
+    MemoryBufferHolder = std::move(*BufOrErr);
 
-    if (Error E = writeToFile(
-            Config.OutputFilename, [&](raw_ostream &OutFile) -> Error {
-              return ProcessRaw(Config, *BufOrErr->get(), OutFile);
-            }))
-      return E;
+    ObjcopyFunc = [&](raw_ostream &OutFile) -> Error {
+      return executeObjcopyOnRawBinary(Config, *MemoryBufferHolder, OutFile);
+    };
+  } else if (Config.InputFormat == FileFormat::IHex) {
+    // Handle FileFormat::IHex.
+    ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
+        MemoryBuffer::getFileOrSTDIN(Config.InputFilename);
+    if (!BufOrErr)
+      return createFileError(Config.InputFilename, BufOrErr.getError());
+    MemoryBufferHolder = std::move(*BufOrErr);
+
+    ObjcopyFunc = [&](raw_ostream &OutFile) -> Error {
+      return executeObjcopyOnIHex(Config, *MemoryBufferHolder, OutFile);
+    };
   } else {
     Expected<OwningBinary<llvm::object::Binary>> BinaryOrErr =
         createBinary(Config.InputFilename);
     if (!BinaryOrErr)
       return createFileError(Config.InputFilename, BinaryOrErr.takeError());
+    BinaryHolder = std::move(*BinaryOrErr);
 
-    if (Archive *Ar = dyn_cast<Archive>(BinaryOrErr.get().getBinary())) {
+    if (Archive *Ar = dyn_cast<Archive>(BinaryHolder.getBinary())) {
+      // Handle Archive.
       if (Error E = executeObjcopyOnArchive(Config, *Ar))
         return E;
     } else {
-      if (Error E = writeToFile(
-              Config.OutputFilename,
-              [&](raw_ostream &OutFile) -> Error {
-                return executeObjcopyOnBinary(
-                    Config, *BinaryOrErr.get().getBinary(), OutFile);
-              },
+      // Handle llvm::object::Binary.
+      ObjcopyFunc = [&](raw_ostream &OutFile) -> Error {
+        return executeObjcopyOnBinary(Config, *BinaryHolder.getBinary(),
+                                      OutFile);
+      };
+    }
+  }
+
+  if (ObjcopyFunc) {
+    if (Config.SplitDWO.empty()) {
+      if (Error E =
+              writeToFile(Config.OutputFilename, ObjcopyFunc,
+                          Config.InputFilename != "-" &&
+                              Config.InputFilename == Config.OutputFilename,
+                          Stat.getUser(), Stat.getGroup()))
+        return E;
+    } else {
+      Config.ExtractDWO = true;
+      Config.StripDWO = false;
+      if (Error E =
+              writeToFile(Config.SplitDWO, ObjcopyFunc,
+                          Config.InputFilename != "-" &&
+                              Config.InputFilename == Config.OutputFilename,
+                          Stat.getUser(), Stat.getGroup()))
+        return E;
+      Config.ExtractDWO = false;
+      Config.StripDWO = true;
+      if (Error E =
+              writeToFile(Config.OutputFilename, ObjcopyFunc,
                           Config.InputFilename != "-" &&
                               Config.InputFilename == Config.OutputFilename,
                           Stat.getUser(), Stat.getGroup()))
         return E;
     }
   }
 
   if (Error E = restoreStatOnFile(Config.OutputFilename, Stat, Config))
     return E;
 
   if (!Config.SplitDWO.empty()) {