Simplify file handling in dsymutil

[• rafael]

Summary

I am trying to unify llvm's temporary file handling around TempFile.

This is a step towards using it in dsymutil. It just moves the file handling out of DwarfLinker.cpp.

This fixes what is at least an oddity if not a bug. DwarfLinker.cpp was using ToolOutputFile, which uses RemoveFileOnSignal. The issue is that dsymutil.cpp uses that too. It is now clear from the interface that only dsymutil.cpp is responsible for creating and deleting files.

[JDevlieghere]

Thanks Rafael! I added the ToolOutputFile when implementing the threading option because I wanted to make sure files were removed if some thread were to exist dsymutil. Because we only use threads when there are multiple architectures which in turn implies we use temp files, we would indeed end up removing the file twice.

It's definitely an improvement to keep the file handling logic out the DwarfLinker. I'm not really happy with how we deal with files in dsymutil.cpp though and maybe this is the opportunity to improve it. For one, I don't like that getOutputFileName is doing so much more than just generating a file name. I also don't like that we don't remove non-temporary output files if linking fails.

Maybe we can start by splitting up the logic for temporary files (using TempFile) and output files (using ToolOutputFile). The former would contribute to your goal of unifying the use of TempFile and the latter would address my second concern. Things are a little tricky for dSYM bundles, where we ideally would want to remove the whole bundle rather than just the file. There's also the call to generateUniversalBinary which takes a file path rather than a handle.

I don't know if this is the best approach and how feasible it is, but consider this a way to kick off the discussion!

[• rafael]

r318334.

tools/dsymutil/DwarfLinker.cpp

class DwarfStreamer {
   std::unique_ptr<MCSubtargetInfo> MSTI;
   MCCodeEmitter *MCE; // Owned by MCStreamer
   MCStreamer *MS;     // Owned by AsmPrinter
   std::unique_ptr<TargetMachine> TM;
   std::unique_ptr<AsmPrinter> Asm;
   /// @}
 
   /// The file we stream the linked Dwarf to.
-  std::unique_ptr<ToolOutputFile> OutFile;
+  raw_fd_ostream &OutFile;
 
   uint32_t RangesSectionSize;
   uint32_t LocSectionSize;
   uint32_t LineSectionSize;
   uint32_t FrameSectionSize;
 
   /// Emit the pubnames or pubtypes section contribution for \p
   /// Unit into \p Sec. The data is provided in \p Names.
   void emitPubSectionForUnit(MCSection *Sec, StringRef Name,
                              const CompileUnit &Unit,
                              const std::vector<CompileUnit::AccelInfo> &Names);
 
 public:
-  /// Actually create the streamer and the ouptut file.
-  ///
-  /// This could be done directly in the constructor, but it feels
-  /// more natural to handle errors through return value.
-  bool init(Triple TheTriple, StringRef OutputFilename);
+  DwarfStreamer(raw_fd_ostream &OutFile) : OutFile(OutFile) {}
+  bool init(Triple TheTriple);
 
   /// Dump the file to the disk.
   bool finish(const DebugMap &);
 
   AsmPrinter &getAsmPrinter() const { return *Asm; }
 
   /// Set the current output section to debug_info and change
   /// the MC Dwarf version to \p DwarfVersion.
public:
   void emitFDE(uint32_t CIEOffset, uint32_t AddreSize, uint32_t Address,
                StringRef Bytes);
 
   uint32_t getFrameSectionSize() const { return FrameSectionSize; }
 };
 
 } // end anonymous namespace
 
-bool DwarfStreamer::init(Triple TheTriple, StringRef OutputFilename) {
+bool DwarfStreamer::init(Triple TheTriple) {
   std::string ErrorStr;
   std::string TripleName;
   StringRef Context = "dwarf streamer init";
 
   // Get the target.
   const Target *TheTarget =
       TargetRegistry::lookupTarget(TripleName, TheTriple, ErrorStr);
   if (!TheTarget)
bool DwarfStreamer::init(Triple TheTriple) {
   MSTI.reset(TheTarget->createMCSubtargetInfo(TripleName, "", ""));
   if (!MSTI)
     return error("no subtarget info for target " + TripleName, Context);
 
   MCE = TheTarget->createMCCodeEmitter(*MII, *MRI, *MC);
   if (!MCE)
     return error("no code emitter for target " + TripleName, Context);
 
-  // Create the output file.
-  std::error_code EC;
-  OutFile =
-      llvm::make_unique<ToolOutputFile>(OutputFilename, EC, sys::fs::F_None);
-  if (EC)
-    return error(Twine(OutputFilename) + ": " + EC.message(), Context);
-
   MCTargetOptions MCOptions = InitMCTargetOptionsFromFlags();
   MS = TheTarget->createMCObjectStreamer(
-      TheTriple, *MC, std::unique_ptr<MCAsmBackend>(MAB), OutFile->os(),
+      TheTriple, *MC, std::unique_ptr<MCAsmBackend>(MAB), OutFile,
       std::unique_ptr<MCCodeEmitter>(MCE), *MSTI, MCOptions.MCRelaxAll,
       MCOptions.MCIncrementalLinkerCompatible,
       /*DWARFMustBeAtTheEnd*/ false);
   if (!MS)
     return error("no object streamer for target " + TripleName, Context);
 
   // Finally create the AsmPrinter we'll use to emit the DIEs.
   TM.reset(TheTarget->createTargetMachine(TripleName, "", "", TargetOptions(),
bool DwarfStreamer::init(Triple TheTriple) {
   FrameSectionSize = 0;
 
   return true;
 }
 
 bool DwarfStreamer::finish(const DebugMap &DM) {
   bool Result = true;
   if (DM.getTriple().isOSDarwin() && !DM.getBinaryPath().empty())
-    Result = MachOUtils::generateDsymCompanion(DM, *MS, OutFile->os());
+    Result = MachOUtils::generateDsymCompanion(DM, *MS, OutFile);
   else
     MS->Finish();
-
-  // Declare success.
-  OutFile->keep();
-
   return Result;
 }
 
 /// Set the current output section to debug_info and change
 /// the MC Dwarf version to \p DwarfVersion.
 void DwarfStreamer::switchToDebugInfoSection(unsigned DwarfVersion) {
   MS->SwitchSection(MOFI->getDwarfInfoSection());
   MC->setDwarfVersion(DwarfVersion);

 ///
 /// The root DIEs are identified because they contain relocations that
 /// correspond to a debug map entry at specific places (the low_pc for
 /// a function, the location for a variable). These relocations are
 /// called ValidRelocs in the DwarfLinker and are gathered as a very
 /// first step when we start processing a DebugMapObject.
 class DwarfLinker {
 public:
-  DwarfLinker(StringRef OutputFilename, const LinkOptions &Options)
-      : OutputFilename(OutputFilename), Options(Options),
-        BinHolder(Options.Verbose) {}
+  DwarfLinker(raw_fd_ostream &OutFile, const LinkOptions &Options)
+      : OutFile(OutFile), Options(Options), BinHolder(Options.Verbose) {}
 
   /// Link the contents of the DebugMap.
   bool link(const DebugMap &);
 
   void reportWarning(const Twine &Warning,
                      const DWARFDie *DIE = nullptr) const;
 
 private:
private:
   /// @}
 
   /// ODR Contexts for that link.
   DeclContextTree ODRContexts;
 
   /// \defgroup Helpers Various helper methods.
   ///
   /// @{
-  bool createStreamer(const Triple &TheTriple, StringRef OutputFilename);
+  bool createStreamer(const Triple &TheTriple, raw_fd_ostream &OutFile);
 
   /// Attempt to load a debug object from disk.
   ErrorOr<const object::ObjectFile &> loadObject(BinaryHolder &BinaryHolder,
                                                  DebugMapObject &Obj,
                                                  const DebugMap &Map);
   /// @}
 
-  std::string OutputFilename;
+  raw_fd_ostream &OutFile;
   LinkOptions Options;
   BinaryHolder BinHolder;
   std::unique_ptr<DwarfStreamer> Streamer;
   uint64_t OutputDebugInfoSize;
 
   /// A unique ID that identifies each compile unit.
   unsigned UnitID;
 
void DwarfLinker::reportWarning(const Twine &Warning,
   DumpOpts.RecurseDepth = 0;
   DumpOpts.Verbose = Options.Verbose;
 
   errs() << "    in DIE:\n";
   DIE->dump(errs(), 6 /* Indent */, DumpOpts);
 }
 
 bool DwarfLinker::createStreamer(const Triple &TheTriple,
-                                 StringRef OutputFilename) {
+                                 raw_fd_ostream &OutFile) {
   if (Options.NoOutput)
     return true;
 
-  Streamer = llvm::make_unique<DwarfStreamer>();
-  return Streamer->init(TheTriple, OutputFilename);
+  Streamer = llvm::make_unique<DwarfStreamer>(OutFile);
+  return Streamer->init(TheTriple);
 }
 
 /// Recursive helper to build the global DeclContext information and
 /// gather the child->parent relationships in the original compile unit.
 ///
 /// \return true when this DIE and all of its children are only
 /// forward declarations to types defined in external clang modules
 /// (i.e., forward declarations that are children of a DW_TAG_module).
for (auto &CurrentUnit : CompileUnits) {
     Linker.Streamer->emitCompileUnitHeader(*CurrentUnit);
     if (!CurrentUnit->getOutputUnitDIE())
       continue;
     Linker.Streamer->emitDIE(*CurrentUnit->getOutputUnitDIE());
   }
 }
 
 bool DwarfLinker::link(const DebugMap &Map) {
-  if (!createStreamer(Map.getTriple(), OutputFilename))
+  if (!createStreamer(Map.getTriple(), OutFile))
     return false;
 
   // Size of the DIEs (and headers) generated for the linked output.
   OutputDebugInfoSize = 0;
   // A unique ID that identifies each compile unit.
   UnitID = 0;
   DebugMap ModuleMap(Map.getTriple(), Map.getBinaryPath());
 

 }
 
 bool error(const Twine &Error, const Twine &Context) {
   errs() << Twine("while processing ") + Context + ":\n";
   errs() << Twine("error: ") + Error + "\n";
   return false;
 }
 
-bool linkDwarf(StringRef OutputFilename, const DebugMap &DM,
+bool linkDwarf(raw_fd_ostream &OutFile, const DebugMap &DM,
                const LinkOptions &Options) {
-  DwarfLinker Linker(OutputFilename, Options);
+  DwarfLinker Linker(OutFile, Options);
   return Linker.link(DM);
 }
 
 } // end namespace dsymutil
 } // end namespace llvm

tools/dsymutil/dsymutil.h

 
 /// \brief Dump the symbol table
 bool dumpStab(StringRef InputFile, ArrayRef<std::string> Archs,
               StringRef PrependPath = "");
 
 /// \brief Link the Dwarf debuginfo as directed by the passed DebugMap
 /// \p DM into a DwarfFile named \p OutputFilename.
 /// \returns false if the link failed.
-bool linkDwarf(StringRef OutputFilename, const DebugMap &DM,
+bool linkDwarf(raw_fd_ostream &OutFile, const DebugMap &DM,
                const LinkOptions &Options);
 
 /// \brief Exit the dsymutil process, cleaning up every temporary
 /// files that we created.
 LLVM_ATTRIBUTE_NORETURN void exitDsymutil(int ExitStatus);
 
 void warn(const Twine &Warning, const Twine &Context);
 bool error(const Twine &Error, const Twine &Context);
 
 } // end namespace dsymutil
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_DSYMUTIL_DSYMUTIL_H

tools/dsymutil/dsymutil.cpp

 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/thread.h"
 #include <algorithm>
 #include <cstdint>
 #include <cstdlib>
 #include <string>
 #include <system_error>
 
+using namespace llvm;
 using namespace llvm::cl;
 using namespace llvm::dsymutil;
 
 static OptionCategory DsymCategory("Specific Options");
 static opt<bool> Help("h", desc("Alias for -help"), Hidden);
 static opt<bool> Version("v", desc("Alias for -version"), Hidden);
 
 static list<std::string> InputFiles(Positional, OneOrMore,
for (auto &Map : *DebugMapPtrsOrErr) {
       if (Map->begin() == Map->end())
         llvm::errs() << "warning: no debug symbols in executable (-arch "
                      << MachOUtils::getArchName(Map->getTriple().getArchName())
                      << ")\n";
 
       std::string OutputFile = getOutputFileName(InputFile, NeedsTempFiles);
 
       auto LinkLambda = [OutputFile, Options, &Map]() {
-        if (OutputFile.empty() || !linkDwarf(OutputFile, *Map, Options))
+        if (OutputFile.empty())
+          exitDsymutil(1);
+        std::error_code EC;
+        raw_fd_ostream OS(NoOutput ? "-" : OutputFile, EC, sys::fs::F_None);
+        if (EC) {
+          errs() << OutputFile << ": " << EC.message();
+          exitDsymutil(1);
+        }
+        if (!linkDwarf(OS, *Map, Options))
           exitDsymutil(1);
       };
 
       // FIXME: The DwarfLinker can have some very deep recursion that can max
       // out the (significantly smaller) stack when using threads. We don't
       // want this limitation when we only have a single thread.
       if (NumThreads == 1) {
         LinkLambda();