Page MenuHomePhabricator

[Driver] Clean up tmp files when deleting Compilation objects
ClosedPublic

Authored by dstenb on Apr 16 2018, 7:16 AM.

Details

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.

Diff Detail

Event Timeline

dstenb created this revision.Apr 16 2018, 7:16 AM

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.

Ping.

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

Ka-Ka added a subscriber: Ka-Ka.May 23 2018, 4:36 AM
lebedev.ri added a subscriber: 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 added inline comments.
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 added inline comments.May 23 2018, 7:32 AM
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 added inline comments.May 23 2018, 7:34 AM
lib/Tooling/CompilationDatabase.cpp
303 ↗(On Diff #142628)

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

zturner edited reviewers, added: alexfh; removed: zturner.May 23 2018, 8:08 AM
dstenb updated this revision to Diff 148577.May 25 2018, 3:45 AM
dstenb retitled this revision from [Tooling] Clean up tmp files when creating a fixed compilation database to [Driver] Clean up tmp files when deleting Compilation objects.
dstenb edited the summary of this revision. (Show Details)

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 added inline comments.May 25 2018, 3:48 AM
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 updated this revision to Diff 148592.May 25 2018, 6:02 AM
dstenb marked an inline comment as done.

Renamed SaveTempsEnabled field to KeepTempFiles.

erichkeane added inline comments.May 25 2018, 6:13 AM
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 added inline comments.May 25 2018, 6:26 AM
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.

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 updated this revision to Diff 148604.May 25 2018, 7:25 AM

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

dstenb marked an inline comment as done.May 25 2018, 7:33 AM

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

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.

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.

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 added a comment.May 26 2018, 1:08 PM

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 updated this revision to Diff 148791.May 28 2018, 1:32 AM

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

JDevlieghere accepted this revision.May 29 2018, 5:06 AM

Thanks David, LGTM

This revision is now accepted and ready to land.May 29 2018, 5:06 AM

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

aaron.ballman accepted this revision.May 30 2018, 4:51 AM

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.

dstenb closed this revision.May 31 2018, 2:09 AM