[flang] Refine output file generation

[awarzynski]

Summary

This patch cleans-up the file generation code in Flang's frontend driver. It improves the layering between CompilerInstance::CreateDefaultOutputFile, CompilerInstance::CreateOutputFile and their various clients.

• Rename CreateOutputFile as CreateOutputFileImpl and make it private. This method is an implementation detail.
• Instead of passing an std::error_code out parameter into CreateOutputFileImpl, have it return Expected<>. This is a bit shorter and idiomatic LLVM.
• Make CreateDefaultOutputFile (which calls CreateOutputFileImpl) issue an error when file creation fails. The error code from CreateOutputFileImpl is used to generate a meaningful diagnostic message.
• Remove error reporting from PrintPreprocessedAction::ExecuteAction. This is only for cases when output file generation fails. This is handled in CreateDefaultOutputFile instead (see the previous point).
• Inline AddOutputFile into its only caller, CreateDefaultOutputFile.
• Switch from lvm::buffer_ostream to llvm::buffer_unique_ostream> for non-seekable output streams. This simplifies the logic in the driver and was introduced for this very reason in [1]
• Moke sure that the diagnostics from the prescanner when running -E (PrintPreprocessedAction::ExecuteAction) are printed before the actual output is generated.
• Update comments, add test.

[1] https://reviews.llvm.org/D93260

[ashermancinelli]

lgtm!

[awarzynski]

I reverted this as our Windows buildbot was failing [1]. I'm not sure what went wrong, I used %errc_ENOENT in output-paths.f90, so this shouldn't really happen:

C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\build\tools\flang\test\Driver\Output\output-paths.f90.tmp:1:1: note: with "MSG" equal to "No such file or directory"
error: unable to open output file 'C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\build\tools\flang\test\Driver\Output\output-paths.f90.tmp.doesnotexist/somename': 'no such file or directory'

Basically, it looks that %errc_ENOENT is expanded to "No such file or directory" rather than "no such file or directory".

@Meinersbur , would you be able to take a look? Thank you!

[1] https://lab.llvm.org/buildbot/#/builders/172/builds/2077

[DavidSpickett]

Something I found on windows on Arm is that cmake was trying to run every try_compile or try_run in debug mode but we didn't have the debug dlls on the path because we were doing a release llvm build. llvm uses a try_run to work out what those generic file errors should be, and it always fails for that reason and so uses the default unix style errors.

Might be worth looking at. You can set -DCMAKE_TRY_COMPILE_CONFIGURATION=Release if that is the case. I found a similar cmake issue https://gitlab.kitware.com/cmake/cmake/-/issues/16553 but haven't looked into it further yet.

See llvm/cmake/modules/GetErrcMessages.cmake

[Meinersbur]

I logged into the worker to see the value it is using:

config.errc_messages = "no such file or directory;is a directory;invalid argument;permission denied"

However, this is LLVM's lit.site.cfg.py, flang does not have an equivalent errc_messages. This causes lit (llvm-project\llvm\utils\lit\lit\llvm\config.py) to fall back to Python's error messages:

# Python's strerror may not supply the same message
# as C++ std::error_code. One example of such a platform is
# Visual Studio. errc_messages may be supplied which contains the error
# messages for ENOENT, EISDIR, EINVAL and EACCES as a semi colon
# separated string. LLVM testsuites can use get_errc_messages in cmake
# to automatically get the messages and pass them into lit.
errc_messages = getattr(self.config, 'errc_messages', '')
if len(errc_messages) != 0:
    (errc_enoent, errc_eisdir,
     errc_einval, errc_eacces) = errc_messages.split(';')
    self.config.substitutions.append(
        ('%errc_ENOENT', '\'' + errc_enoent + '\''))
     ...
else:
    self.config.substitutions.append(
        ('%errc_ENOENT', '\'' + os.strerror(errno.ENOENT) + '\''))
     ...

That is, I suggest to add the following line

config.errc_messages = "@LLVM_LIT_ERRC_MESSAGES@"

into flang's lit.site.cfg.py.in to fix this. Also ensure that LLVM_LIT_ERRC_MESSAGES is populated in standalone builds, like clang does.

[Meinersbur]

@DavidSpickett: Look like the appropriate ucrtbased.dll (debug version of ucrtbase.dll) is not installed into system folders. Also see https://stackoverflow.com/questions/33743493/why-visual-studio-2015-cant-run-exe-file-ucrtbased-dll. The try_run calls should not need paths to any LLVM dll.

[awarzynski]

Thank you @Meinersbur , this is incredibly helpful and much appreciated! I really had no clue where to look in this case. I'll update this patch in a few minutes.

Also ensure that LLVM_LIT_ERRC_MESSAGES is populated in standalone builds, like clang does.

I think that we are missing some logic surrounding test configuration for standalone builds, so I'm not sure _where_ it should go. Could you take a look?

[awarzynski]

Add config.errc_messages="@LLVM_LIT_ERRC_MESSAGES@"

[Meinersbur]

In D108390#2958232, @awarzynski wrote:

Also ensure that LLVM_LIT_ERRC_MESSAGES is populated in standalone builds, like clang does.

I think that we are missing some logic surrounding test configuration for standalone builds, so I'm not sure _where_ it should go. Could you take a look?

From clang's CMakeLists.txt:

if( CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR )
  ...
  include(GetErrcMessages)
  ...
  get_errc_messages(LLVM_LIT_ERRC_MESSAGES)

[awarzynski]

Add missing include, fix clang-tidy warning

[awarzynski]

The pre-commit CI is now passing and so I will merge this shortly (I will be on a break for 2 weeks and would really prefer for this to be complete before I leave). Thank you all for helping with this!

[yaron.keren]

The x64 windows pre-check bot is failing on this test, see:

failed build

---

Failed Tests (1):

Flang :: Driver/output-paths.f90

[awarzynski]

In D108390#2967728, @yaron.keren wrote:

The x64 windows pre-check bot is failing on this test, see:

failed build

Hi @yaron.keren , thank you for pointing this out. This test was failing in the original version of the patch, which then got reverted, updated/fixed and re-committed. I see that you have already merged your patch, so I guess that all is good? If there are any outstanding issues, I will try to address them ASAP. Sadly, I can't check the buildbots to verify this as they are offline ATM.

Oh, and apologies for the delay, I was AFK for a couple of weeks.

[yaron.keren]

Yes the test had passed since then, so all OK.

flang/include/flang/Frontend/CompilerInstance.h

class CompilerInstance {
 
   /// Holds information about the output file.
   struct OutputFile {
     std::string filename_;
     OutputFile(std::string inputFilename)
         : filename_(std::move(inputFilename)) {}
   };
 
-  /// Output stream that doesn't support seeking (e.g. terminal, pipe).
-  /// This stream is normally wrapped in buffer_ostream before being passed
-  /// to users (e.g. via CreateOutputFile).
-  std::unique_ptr<llvm::raw_fd_ostream> nonSeekStream_;
-
   /// The list of active output files.
   std::list<OutputFile> outputFiles_;
 
   /// Holds the output stream provided by the user. Normally, users of
   /// CompilerInstance will call CreateOutputFile to obtain/create an output
   /// stream. If they want to provide their own output stream, this field will
   /// facilitate this. It is optional and will normally be just a nullptr.
   std::unique_ptr<llvm::raw_pwrite_stream> outputStream_;
assert(diagnostics_ && diagnostics_->getClient() &&
         "Compiler instance has no diagnostic client!");
     return *diagnostics_->getClient();
   }
 
   /// {
   /// @name Output Files
   /// {
 
-  /// Add an output file onto the list of tracked output files.
-  ///
-  /// \param outFile - The output file info.
-  void AddOutputFile(OutputFile &&outFile);
-
   /// Clear the output file list.
   void ClearOutputFiles(bool eraseFiles);
 
   /// Create the default output file (based on the invocation's options) and
   /// add it to the list of tracked output files. If the name of the output
-  /// file is not provided, it is derived from the input file.
+  /// file is not provided, it will be derived from the input file.
   ///
   /// \param binary     The mode to open the file in.
   /// \param baseInput  If the invocation contains no output file name (i.e.
   ///                   outputFile in FrontendOptions is empty), the input path
   ///                   name to use for deriving the output path.
   /// \param extension  The extension to use for output names derived from
   ///                   \p baseInput.
-  /// \return           ostream for the output file or nullptr on error.
+  /// \return           Null on error, ostream for the output file otherwise
   std::unique_ptr<llvm::raw_pwrite_stream> CreateDefaultOutputFile(
       bool binary = true, llvm::StringRef baseInput = "",
       llvm::StringRef extension = "");
 
+private:
   /// Create a new output file
   ///
   /// \param outputPath   The path to the output file.
-  /// \param error [out]  On failure, the error.
   /// \param binary       The mode to open the file in.
-  /// \return             ostream for the output file or nullptr on error.
-  std::unique_ptr<llvm::raw_pwrite_stream> CreateOutputFile(
-      llvm::StringRef outputPath, std::error_code &error, bool binary);
+  /// \return             Null on error, ostream for the output file otherwise
+  llvm::Expected<std::unique_ptr<llvm::raw_pwrite_stream>> CreateOutputFileImpl(
+      llvm::StringRef outputPath, bool binary);
 
+public:
   /// }
   /// @name Construction Utility Methods
   /// {
 
   /// Create a DiagnosticsEngine object
   ///
   /// If no diagnostic client is provided, this method creates a
   /// DiagnosticConsumer that is owned by the returned diagnostic object. If

flang/lib/Frontend/CompilerInstance.cpp

 }
 
 void CompilerInstance::set_semaOutputStream(
     std::unique_ptr<raw_ostream> Value) {
   ownedSemaOutputStream_.swap(Value);
   semaOutputStream_ = ownedSemaOutputStream_.get();
 }
 
-void CompilerInstance::AddOutputFile(OutputFile &&outFile) {
-  outputFiles_.push_back(std::move(outFile));
-}
-
 // Helper method to generate the path of the output file. The following logic
 // applies:
 // 1. If the user specifies the output file via `-o`, then use that (i.e.
 //    the outputFilename parameter).
 // 2. If the user does not specify the name of the output file, derive it from
 //    the input file (i.e. inputFilename + extension)
 // 3. If the output file is not specified and the input file is `-`, then set
 //    the output file to `-` as well.
static std::string GetOutputFilePath(llvm::StringRef outputFilename,
   }
 
   return outFile;
 }
 
 std::unique_ptr<llvm::raw_pwrite_stream>
 CompilerInstance::CreateDefaultOutputFile(
     bool binary, llvm::StringRef baseName, llvm::StringRef extension) {
-  std::string outputPathName;
-  std::error_code ec;
 
   // Get the path of the output file
   std::string outputFilePath =
       GetOutputFilePath(frontendOpts().outputFile, baseName, extension);
 
   // Create the output file
-  std::unique_ptr<llvm::raw_pwrite_stream> os =
-      CreateOutputFile(outputFilePath, ec, binary);
+  llvm::Expected<std::unique_ptr<llvm::raw_pwrite_stream>> os =
+      CreateOutputFileImpl(outputFilePath, binary);
 
-  // Add the file to the list of tracked output files (provided it was created
-  // successfully)
-  if (os)
-    AddOutputFile(OutputFile(outputPathName));
+  // If successful, add the file to the list of tracked output files and
+  // return.
+  if (os) {
+    outputFiles_.emplace_back(OutputFile(outputFilePath));
+    return std::move(*os);
+  }
 
-  return os;
+  // If unsuccessful, issue an error and return Null
+  unsigned DiagID = diagnostics().getCustomDiagID(
+      clang::DiagnosticsEngine::Error, "unable to open output file '%0': '%1'");
+  diagnostics().Report(DiagID)
+      << outputFilePath << llvm::errorToErrorCode(os.takeError()).message();
+  return nullptr;
 }
 
-std::unique_ptr<llvm::raw_pwrite_stream> CompilerInstance::CreateOutputFile(
-    llvm::StringRef outputFilePath, std::error_code &error, bool binary) {
+llvm::Expected<std::unique_ptr<llvm::raw_pwrite_stream>>
+CompilerInstance::CreateOutputFileImpl(
+    llvm::StringRef outputFilePath, bool binary) {
 
   // Creates the file descriptor for the output file
   std::unique_ptr<llvm::raw_fd_ostream> os;
-  std::string osFile;
-  if (!os) {
-    osFile = outputFilePath;
-    os.reset(new llvm::raw_fd_ostream(osFile, error,
-        (binary ? llvm::sys::fs::OF_None : llvm::sys::fs::OF_TextWithCRLF)));
-    if (error)
-      return nullptr;
-  }
+
+  std::error_code error;
+  os.reset(new llvm::raw_fd_ostream(outputFilePath, error,
+    return llvm::errorCodeToError(error);
 
-  // Return the stream corresponding to the output file.
-  // For non-seekable streams, wrap it in llvm::buffer_ostream first.
+  // For seekable streams, just return the stream corresponding to the output
+  // file.
   if (!binary || os->supportsSeeking())
     return std::move(os);
 
-  assert(!nonSeekStream_ && "The non-seek stream has already been set!");
-  auto b = std::make_unique<llvm::buffer_ostream>(*os);
-  nonSeekStream_ = std::move(os);
-  return std::move(b);
+  // For non-seekable streams, we need to wrap the output stream into something
+  // that supports 'pwrite' and takes care of the ownership for us.
+  return std::make_unique<llvm::buffer_unique_ostream>(std::move(os));
 }
 
 void CompilerInstance::ClearOutputFiles(bool eraseFiles) {
   for (OutputFile &of : outputFiles_)
     if (!of.filename_.empty() && eraseFiles)
       llvm::sys::fs::remove(of.filename_);
 
   outputFiles_.clear();
-  nonSeekStream_.reset();
 }
 
 bool CompilerInstance::ExecuteAction(FrontendAction &act) {
   auto &invoc = this->invocation();
 
   // Set some sane defaults for the frontend.
   invoc.SetDefaultFortranOpts();
   // Update the fortran options based on user-based input.

flang/lib/Frontend/FrontendActions.cpp

void PrintPreprocessedAction::ExecuteAction() {
   CompilerInstance &ci = this->instance();
   if (ci.invocation().preprocessorOpts().noReformat) {
     ci.parsing().DumpCookedChars(outForPP);
   } else {
     ci.parsing().EmitPreprocessedSource(
         outForPP, !ci.invocation().preprocessorOpts().noLineDirectives);
   }
 
+  // Print diagnostics from the prescanner
+  ci.parsing().messages().Emit(llvm::errs(), ci.allCookedSources());
+
   // If a pre-defined output stream exists, dump the preprocessed content there
   if (!ci.IsOutputStreamNull()) {
     // Send the output to the pre-defined output buffer.
     ci.WriteOutputStream(outForPP.str());
     return;
   }
 
-  // Print diagnostics from the prescanner
-  ci.parsing().messages().Emit(llvm::errs(), ci.allCookedSources());
-
   // Create a file and save the preprocessed output there
-  if (auto os{ci.CreateDefaultOutputFile(
-          /*Binary=*/true, /*InFile=*/GetCurrentFileOrBufferName())}) {
-    (*os) << outForPP.str();
-  } else {
-    llvm::errs() << "Unable to create the output file\n";
+  std::unique_ptr<llvm::raw_pwrite_stream> os{ci.CreateDefaultOutputFile(
+      /*Binary=*/true, /*InFile=*/GetCurrentFileOrBufferName())};
+  if (!os) {
+    return;
   }
+
+  (*os) << outForPP.str();
 }
 
 void DebugDumpProvenanceAction::ExecuteAction() {
   this->instance().parsing().DumpProvenance(llvm::outs());
 }
 
 void ParseSyntaxOnlyAction::ExecuteAction() {
 }

flang/test/Driver/output-paths.f90

+! Test the diagnostic for cases when the output file cannot be generated
+
+!--------------------------
+! RUN lines
+!--------------------------
+! RUN: not %flang_fc1 -E -o %t.doesnotexist/somename %s 2> %t
+! RUN: FileCheck -check-prefix=OUTPUTFAIL -DMSG=%errc_ENOENT -input-file=%t %s
+
+!-----------------------
+! EXPECTED OUTPUT
+!-----------------------
+! OUTPUTFAIL: error: unable to open output file '{{.*}}doesnotexist{{.}}somename': '[[MSG]]'