[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

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

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

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 ↗

(On Diff #148577)

KeepTempFiles?

lib/Driver/Compilation.cpp

276–277 ↗

(On Diff #148577)

Is there a test that breaks without this?

[dstenb]

Renamed SaveTempsEnabled field to KeepTempFiles.

[erichkeane]

lib/Driver/Compilation.cpp

42 ↗

(On Diff #148592)

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

276–277 ↗

(On Diff #148577)

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

276–277 ↗

(On Diff #148577)

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.

lib/Tooling/CompilationDatabase.cpp

for (const auto &Cmd : Jobs) {
     // Collect only for Assemble and Compile jobs. If we do all jobs we get
     // duplicates since Link jobs point to Assemble jobs as inputs.
     if (Cmd.getSource().getKind() == driver::Action::AssembleJobClass ||
         Cmd.getSource().getKind() == driver::Action::CompileJobClass) {
       CompileAnalyzer.run(&Cmd.getSource());
     }
   }
 
+  // Remove temp files.
+  Compilation->CleanupFileList(Compilation->getTempFiles());
+
   if (CompileAnalyzer.Inputs.empty()) {
     ErrorMsg = "warning: no compile jobs found\n";
     return false;
   }
 
   // Remove all compilation input files from the command line. This is
   // necessary so that getCompileCommands() can construct a command line for
   // each file.