Commit ad7aaa47 authored by Duncan P. N. Exon Smith's avatar Duncan P. N. Exon Smith Committed by Duncan P. N. Exon Smith
Browse files

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

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 `createOutputFile`s 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`.

Differential Revision: https://reviews.llvm.org/D93248
parent 4d28f0a6

clang/include/clang/Frontend/CompilerInstance.h

+18 −28
Original line number Diff line number Diff line
@@ -578,11 +578,6 @@ public: 
  /// @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.

  ///
@@ -695,25 +690,29 @@ public: 
  /// 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
@@ -722,10 +721,6 @@ public: 
  /// 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
@@ -734,17 +729,12 @@ public: 
  /// 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();



  /// }

clang/lib/Frontend/CompilerInstance.cpp

+51 −62
Original line number Diff line number Diff line
@@ -646,10 +646,6 @@ void CompilerInstance::createSema(TranslationUnitKind TUKind, 


// 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()) {
@@ -682,10 +678,25 @@ void CompilerInstance::clearOutputFiles(bool EraseFiles) { 


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() {
@@ -694,64 +705,40 @@ std::unique_ptr<raw_pwrite_stream> CompilerInstance::createNullOutputFile() { 


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();

  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();

  return nullptr;

}



  // 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'
@@ -761,14 +748,15 @@ std::unique_ptr<llvm::raw_pwrite_stream> CompilerInstance::createOutputFile( 
    }

  }



  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";
@@ -795,22 +783,23 @@ std::unique_ptr<llvm::raw_pwrite_stream> CompilerInstance::createOutputFile( 
  }



  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);

clang/lib/Frontend/FrontendActions.cpp

+7 −14
Original line number Diff line number Diff line
@@ -136,13 +136,9 @@ bool GeneratePCHAction::ComputeASTConsumerArguments(CompilerInstance &CI, 
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;
@@ -219,12 +215,9 @@ GenerateModuleFromModuleMapAction::CreateOutputFile(CompilerInstance &CI, 
                                   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);

}

clang/tools/driver/cc1_main.cpp

+3 −7
Original line number Diff line number Diff line
@@ -248,13 +248,9 @@ int cc1_main(ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr) { 
  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",

    if (auto profilerOutput = Clang->createOutputFile(

            Path.str(), /*Binary=*/false, /*RemoveFileOnSignal=*/false,

            /*useTemporary=*/false)) {



      llvm::timeTraceProfilerWrite(*profilerOutput);

      // FIXME(ibiryukov): make profilerOutput flush in destructor instead.

      profilerOutput->flush();