Frontend: Fix layering between create{,Default}OutputFile, NFC

[dexonsmith]

Summary

Fix layering between CompilerInstance::createDefaultOutputFile and the
two versions of createOutputFile.

• Add missing configuration flags to createDefaultOutputFile so that GeneratePCHAction and GenerateModuleFromModuleMapAction can use it. They previously promised that temporary files were turned on; now createDefaultOutputFile handles that logic.
• Lift the logic handling InFile and Extension to createDefaultOutputFile, since it's only the callers of that function that are using it.
• Rename the deeper of the two createOutputFiles to createOutputFileImpl and make it private to CompilerInstance (to prove that no one else is using it).
• Sink the logic for adding to CompilerInstance::OutputFiles down to createOutputFileImpl, allowing two "optional" (but always used) std::string* out parameters to be removed.
• Instead of passing a std::error_code out parameter into createOutputFileImpl, have it return Expected<>.
• As a drive-by, inline CompilerInstance::addOutputFile into its only caller, createOutputFileImpl.

Clean layering makes it easier for a future commit to extract
createOutputFileImpl out of CompilerInstance.

[dexonsmith]

Ping!

[erik.pilkington]

LGTM!

clang/include/clang/Frontend/CompilerInstance.h

llvm::Timer &getFrontendTimer() const {
     assert(FrontendTimer && "Compiler instance has no frontend timer!");
     return *FrontendTimer;
   }
 
   /// }
   /// @name Output Files
   /// {
 
-  /// addOutputFile - Add an output file onto the list of tracked output files.
-  ///
-  /// \param OutFile - The output file info.
-  void addOutputFile(OutputFile &&OutFile);
-
   /// clearOutputFiles - Clear the output file list. The underlying output
   /// streams must have been closed beforehand.
   ///
   /// \param EraseFiles - If true, attempt to erase the files from disk.
   void clearOutputFiles(bool EraseFiles);
 
   /// }
   /// @name Construction Utility Methods
void createSema(TranslationUnitKind TUKind,
                   CodeCompleteConsumer *CompletionConsumer);
 
   /// Create the frontend timer and replace any existing one with it.
   void createFrontendTimer();
 
   /// Create the default output file (from the invocation's options) and add it
   /// to the list of tracked output files.
   ///
-  /// The files created by this function always use temporary files to write to
-  /// their result (that is, the data is written to a temporary file which will
-  /// atomically replace the target output on success).
+  /// The files created by this are usually removed on signal, and, depending
+  /// on FrontendOptions, may also use a temporary file (that is, the data is
+  /// written to a temporary file which will atomically replace the target
+  /// output on success). If a client (like libclang) needs to disable
+  /// RemoveFileOnSignal, temporary files will be forced on.
   ///
   /// \return - Null on error.
   std::unique_ptr<raw_pwrite_stream>
   createDefaultOutputFile(bool Binary = true, StringRef BaseInput = "",
-                          StringRef Extension = "");
+                          StringRef Extension = "",
+                          bool RemoveFileOnSignal = true,
+                          bool CreateMissingDirectories = false);
 
-  /// Create a new output file and add it to the list of tracked output files,
-  /// optionally deriving the output path name.
+  /// Create a new output file, optionally deriving the output path name, and
+  /// add it to the list of tracked output files.
   ///
   /// \return - Null on error.
   std::unique_ptr<raw_pwrite_stream>
   createOutputFile(StringRef OutputPath, bool Binary, bool RemoveFileOnSignal,
-                   StringRef BaseInput, StringRef Extension, bool UseTemporary,
-                   bool CreateMissingDirectories = false);
+                   bool UseTemporary, bool CreateMissingDirectories = false);
 
-  /// Create a new output file, optionally deriving the output path name.
+private:
+  /// Create a new output file and add it to the list of tracked output files.
   ///
   /// If \p OutputPath is empty, then createOutputFile will derive an output
   /// path location as \p BaseInput, with any suffix removed, and \p Extension
   /// appended. If \p OutputPath is not stdout and \p UseTemporary
   /// is true, createOutputFile will create a new temporary file that must be
   /// renamed to \p OutputPath in the end.
   ///
   /// \param OutputPath - If given, the path to the output file.
-  /// \param Error [out] - On failure, the error.
-  /// \param BaseInput - If \p OutputPath is empty, the input path name to use
-  /// for deriving the output path.
-  /// \param Extension - The extension to use for derived output names.
   /// \param Binary - The mode to open the file in.
   /// \param RemoveFileOnSignal - Whether the file should be registered with
   /// llvm::sys::RemoveFileOnSignal. Note that this is not safe for
   /// multithreaded use, as the underlying signal mechanism is not reentrant
   /// \param UseTemporary - Create a new temporary file that must be renamed to
   /// OutputPath in the end.
   /// \param CreateMissingDirectories - When \p UseTemporary is true, create
   /// missing directories in the output path.
-  /// \param ResultPathName [out] - If given, the result path name will be
-  /// stored here on success.
-  /// \param TempPathName [out] - If given, the temporary file path name
-  /// will be stored here on success.
-  std::unique_ptr<raw_pwrite_stream>
-  createOutputFile(StringRef OutputPath, std::error_code &Error, bool Binary,
-                   bool RemoveFileOnSignal, StringRef BaseInput,
-                   StringRef Extension, bool UseTemporary,
-                   bool CreateMissingDirectories, std::string *ResultPathName,
-                   std::string *TempPathName);
+  Expected<std::unique_ptr<raw_pwrite_stream>>
+  createOutputFileImpl(StringRef OutputPath, bool Binary,
+                       bool RemoveFileOnSignal, bool UseTemporary,
+                       bool CreateMissingDirectories);
 
+public:
   std::unique_ptr<raw_pwrite_stream> createNullOutputFile();
 
   /// }
   /// @name Initialization Utility Methods
   /// {
 
   /// InitializeSourceManager - Initialize the source manager to set InputFile
   /// as the main file.

clang/lib/Frontend/CompilerInstance.cpp

void CompilerInstance::createSema(TranslationUnitKind TUKind,
   if (ExternalSemaSrc) {
     TheSema->addExternalSource(ExternalSemaSrc.get());
     ExternalSemaSrc->InitializeSema(*TheSema);
   }
 }
 
 // Output Files
 
-void CompilerInstance::addOutputFile(OutputFile &&OutFile) {
-  OutputFiles.push_back(std::move(OutFile));
-}
-
 void CompilerInstance::clearOutputFiles(bool EraseFiles) {
   for (OutputFile &OF : OutputFiles) {
     if (!OF.TempFilename.empty()) {
       if (EraseFiles) {
         llvm::sys::fs::remove(OF.TempFilename);
       } else {
         SmallString<128> NewOutFile(OF.Filename);
 
for (auto &Module : BuiltModules)
       llvm::sys::fs::remove(Module.second);
     BuiltModules.clear();
   }
   NonSeekStream.reset();
 }
 
 std::unique_ptr<raw_pwrite_stream>
 CompilerInstance::createDefaultOutputFile(bool Binary, StringRef InFile,
-                                          StringRef Extension) {
-  return createOutputFile(getFrontendOpts().OutputFile, Binary,
-                          /*RemoveFileOnSignal=*/true, InFile, Extension,
-                          getFrontendOpts().UseTemporary);
+                                          StringRef Extension,
+                                          bool RemoveFileOnSignal,
+                                          bool CreateMissingDirectories) {
+  StringRef OutputPath = getFrontendOpts().OutputFile;
+  Optional<SmallString<128>> PathStorage;
+  if (OutputPath.empty()) {
+    if (InFile == "-" || Extension.empty()) {
+      OutputPath = "-";
+    } else {
+      PathStorage.emplace(InFile);
+      llvm::sys::path::replace_extension(*PathStorage, Extension);
+      OutputPath = *PathStorage;
+    }
+  }
+
+  // Force a temporary file if RemoveFileOnSignal was disabled.
+  return createOutputFile(OutputPath, Binary, RemoveFileOnSignal,
+                          getFrontendOpts().UseTemporary || !RemoveFileOnSignal,
+                          CreateMissingDirectories);
 }
 
 std::unique_ptr<raw_pwrite_stream> CompilerInstance::createNullOutputFile() {
   return std::make_unique<llvm::raw_null_ostream>();
 }
 
 std::unique_ptr<raw_pwrite_stream>
 CompilerInstance::createOutputFile(StringRef OutputPath, bool Binary,
-                                   bool RemoveFileOnSignal, StringRef InFile,
-                                   StringRef Extension, bool UseTemporary,
+                                   bool RemoveFileOnSignal, bool UseTemporary,
                                    bool CreateMissingDirectories) {
-  std::string OutputPathName, TempPathName;
-  std::error_code EC;
-  std::unique_ptr<raw_pwrite_stream> OS = createOutputFile(
-      OutputPath, EC, Binary, RemoveFileOnSignal, InFile, Extension,
-      UseTemporary, CreateMissingDirectories, &OutputPathName, &TempPathName);
-  if (!OS) {
-    getDiagnostics().Report(diag::err_fe_unable_to_open_output) << OutputPath
-                                                                << EC.message();
-    return nullptr;
-  }
+  Expected<std::unique_ptr<raw_pwrite_stream>> OS =
+      createOutputFileImpl(OutputPath, Binary, RemoveFileOnSignal, UseTemporary,
+                           CreateMissingDirectories);
+  if (OS)
+    return std::move(*OS);
+  getDiagnostics().Report(diag::err_fe_unable_to_open_output)
+      << OutputPath << errorToErrorCode(OS.takeError()).message();
 
-  // Add the output file -- but don't try to remove "-", since this means we are
-  // using stdin.
-  addOutputFile(
-      OutputFile((OutputPathName != "-") ? OutputPathName : "", TempPathName));
-
-  return OS;
-}
-
-std::unique_ptr<llvm::raw_pwrite_stream> CompilerInstance::createOutputFile(
-    StringRef OutputPath, std::error_code &Error, bool Binary,
-    bool RemoveFileOnSignal, StringRef InFile, StringRef Extension,
-    bool UseTemporary, bool CreateMissingDirectories,
-    std::string *ResultPathName, std::string *TempPathName) {
+Expected<std::unique_ptr<llvm::raw_pwrite_stream>>
+CompilerInstance::createOutputFileImpl(StringRef OutputPath, bool Binary,
+                                       bool RemoveFileOnSignal,
+                                       bool UseTemporary,
+                                       bool CreateMissingDirectories) {
   assert((!CreateMissingDirectories || UseTemporary) &&
          "CreateMissingDirectories is only allowed when using temporary files");
 
-  std::string OutFile, TempFile;
-  if (!OutputPath.empty()) {
-    OutFile = std::string(OutputPath);
-  } else if (InFile == "-") {
-    OutFile = "-";
-  } else if (!Extension.empty()) {
-    SmallString<128> Path(InFile);
-    llvm::sys::path::replace_extension(Path, Extension);
-    OutFile = std::string(Path.str());
-  } else {
-    OutFile = "-";
-  }
-
   std::unique_ptr<llvm::raw_fd_ostream> OS;
-  std::string OSFile;
+  Optional<StringRef> OSFile;
 
   if (UseTemporary) {
-    if (OutFile == "-")
+    if (OutputPath == "-")
       UseTemporary = false;
     else {
       llvm::sys::fs::file_status Status;
       llvm::sys::fs::status(OutputPath, Status);
       if (llvm::sys::fs::exists(Status)) {
         // Fail early if we can't write to the final destination.
-        if (!llvm::sys::fs::can_write(OutputPath)) {
-          Error = make_error_code(llvm::errc::operation_not_permitted);
-          return nullptr;
-        }
+        if (!llvm::sys::fs::can_write(OutputPath))
+          return llvm::errorCodeToError(
+              make_error_code(llvm::errc::operation_not_permitted));
 
         // Don't use a temporary if the output is a special file. This handles
         // things like '-o /dev/null'
         if (!llvm::sys::fs::is_regular_file(Status))
           UseTemporary = false;
       }
     }
   }
 
+  std::string TempFile;
   if (UseTemporary) {
     // Create a temporary file.
     // Insert -%%%%%%%% before the extension (if any), and because some tools
     // (noticeable, clang's own GlobalModuleIndex.cpp) glob for build
     // artifacts, also append .tmp.
-    StringRef OutputExtension = llvm::sys::path::extension(OutFile);
+    StringRef OutputExtension = llvm::sys::path::extension(OutputPath);
     SmallString<128> TempPath =
-        StringRef(OutFile).drop_back(OutputExtension.size());
+        StringRef(OutputPath).drop_back(OutputExtension.size());
     TempPath += "-%%%%%%%%";
     TempPath += OutputExtension;
     TempPath += ".tmp";
     int fd;
     std::error_code EC =
         llvm::sys::fs::createUniqueFile(TempPath, fd, TempPath);
 
     if (CreateMissingDirectories &&
if (!EC) {
       OSFile = TempFile = std::string(TempPath.str());
     }
     // If we failed to create the temporary, fallback to writing to the file
     // directly. This handles the corner case where we cannot write to the
     // directory, but can write to the file.
   }
 
   if (!OS) {
-    OSFile = OutFile;
+    OSFile = OutputPath;
+    std::error_code EC;
     OS.reset(new llvm::raw_fd_ostream(
-        OSFile, Error,
+        *OSFile, EC,
         (Binary ? llvm::sys::fs::OF_None : llvm::sys::fs::OF_Text)));
-    if (Error)
-      return nullptr;
+    if (EC)
+      return llvm::errorCodeToError(EC);
   }
 
   // Make sure the out stream file gets removed if we crash.
   if (RemoveFileOnSignal)
-    llvm::sys::RemoveFileOnSignal(OSFile);
+    llvm::sys::RemoveFileOnSignal(*OSFile);
 
-  if (ResultPathName)
-    *ResultPathName = OutFile;
-  if (TempPathName)
-    *TempPathName = TempFile;
+  // Add the output file -- but don't try to remove "-", since this means we are
+  // using stdin.
+  OutputFiles.emplace_back(((OutputPath != "-") ? OutputPath : "").str(),
+                           std::move(TempFile));
 
   if (!Binary || OS->supportsSeeking())
     return std::move(OS);
 
   auto B = std::make_unique<llvm::buffer_ostream>(*OS);
   assert(!NonSeekStream);
   NonSeekStream = std::move(OS);
   return std::move(B);

clang/lib/Frontend/FrontendActions.cpp

bool GeneratePCHAction::ComputeASTConsumerArguments(CompilerInstance &CI,
   }
 
   return true;
 }
 
 std::unique_ptr<llvm::raw_pwrite_stream>
 GeneratePCHAction::CreateOutputFile(CompilerInstance &CI, StringRef InFile,
                                     std::string &OutputFile) {
-  // We use createOutputFile here because this is exposed via libclang, and we
-  // must disable the RemoveFileOnSignal behavior.
-  // We use a temporary to avoid race conditions.
-  std::unique_ptr<raw_pwrite_stream> OS =
-      CI.createOutputFile(CI.getFrontendOpts().OutputFile, /*Binary=*/true,
-                          /*RemoveFileOnSignal=*/false, InFile,
-                          /*Extension=*/"", CI.getFrontendOpts().UseTemporary);
+  // Because this is exposed via libclang we must disable RemoveFileOnSignal.
+  std::unique_ptr<raw_pwrite_stream> OS = CI.createDefaultOutputFile(
+      /*Binary=*/true, InFile, /*Extension=*/"", /*RemoveFileOnSignal=*/false);
   if (!OS)
     return nullptr;
 
   OutputFile = CI.getFrontendOpts().OutputFile;
   return OS;
 }
 
 bool GeneratePCHAction::shouldEraseOutputFiles() {
if (ModuleMapFile.empty())
       ModuleMapFile = InFile;
 
     HeaderSearch &HS = CI.getPreprocessor().getHeaderSearchInfo();
     CI.getFrontendOpts().OutputFile =
         HS.getCachedModuleFileName(CI.getLangOpts().CurrentModule,
                                    ModuleMapFile);
   }
 
-  // We use createOutputFile here because this is exposed via libclang, and we
-  // must disable the RemoveFileOnSignal behavior.
-  // We use a temporary to avoid race conditions.
-  return CI.createOutputFile(CI.getFrontendOpts().OutputFile, /*Binary=*/true,
-                             /*RemoveFileOnSignal=*/false, InFile,
-                             /*Extension=*/"", /*UseTemporary=*/true,
+  // Because this is exposed via libclang we must disable RemoveFileOnSignal.
+  return CI.createDefaultOutputFile(/*Binary=*/true, InFile, /*Extension=*/"",
+                                    /*RemoveFileOnSignal=*/false,
                                     /*CreateMissingDirectories=*/true);
 }
 
 bool GenerateModuleInterfaceAction::BeginSourceFileAction(
     CompilerInstance &CI) {
   if (!CI.getLangOpts().ModulesTS && !CI.getLangOpts().CPlusPlusModules) {
     CI.getDiagnostics().Report(diag::err_module_interface_requires_cpp_modules);
     return false;
   }

clang/tools/driver/cc1_main.cpp

int cc1_main(ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr) {
   // If any timers were active but haven't been destroyed yet, print their
   // results now.  This happens in -disable-free mode.
   llvm::TimerGroup::printAll(llvm::errs());
   llvm::TimerGroup::clearAll();
 
   if (llvm::timeTraceProfilerEnabled()) {
     SmallString<128> Path(Clang->getFrontendOpts().OutputFile);
     llvm::sys::path::replace_extension(Path, "json");
-    if (auto profilerOutput =
-            Clang->createOutputFile(Path.str(),
-                                    /*Binary=*/false,
-                                    /*RemoveFileOnSignal=*/false, "",
-                                    /*Extension=*/"json",
-                                    /*useTemporary=*/false)) {
-
+    if (auto profilerOutput = Clang->createOutputFile(
+            Path.str(), /*Binary=*/false, /*RemoveFileOnSignal=*/false,
       llvm::timeTraceProfilerWrite(*profilerOutput);
       // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
       profilerOutput->flush();
       llvm::timeTraceProfilerCleanup();
       Clang->clearOutputFiles(false);
     }
   }