[Driver] Clean up tmp files when deleting Compilation objects

[dstenb]

Summary

In rL327851 the createUniqueFile() and createTemporaryFile()
variants that do not return the file descriptors were changed to
create empty files, rather than only check if the paths are free.
This change was done in order to make the functions race-free.

That change led to clang-tidy (and possibly other tools) leaving
behind temporary assembly files, of the form placeholder-*, when
using a target that does not support the internal assembler.

The temporary files are created when building the Compilation
object in stripPositionalArgs(), as a part of creating the
compilation database for the arguments after the double-dash. The
files are created by Driver::GetNamedOutputPath().

Fix this issue by cleaning out temporary files at the deletion of
Compilation objects.

This fixes https://bugs.llvm.org/show_bug.cgi?id=37091.

[dstenb]

Ping. It feels a bit nasty that the tools leave behind temporary files, so I think that it would be good to find a fix for that.

[dstenb]

Ping.

We have added a lit reproducer for this now in clang-tools-extra: https://reviews.llvm.org/D47251.

[lebedev.ri]

The testcase sounds reasonable. Can't comment on the actual fix.
Adding some more reviewers that potentially worked on that source file.

[erichkeane]

lib/Tooling/CompilationDatabase.cpp

303 ↗

(On Diff #142628)

It seems to me that this would be best called from the destructor of the Compilation object. I realize nothing returns before this, but IMO this seems universal enough that if it doesn't break anything, putting this in the destructor would be a proper place for cleanup.

[dstenb]

lib/Tooling/CompilationDatabase.cpp

303 ↗

(On Diff #142628)

That sounds cleaner (if it's viable).

I considered that initially, but I was not sure that there are no cases where the temporary files outlive a Compilation object, and the "/// Temporary files which should be removed on exit." comment for the TempFiles field did not really make me more confident, so I went the conservative route instead.

I'll do that change and run some tests to see where that leads.

[lebedev.ri]

lib/Tooling/CompilationDatabase.cpp

303 ↗

(On Diff #142628)

It just looks off-place for the function named stripPositionalArgs()

[dstenb]

I have now updated the patch so that the files are removed when deleting Compilation objects.

Expect for the fact that we now also remove files created in stripPositionalArgs, the intention is that the removal behavior remains as before.

[lebedev.ri]

include/clang/Driver/Compilation.h

126

KeepTempFiles?

lib/Driver/Compilation.cpp

275–276

Is there a test that breaks without this?

[dstenb]

Renamed SaveTempsEnabled field to KeepTempFiles.

[erichkeane]

lib/Driver/Compilation.cpp

42

I'd prefer to just look this up from 'TheDriver' when it is needed. We shouldn't store things we can look up easy enough.

[dstenb]

lib/Driver/Compilation.cpp

275–276

Yes, the following tests fail:

• Driver/crash-report-modules.m
• Driver/crash-report-spaces.c
• Driver/crash-report-header.h
• Driver/crash-report.c
• Modules/crash-vfs-path-emptydir-entries.m
• Modules/crash-vfs-path-symlink-topheader.m
• Modules/crash-vfs-path-symlink-component.m
• Modules/crash-vfs-path-traversal.m
• Modules/crash-vfs-relative-overlay.m
• Modules/crash-vfs-umbrella-frameworks.m

due to the preprocessed files for the crash diagnostics having been removed.

[lebedev.ri]

LGTM, but i'm quite unfamiliar with this area of the code,
so please wait for someone else to accept :)

lib/Driver/Compilation.cpp

275–276

Ok, good.

[dstenb]

Query TheDriver.isSaveTempsEnabled() at uses instead of storing the value in the constructor.

[Ka-Ka]

Added Jonas Devlieghere as reviewer as he was involved in the review of rL327851 (which seems to be the reason for this patch).

[aaron.ballman]

In D45686#1109295, @dstenb wrote:

Ping.

We have added a lit reproducer for this now in clang-tools-extra: https://reviews.llvm.org/D47251.

The above should be rolled into this patch as the test case verifying the behavioral changes.

[Ka-Ka]

In D45686#1113414, @aaron.ballman wrote:

In D45686#1109295, @dstenb wrote:

Ping.

We have added a lit reproducer for this now in clang-tools-extra: https://reviews.llvm.org/D47251.

The above should be rolled into this patch as the test case verifying the behavioral changes.

I guess the intention from @dstenb was to submit both patches together. The reason for a separate patch with the testcase alone is that the reproducer use clang-tidy and as far as I know all clang-tidy tests exist in clang-tools-extra.

[lebedev.ri]

In D45686#1113425, @Ka-Ka wrote:

In D45686#1113414, @aaron.ballman wrote:

In D45686#1109295, @dstenb wrote:

Ping.

We have added a lit reproducer for this now in clang-tools-extra: https://reviews.llvm.org/D47251.

The above should be rolled into this patch as the test case verifying the behavioral changes.

I guess the intention from @dstenb was to submit both patches together. The reason for a separate patch with the testcase alone is that the reproducer use clang-tidy and as far as I know all clang-tidy tests exist in clang-tools-extra.

.. and both of them exist in the same svn repo, or git monorepo. (granted, not many use that)

[Ka-Ka]

In D45686#1113426, @lebedev.ri wrote:

In D45686#1113425, @Ka-Ka wrote:

In D45686#1113414, @aaron.ballman wrote:

In D45686#1109295, @dstenb wrote:

Ping.

We have added a lit reproducer for this now in clang-tools-extra: https://reviews.llvm.org/D47251.

The above should be rolled into this patch as the test case verifying the behavioral changes.

I guess the intention from @dstenb was to submit both patches together. The reason for a separate patch with the testcase alone is that the reproducer use clang-tidy and as far as I know all clang-tidy tests exist in clang-tools-extra.

.. and both of them exist in the same svn repo, or git monorepo. (granted, not many use that)

I see, I didn't know that. Well, then it make sense to merge both patches into a single one. Thanks for clarifying that.

[dstenb]

Update patch to include the clang-tools-extra test case originally added in D47251.

[JDevlieghere]

Thanks David, LGTM

[dstenb]

Any more comments or concerns, or can I land this?

[aaron.ballman]

In D45686#1115836, @dstenb wrote:

Any more comments or concerns, or can I land this?

None from me; you're good to land it. Any further comments can be handled post-commit.

include/clang/Driver/Compilation.h

class Compilation {
   std::vector<Optional<StringRef>> Redirects;
 
   /// Whether we're compiling for diagnostic purposes.
   bool ForDiagnostics = false;
 
   /// Whether an error during the parsing of the input args.
   bool ContainsError;
 
+  /// Whether to keep temporary files regardless of -save-temps.
+  bool ForceKeepTempFiles = false;
+
 public:
   Compilation(const Driver &D, const ToolChain &DefaultToolChain,
               llvm::opt::InputArgList *Args,
               llvm::opt::DerivedArgList *TranslatedArgs, bool ContainsError);
   ~Compilation();
 
   const Driver &getDriver() const { return TheDriver; }
 

lib/Driver/Compilation.cpp

 using namespace driver;
 using namespace llvm::opt;
 
 Compilation::Compilation(const Driver &D, const ToolChain &_DefaultToolChain,
                          InputArgList *_Args, DerivedArgList *_TranslatedArgs,
                          bool ContainsError)
     : TheDriver(D), DefaultToolChain(_DefaultToolChain), Args(_Args),
       TranslatedArgs(_TranslatedArgs), ContainsError(ContainsError) {
   // The offloading host toolchain is the default toolchain.
   OrderedOffloadingToolchains.insert(
       std::make_pair(Action::OFK_Host, &DefaultToolChain));
 }
 
 Compilation::~Compilation() {
+  // Remove temporary files. This must be done before arguments are freed, as
+  // the file names might be derived from the input arguments.
+  if (!TheDriver.isSaveTempsEnabled() && !ForceKeepTempFiles)
+    CleanupFileList(TempFiles);
+
   delete TranslatedArgs;
   delete Args;
 
   // Free any derived arg lists.
   for (auto Arg : TCArgs)
     if (Arg.second != TranslatedArgs)
       delete Arg.second;
 }

 void Compilation::initCompilationForDiagnostics() {
   ForDiagnostics = true;
 
   // Free actions and jobs.
   Actions.clear();
   AllActions.clear();
   Jobs.clear();
 
+  // Remove temporary files.
+  if (!TheDriver.isSaveTempsEnabled() && !ForceKeepTempFiles)
+    CleanupFileList(TempFiles);
+
   // Clear temporary/results file lists.
   TempFiles.clear();
   ResultFiles.clear();
   FailureResultFiles.clear();
 
   // Remove any user specified output.  Claim any unclaimed arguments, so as
   // to avoid emitting warnings about unused args.
   OptSpecifier OutputOpts[] = { options::OPT_o, options::OPT_MD,
                                 options::OPT_MMD };
   for (unsigned i = 0, e = llvm::array_lengthof(OutputOpts); i != e; ++i) {
     if (TranslatedArgs->hasArg(OutputOpts[i]))
       TranslatedArgs->eraseArg(OutputOpts[i]);
   }
   TranslatedArgs->ClaimAllArgs();
 
   // Redirect stdout/stderr to /dev/null.
   Redirects = {None, {""}, {""}};
+
+  // Temporary files added by diagnostics should be kept.
+  ForceKeepTempFiles = true;
 }
 
 StringRef Compilation::getSysRoot() const {
   return getDriver().SysRoot;
 }
 
 void Compilation::Redirect(ArrayRef<Optional<StringRef>> Redirects) {
   this->Redirects = Redirects;
 }

lib/Driver/Driver.cpp

void Driver::generateCompilationDiagnostics(
   }
 
   // Generate preprocessed output.
   SmallVector<std::pair<int, const Command *>, 4> FailingCommands;
   C.ExecuteJobs(C.getJobs(), FailingCommands);
 
   // If any of the preprocessing commands failed, clean up and exit.
   if (!FailingCommands.empty()) {
-    if (!isSaveTempsEnabled())
-      C.CleanupFileList(C.getTempFiles(), true);
-
     Diag(clang::diag::note_drv_command_failed_diag_msg)
         << "Error generating preprocessed source(s).";
     return;
   }
 
   const ArgStringList &TempFiles = C.getTempFiles();
   if (TempFiles.empty()) {
     Diag(clang::diag::note_drv_command_failed_diag_msg)
if (Diags.hasErrorOccurred())
     return 1;
 
   // Set up response file names for each command, if necessary
   for (auto &Job : C.getJobs())
     setUpResponseFiles(C, Job);
 
   C.ExecuteJobs(C.getJobs(), FailingCommands);
 
-  // Remove temp files.
-  C.CleanupFileList(C.getTempFiles());
-
   // If the command succeeded, we are done.
   if (FailingCommands.empty())
     return 0;
 
   // Otherwise, remove result files and print extra information about abnormal
   // failures.
   for (const auto &CmdPair : FailingCommands) {
     int Res = CmdPair.first;