Fix for Bug 50033 - -fno-temp-file is not respected when creating a pch in clang 12

[zahen]

Summary

When creating a PCH file the use of a temp file will be dictated by the presence or absence of the -fno-temp-file flag. Creating a module file will always use a temp file via the new ForceUseTemporary flag.

This design was worked out over email with @dexonsmith.

Once accepted I'll need someone to commit this change on my behalf.

[dexonsmith]

Yep, this fixes a regression (https://bugs.llvm.org/show_bug.cgi?id=50033) caused by ad7aaa475e5e632242b07380ec47d2f35d077209, where I somehow missed that modules and PCHs had different actual behaviour around -fno-temp-file despite sharing the same comments.

The code LGTM. Ideally there would be a test, but it's a bit hard to add a test for this, given that temp files aren't currently observable. Can you think of something? (Maybe it's worth adding a remark for debugging purposes when a temp file is used? With or without the filename in it? Although that could be awkward to thread through LLVM when the output backends get virtualized... What do others think?)

If no one has ideas in the next few days, I'll "accept" and commit for you. (Feel free to ping me; there's a good chance I'll lose track of this otherwise; usual ping rate is 1x/week, but happy for you to ping me sooner on this...)

[zahen]

Thanks for taking a look right away Duncan! The same concern about a lack of tests was raised when I first added -fno-temp-file and I'm afraid inspiration hasn't struck me in the meantime.

[zahen]

If no one has ideas in the next few days, I'll "accept" and commit for you. (Feel free to ping me; there's a good chance I'll lose track of this otherwise; usual ping rate is 1x/week, but happy for you to ping me sooner on this...)

@dexonsmith Ping as requested. Unfortunately I haven't come up with any way to get this tested in a cross-platform manner.

[zahen]

@dexonsmith anything else required before this can land?

[dexonsmith]

LGTM; thanks for you patience.

I've been a bit paralyzed, thinking about whether we could/should add a remark, something like -Rtemp-file or -Routput-file, which could be triggered when opening an output file. That would be a path toward testing this. On the other hand, I'm afraid maintaining the remark could restrict options in the future for output backends. For now let's just go with what you have.

[dexonsmith]

@zahen , what author information should I use for the Git commit?

[zahen]

In D101775#2791938, @dexonsmith wrote:

@zahen , what author information should I use for the Git commit?

Zachary Henkel, [e-mail] (GitHub username ZacharyHenkel)

Thanks Duncan!

[dexonsmith]

I pushed this as 05d0f1a8ea012a6b7b8ea65893ec4121106444b5 last night, but I just realized I forgot to include the link to the review :(. Closing manually.

clang/include/clang/Frontend/CompilerInstance.h

public:
   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 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.
+  /// output on success).
   ///
   /// \return - Null on error.
-  std::unique_ptr<raw_pwrite_stream>
-  createDefaultOutputFile(bool Binary = true, StringRef BaseInput = "",
-                          StringRef Extension = "",
-                          bool RemoveFileOnSignal = true,
-                          bool CreateMissingDirectories = false);
+  std::unique_ptr<raw_pwrite_stream> createDefaultOutputFile(
+      bool Binary = true, StringRef BaseInput = "", StringRef Extension = "",
+      bool RemoveFileOnSignal = true, bool CreateMissingDirectories = false,
+      bool ForceUseTemporary = false);
 
   /// 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,
                    bool UseTemporary, bool CreateMissingDirectories = false);

clang/lib/Frontend/CompilerInstance.cpp

void CompilerInstance::clearOutputFiles(bool EraseFiles) {
   OutputFiles.clear();
   if (DeleteBuiltModules) {
     for (auto &Module : BuiltModules)
       llvm::sys::fs::remove(Module.second);
     BuiltModules.clear();
   }
 }
 
-std::unique_ptr<raw_pwrite_stream>
-CompilerInstance::createDefaultOutputFile(bool Binary, StringRef InFile,
-                                          StringRef Extension,
-                                          bool RemoveFileOnSignal,
-                                          bool CreateMissingDirectories) {
+std::unique_ptr<raw_pwrite_stream> CompilerInstance::createDefaultOutputFile(
+    bool Binary, StringRef InFile, StringRef Extension, bool RemoveFileOnSignal,
+    bool CreateMissingDirectories, bool ForceUseTemporary) {
   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,
+                          getFrontendOpts().UseTemporary || ForceUseTemporary,
                           CreateMissingDirectories);
 }
 
 std::unique_ptr<raw_pwrite_stream> CompilerInstance::createNullOutputFile() {
   return std::make_unique<llvm::raw_null_ostream>();
 }
 
 std::unique_ptr<raw_pwrite_stream>

clang/lib/Frontend/FrontendActions.cpp

if (CI.getFrontendOpts().OutputFile.empty()) {
     CI.getFrontendOpts().OutputFile =
         HS.getCachedModuleFileName(CI.getLangOpts().CurrentModule,
                                    ModuleMapFile);
   }
 
   // Because this is exposed via libclang we must disable RemoveFileOnSignal.
   return CI.createDefaultOutputFile(/*Binary=*/true, InFile, /*Extension=*/"",
                                     /*RemoveFileOnSignal=*/false,
-                                    /*CreateMissingDirectories=*/true);
+                                    /*CreateMissingDirectories=*/true,
+                                    /*ForceUseTemporary=*/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;
   }