[PATCH] D44568: Fix emission of phony dependency targets when adding extra deps

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

Ping.


Repository:
  rC Clang

https://reviews.llvm.org/D44568



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44568: Fix emission of phony dependency targets when adding extra deps

[David Stenberg via Phabricator via cfe-commits]

dstenb added reviewers: bruno, vsapsai.
dstenb added subscribers: bruno, vsapsai.
dstenb added a comment.

@bruno, @vsapsai: I added you since you I saw that you recently reviewed, 
respectively delivered, https://reviews.llvm.org/D30881. That is the only 
DependencyFile commit since October; although, this feels a bit orthogonal from 
that, so feel free to remove yourselves as reviewers (and I'm sorry for adding 
you in that case)!


Repository:
  rC Clang

https://reviews.llvm.org/D44568



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47251: Add a lit reproducer for PR37091

[David Stenberg via Phabricator via cfe-commits]

dstenb created this revision.
Herald added a subscriber: cfe-commits.

This adds a lit reproducer that verifies that no temporary
assembly files are left behind when using clang-tidy with a
target that does not support the internal assembler.

The fix is in Tooling (https://reviews.llvm.org/D45686), but
as we need to verify that no files are left behind, it is
probably easier and better to create a lit reproducer for a
specific tool here instead of creating a unit test.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47251

Files:
  test/clang-tidy/pr37091.cpp


Index: test/clang-tidy/pr37091.cpp
===
--- /dev/null
+++ test/clang-tidy/pr37091.cpp
@@ -0,0 +1,10 @@
+// REQUIRES: shell
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+
+// This is a reproducer for PR37091.
+//
+// Verify that no temporary files are left behind by the clang-tidy invocation.
+
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: rmdir %t


Index: test/clang-tidy/pr37091.cpp
===
--- /dev/null
+++ test/clang-tidy/pr37091.cpp
@@ -0,0 +1,10 @@
+// REQUIRES: shell
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+
+// This is a reproducer for PR37091.
+//
+// Verify that no temporary files are left behind by the clang-tidy invocation.
+
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: rmdir %t
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45686: [Tooling] Clean up tmp files when creating a fixed compilation database

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

Ping.

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


Repository:
  rC Clang

https://reviews.llvm.org/D45686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45686: [Tooling] Clean up tmp files when creating a fixed compilation database

[David Stenberg via Phabricator via cfe-commits]

dstenb added inline comments.



Comment at: lib/Tooling/CompilationDatabase.cpp:303
+  // Remove temp files.
+  Compilation->CleanupFileList(Compilation->getTempFiles());
+

erichkeane wrote:
> 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.
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.



Repository:
  rC Clang

https://reviews.llvm.org/D45686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[David Stenberg via Phabricator via cfe-commits]

dstenb updated this revision to Diff 148577.
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.
dstenb added a comment.

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.


https://reviews.llvm.org/D45686

Files:
  include/clang/Driver/Compilation.h
  lib/Driver/Compilation.cpp
  lib/Driver/Driver.cpp


Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -1243,9 +1243,6 @@
 
   // 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;
@@ -1362,9 +1359,6 @@
 
   C.ExecuteJobs(C.getJobs(), FailingCommands);
 
-  // Remove temp files.
-  C.CleanupFileList(C.getTempFiles());
-
   // If the command succeeded, we are done.
   if (FailingCommands.empty())
 return 0;
Index: lib/Driver/Compilation.cpp
===
--- lib/Driver/Compilation.cpp
+++ lib/Driver/Compilation.cpp
@@ -38,13 +38,19 @@
  InputArgList *_Args, DerivedArgList *_TranslatedArgs,
  bool ContainsError)
 : TheDriver(D), DefaultToolChain(_DefaultToolChain), Args(_Args),
-  TranslatedArgs(_TranslatedArgs), ContainsError(ContainsError) {
+  TranslatedArgs(_TranslatedArgs), ContainsError(ContainsError),
+  SaveTempsEnabled(D.isSaveTempsEnabled()) {
   // 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 (!SaveTempsEnabled)
+CleanupFileList(TempFiles);
+
   delete TranslatedArgs;
   delete Args;
 
@@ -245,6 +251,10 @@
   AllActions.clear();
   Jobs.clear();
 
+  // Remove temporary files.
+  if (!SaveTempsEnabled)
+CleanupFileList(TempFiles);
+
   // Clear temporary/results file lists.
   TempFiles.clear();
   ResultFiles.clear();
@@ -262,6 +272,9 @@
 
   // Redirect stdout/stderr to /dev/null.
   Redirects = {None, {""}, {""}};
+
+  // Temporary files added by diagnostics should be kept.
+  SaveTempsEnabled = true;
 }
 
 StringRef Compilation::getSysRoot() const {
Index: include/clang/Driver/Compilation.h
===
--- include/clang/Driver/Compilation.h
+++ include/clang/Driver/Compilation.h
@@ -122,6 +122,9 @@
   /// Whether an error during the parsing of the input args.
   bool ContainsError;
 
+  /// Whether to save temporary files.
+  bool SaveTempsEnabled;
+
 public:
   Compilation(const Driver &D, const ToolChain &DefaultToolChain,
   llvm::opt::InputArgList *Args,


Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -1243,9 +1243,6 @@
 
   // 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;
@@ -1362,9 +1359,6 @@
 
   C.ExecuteJobs(C.getJobs(), FailingCommands);
 
-  // Remove temp files.
-  C.CleanupFileList(C.getTempFiles());
-
   // If the command succeeded, we are done.
   if (FailingCommands.empty())
 return 0;
Index: lib/Driver/Compilation.cpp
===
--- lib/Driver/Compilation.cpp
+++ lib/Driver/Compilation.cpp
@@ -38,13 +38,19 @@
  InputArgList *_Args, DerivedArgList *_TranslatedArgs,
  bool ContainsError)
 : TheDriver(D), DefaultToolChain(_DefaultToolChain), Args(_Args),
-  TranslatedArgs(_TranslatedArgs), ContainsError(ContainsError) {
+  TranslatedArgs(_TranslatedArgs), ContainsError(ContainsError),
+  SaveTempsEnabled(D.isSaveTempsEnabled()) {
   // 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 

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[David Stenberg via Phabricator via cfe-commits]

dstenb updated this revision to Diff 148592.
dstenb marked an inline comment as done.
dstenb added a comment.

Renamed SaveTempsEnabled field to KeepTempFiles.


https://reviews.llvm.org/D45686

Files:
  include/clang/Driver/Compilation.h
  lib/Driver/Compilation.cpp
  lib/Driver/Driver.cpp


Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -1243,9 +1243,6 @@
 
   // 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;
@@ -1362,9 +1359,6 @@
 
   C.ExecuteJobs(C.getJobs(), FailingCommands);
 
-  // Remove temp files.
-  C.CleanupFileList(C.getTempFiles());
-
   // If the command succeeded, we are done.
   if (FailingCommands.empty())
 return 0;
Index: lib/Driver/Compilation.cpp
===
--- lib/Driver/Compilation.cpp
+++ lib/Driver/Compilation.cpp
@@ -38,13 +38,19 @@
  InputArgList *_Args, DerivedArgList *_TranslatedArgs,
  bool ContainsError)
 : TheDriver(D), DefaultToolChain(_DefaultToolChain), Args(_Args),
-  TranslatedArgs(_TranslatedArgs), ContainsError(ContainsError) {
+  TranslatedArgs(_TranslatedArgs), ContainsError(ContainsError),
+  KeepTempFiles(D.isSaveTempsEnabled()) {
   // 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 (!KeepTempFiles)
+CleanupFileList(TempFiles);
+
   delete TranslatedArgs;
   delete Args;
 
@@ -245,6 +251,10 @@
   AllActions.clear();
   Jobs.clear();
 
+  // Remove temporary files.
+  if (!KeepTempFiles)
+CleanupFileList(TempFiles);
+
   // Clear temporary/results file lists.
   TempFiles.clear();
   ResultFiles.clear();
@@ -262,6 +272,9 @@
 
   // Redirect stdout/stderr to /dev/null.
   Redirects = {None, {""}, {""}};
+
+  // Temporary files added by diagnostics should be kept.
+  KeepTempFiles = true;
 }
 
 StringRef Compilation::getSysRoot() const {
Index: include/clang/Driver/Compilation.h
===
--- include/clang/Driver/Compilation.h
+++ include/clang/Driver/Compilation.h
@@ -122,6 +122,9 @@
   /// Whether an error during the parsing of the input args.
   bool ContainsError;
 
+  /// Whether to keep temporary files.
+  bool KeepTempFiles;
+
 public:
   Compilation(const Driver &D, const ToolChain &DefaultToolChain,
   llvm::opt::InputArgList *Args,


Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -1243,9 +1243,6 @@
 
   // 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;
@@ -1362,9 +1359,6 @@
 
   C.ExecuteJobs(C.getJobs(), FailingCommands);
 
-  // Remove temp files.
-  C.CleanupFileList(C.getTempFiles());
-
   // If the command succeeded, we are done.
   if (FailingCommands.empty())
 return 0;
Index: lib/Driver/Compilation.cpp
===
--- lib/Driver/Compilation.cpp
+++ lib/Driver/Compilation.cpp
@@ -38,13 +38,19 @@
  InputArgList *_Args, DerivedArgList *_TranslatedArgs,
  bool ContainsError)
 : TheDriver(D), DefaultToolChain(_DefaultToolChain), Args(_Args),
-  TranslatedArgs(_TranslatedArgs), ContainsError(ContainsError) {
+  TranslatedArgs(_TranslatedArgs), ContainsError(ContainsError),
+  KeepTempFiles(D.isSaveTempsEnabled()) {
   // 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 (!KeepTempFiles)
+CleanupFileList(TempFiles);
+
   delete TranslatedArgs;
   delete Args;
 
@@ -245,6 +251,10 @@
   AllActions.clear();
   Jobs.clear();
 
+  // Remove temporary files.
+  if (!KeepTempFiles)
+CleanupFileList(TempFiles);
+
   // Clear temporary/results file lists.
   TempFiles.clear();
   ResultFiles.clear();
@@ -262,6 +272,9 @@
 
   // Redirect stdo

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[David Stenberg via Phabricator via cfe-commits]

dstenb added inline comments.



Comment at: lib/Driver/Compilation.cpp:276-277
+
+  // Temporary files added by diagnostics should be kept.
+  SaveTempsEnabled = true;
 }

lebedev.ri wrote:
> Is there a test that breaks without this?
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.


https://reviews.llvm.org/D45686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[David Stenberg via Phabricator via cfe-commits]

dstenb updated this revision to Diff 148604.
dstenb added a comment.

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


https://reviews.llvm.org/D45686

Files:
  include/clang/Driver/Compilation.h
  lib/Driver/Compilation.cpp
  lib/Driver/Driver.cpp


Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -1243,9 +1243,6 @@
 
   // 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;
@@ -1362,9 +1359,6 @@
 
   C.ExecuteJobs(C.getJobs(), FailingCommands);
 
-  // Remove temp files.
-  C.CleanupFileList(C.getTempFiles());
-
   // If the command succeeded, we are done.
   if (FailingCommands.empty())
 return 0;
Index: lib/Driver/Compilation.cpp
===
--- lib/Driver/Compilation.cpp
+++ lib/Driver/Compilation.cpp
@@ -45,6 +45,11 @@
 }
 
 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;
 
@@ -245,6 +250,10 @@
   AllActions.clear();
   Jobs.clear();
 
+  // Remove temporary files.
+  if (!TheDriver.isSaveTempsEnabled() && !ForceKeepTempFiles)
+CleanupFileList(TempFiles);
+
   // Clear temporary/results file lists.
   TempFiles.clear();
   ResultFiles.clear();
@@ -262,6 +271,9 @@
 
   // Redirect stdout/stderr to /dev/null.
   Redirects = {None, {""}, {""}};
+
+  // Temporary files added by diagnostics should be kept.
+  ForceKeepTempFiles = true;
 }
 
 StringRef Compilation::getSysRoot() const {
Index: include/clang/Driver/Compilation.h
===
--- include/clang/Driver/Compilation.h
+++ include/clang/Driver/Compilation.h
@@ -122,6 +122,9 @@
   /// 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,


Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -1243,9 +1243,6 @@
 
   // 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;
@@ -1362,9 +1359,6 @@
 
   C.ExecuteJobs(C.getJobs(), FailingCommands);
 
-  // Remove temp files.
-  C.CleanupFileList(C.getTempFiles());
-
   // If the command succeeded, we are done.
   if (FailingCommands.empty())
 return 0;
Index: lib/Driver/Compilation.cpp
===
--- lib/Driver/Compilation.cpp
+++ lib/Driver/Compilation.cpp
@@ -45,6 +45,11 @@
 }
 
 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;
 
@@ -245,6 +250,10 @@
   AllActions.clear();
   Jobs.clear();
 
+  // Remove temporary files.
+  if (!TheDriver.isSaveTempsEnabled() && !ForceKeepTempFiles)
+CleanupFileList(TempFiles);
+
   // Clear temporary/results file lists.
   TempFiles.clear();
   ResultFiles.clear();
@@ -262,6 +271,9 @@
 
   // Redirect stdout/stderr to /dev/null.
   Redirects = {None, {""}, {""}};
+
+  // Temporary files added by diagnostics should be kept.
+  ForceKeepTempFiles = true;
 }
 
 StringRef Compilation::getSysRoot() const {
Index: include/clang/Driver/Compilation.h
===
--- include/clang/Driver/Compilation.h
+++ include/clang/Driver/Compilation.h
@@ -122,6 +122,9 @@
   /// 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,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[David Stenberg via Phabricator via cfe-commits]

dstenb updated this revision to Diff 148791.
dstenb added a comment.

Update patch to include the clang-tools-extra test case originally added in 
https://reviews.llvm.org/D47251.


https://reviews.llvm.org/D45686

Files:
  cfe/trunk/include/clang/Driver/Compilation.h
  cfe/trunk/lib/Driver/Compilation.cpp
  cfe/trunk/lib/Driver/Driver.cpp
  clang-tools-extra/trunk/test/clang-tidy/pr37091.cpp


Index: clang-tools-extra/trunk/test/clang-tidy/pr37091.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/pr37091.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/pr37091.cpp
@@ -0,0 +1,10 @@
+// REQUIRES: shell
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+
+// This is a reproducer for PR37091.
+//
+// Verify that no temporary files are left behind by the clang-tidy invocation.
+
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: rmdir %t
Index: cfe/trunk/lib/Driver/Driver.cpp
===
--- cfe/trunk/lib/Driver/Driver.cpp
+++ cfe/trunk/lib/Driver/Driver.cpp
@@ -1243,9 +1243,6 @@
 
   // 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;
@@ -1362,9 +1359,6 @@
 
   C.ExecuteJobs(C.getJobs(), FailingCommands);
 
-  // Remove temp files.
-  C.CleanupFileList(C.getTempFiles());
-
   // If the command succeeded, we are done.
   if (FailingCommands.empty())
 return 0;
Index: cfe/trunk/lib/Driver/Compilation.cpp
===
--- cfe/trunk/lib/Driver/Compilation.cpp
+++ cfe/trunk/lib/Driver/Compilation.cpp
@@ -45,6 +45,11 @@
 }
 
 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;
 
@@ -245,6 +250,10 @@
   AllActions.clear();
   Jobs.clear();
 
+  // Remove temporary files.
+  if (!TheDriver.isSaveTempsEnabled() && !ForceKeepTempFiles)
+CleanupFileList(TempFiles);
+
   // Clear temporary/results file lists.
   TempFiles.clear();
   ResultFiles.clear();
@@ -262,6 +271,9 @@
 
   // Redirect stdout/stderr to /dev/null.
   Redirects = {None, {""}, {""}};
+
+  // Temporary files added by diagnostics should be kept.
+  ForceKeepTempFiles = true;
 }
 
 StringRef Compilation::getSysRoot() const {
Index: cfe/trunk/include/clang/Driver/Compilation.h
===
--- cfe/trunk/include/clang/Driver/Compilation.h
+++ cfe/trunk/include/clang/Driver/Compilation.h
@@ -122,6 +122,9 @@
   /// 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,


Index: clang-tools-extra/trunk/test/clang-tidy/pr37091.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/pr37091.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/pr37091.cpp
@@ -0,0 +1,10 @@
+// REQUIRES: shell
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+
+// This is a reproducer for PR37091.
+//
+// Verify that no temporary files are left behind by the clang-tidy invocation.
+
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: rmdir %t
Index: cfe/trunk/lib/Driver/Driver.cpp
===
--- cfe/trunk/lib/Driver/Driver.cpp
+++ cfe/trunk/lib/Driver/Driver.cpp
@@ -1243,9 +1243,6 @@
 
   // 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;
@@ -1362,9 +1359,6 @@
 
   C.ExecuteJobs(C.getJobs(), FailingCommands);
 
-  // Remove temp files.
-  C.CleanupFileList(C.getTempFiles());
-
   // If the command succeeded, we are done.
   if (FailingCommands.empty())
 return 0;
Index: cfe/trunk/lib/Driver/Compilation.cpp
===
--- cfe/trunk/lib/Driver/Compilation.cpp
+++ cfe/trunk/lib/Driver/Compilation.cpp
@@ -45,6 +45,11 @@
 }
 
 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

[PATCH] D47251: Add a lit reproducer for PR37091

[David Stenberg via Phabricator via cfe-commits]

dstenb abandoned this revision.
dstenb added a comment.

Merged into https://reviews.llvm.org/D45686.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47251



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44568: Fix emission of phony dependency targets when adding extra deps

[David Stenberg via Phabricator via cfe-commits]

dstenb updated this revision to Diff 148819.
dstenb marked an inline comment as done.
dstenb added a comment.

Addressed vsapsai's comments.


https://reviews.llvm.org/D44568

Files:
  lib/Frontend/DependencyFile.cpp
  test/Frontend/dependency-gen-extradeps-phony.c


Index: test/Frontend/dependency-gen-extradeps-phony.c
===
--- /dev/null
+++ test/Frontend/dependency-gen-extradeps-phony.c
@@ -0,0 +1,10 @@
+// RUN: %clang -MM -MP -Xclang -fdepfile-entry=1.extra -Xclang 
-fdepfile-entry=2.extra -Xclang -fdepfile-entry=2.extra %s | \
+// RUN: FileCheck %s --implicit-check-not=.c:
+//
+// Verify that phony targets are only created for the extra dependency files,
+// and not the input file.
+
+// CHECK: dependency-gen-extradeps-phony.o: 1.extra 2.extra \
+// CHECK-NEXT: dependency-gen-extradeps-phony.c
+// CHECK: 1.extra:
+// CHECK: 2.extra:
Index: lib/Frontend/DependencyFile.cpp
===
--- lib/Frontend/DependencyFile.cpp
+++ lib/Frontend/DependencyFile.cpp
@@ -163,6 +163,7 @@
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
   DependencyOutputFormat OutputFormat;
+  unsigned InputFileIndex;
 
 private:
   bool FileMatchesDepCriteria(const char *Filename,
@@ -177,9 +178,11 @@
   AddMissingHeaderDeps(Opts.AddMissingHeaderDeps),
   SeenMissingHeader(false),
   IncludeModuleFiles(Opts.IncludeModuleFiles),
-  OutputFormat(Opts.OutputFormat) {
+  OutputFormat(Opts.OutputFormat),
+  InputFileIndex(0) {
 for (const auto &ExtraDep : Opts.ExtraDeps) {
-  AddFilename(ExtraDep);
+  if (AddFilename(ExtraDep))
+++InputFileIndex;
 }
   }
 
@@ -201,7 +204,7 @@
 OutputDependencyFile();
   }
 
-  void AddFilename(StringRef Filename);
+  bool AddFilename(StringRef Filename);
   bool includeSystemHeaders() const { return IncludeSystemHeaders; }
   bool includeModuleFiles() const { return IncludeModuleFiles; }
 };
@@ -325,9 +328,12 @@
   }
 }
 
-void DFGImpl::AddFilename(StringRef Filename) {
-  if (FilesSet.insert(Filename).second)
+bool DFGImpl::AddFilename(StringRef Filename) {
+  if (FilesSet.insert(Filename).second) {
 Files.push_back(Filename);
+return true;
+  }
+  return false;
 }
 
 /// Print the filename, with escaping or quoting that accommodates the three
@@ -463,8 +469,10 @@
 
   // Create phony targets if requested.
   if (PhonyTarget && !Files.empty()) {
-// Skip the first entry, this is always the input file itself.
-for (auto I = Files.begin() + 1, E = Files.end(); I != E; ++I) {
+unsigned Index = 0;
+for (auto I = Files.begin(), E = Files.end(); I != E; ++I) {
+  if (Index++ == InputFileIndex)
+continue;
   OS << '\n';
   PrintFilename(OS, *I, OutputFormat);
   OS << ":\n";


Index: test/Frontend/dependency-gen-extradeps-phony.c
===
--- /dev/null
+++ test/Frontend/dependency-gen-extradeps-phony.c
@@ -0,0 +1,10 @@
+// RUN: %clang -MM -MP -Xclang -fdepfile-entry=1.extra -Xclang -fdepfile-entry=2.extra -Xclang -fdepfile-entry=2.extra %s | \
+// RUN: FileCheck %s --implicit-check-not=.c:
+//
+// Verify that phony targets are only created for the extra dependency files,
+// and not the input file.
+
+// CHECK: dependency-gen-extradeps-phony.o: 1.extra 2.extra \
+// CHECK-NEXT: dependency-gen-extradeps-phony.c
+// CHECK: 1.extra:
+// CHECK: 2.extra:
Index: lib/Frontend/DependencyFile.cpp
===
--- lib/Frontend/DependencyFile.cpp
+++ lib/Frontend/DependencyFile.cpp
@@ -163,6 +163,7 @@
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
   DependencyOutputFormat OutputFormat;
+  unsigned InputFileIndex;
 
 private:
   bool FileMatchesDepCriteria(const char *Filename,
@@ -177,9 +178,11 @@
   AddMissingHeaderDeps(Opts.AddMissingHeaderDeps),
   SeenMissingHeader(false),
   IncludeModuleFiles(Opts.IncludeModuleFiles),
-  OutputFormat(Opts.OutputFormat) {
+  OutputFormat(Opts.OutputFormat),
+  InputFileIndex(0) {
 for (const auto &ExtraDep : Opts.ExtraDeps) {
-  AddFilename(ExtraDep);
+  if (AddFilename(ExtraDep))
+++InputFileIndex;
 }
   }
 
@@ -201,7 +204,7 @@
 OutputDependencyFile();
   }
 
-  void AddFilename(StringRef Filename);
+  bool AddFilename(StringRef Filename);
   bool includeSystemHeaders() const { return IncludeSystemHeaders; }
   bool includeModuleFiles() const { return IncludeModuleFiles; }
 };
@@ -325,9 +328,12 @@
   }
 }
 
-void DFGImpl::AddFilename(StringRef Filename) {
-  if (FilesSet.insert(Filename).second)
+bool DFGImpl::AddFilename(StringRef Filename) {
+  if (FilesSet.insert(Filename).second) {
 Files.push_back(Filename);
+return true;
+  }
+  return false;
 }
 
 /// Print the filename, with escaping or quoting that accommodates the three
@@ -463,8 +469,10 @@
 
   // Create phony targets i

[PATCH] D44568: Fix emission of phony dependency targets when adding extra deps

[David Stenberg via Phabricator via cfe-commits]

dstenb added inline comments.



Comment at: test/Frontend/dependency-gen-extradeps-phony.c:6-7
+
+// CHECK: dependency-gen-extradeps-phony.o: 1.extra 2.extra
+// CHECK-NOT: .c:
+// CHECK: 1.extra:

vsapsai wrote:
> I think it would be better to have a check
> 
> // CHECK:   dependency-gen-extradeps-phony.c
> 
> Because you expect it to be there and it's not that simple to notice the 
> colon in `.c:`, so it's not immediately clear how CHECK-NOT is applied here.
Do you mean replace the two checks with that? Isn't there a risk that that 
would match with `dependency-gen-extradeps-phony.c:`, which the not-checks 
would not pick up then?

I added a CHECK-NEXT check for the input file so that we match that whole 
dependency entry at least.



Comment at: test/Frontend/dependency-gen-extradeps-phony.c:9-11
+// CHECK-NOT: .c:
+// CHECK: 2.extra:
+// CHECK-NOT: .c:

vsapsai wrote:
> For these repeated CHECK-NOT please consider using `FileCheck 
> --implicit-check-not`. In this case it's not that important as the test is 
> small but it can still help to capture your intention more clearly.
I'll add that in the next patch (and I'll keep that in mind for future 
changes). Thanks!


https://reviews.llvm.org/D44568



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44568: Fix emission of phony dependency targets when adding extra deps

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

In https://reviews.llvm.org/D44568#1114188, @vsapsai wrote:

> Looks good to me. Just watch the build bots in case some of them are strict 
> with warnings and require `(void)AddFilename(Filename)`.


Yes, I'll keep an eye out for that.

I'll submit this shortly. Thanks for the review!


https://reviews.llvm.org/D44568



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44568: Fix emission of phony dependency targets when adding extra deps

[David Stenberg via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL333413: Fix emission of phony dependency targets when adding 
extra deps (authored by dstenb, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D44568?vs=148819&id=148891#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D44568

Files:
  cfe/trunk/lib/Frontend/DependencyFile.cpp
  cfe/trunk/test/Frontend/dependency-gen-extradeps-phony.c


Index: cfe/trunk/test/Frontend/dependency-gen-extradeps-phony.c
===
--- cfe/trunk/test/Frontend/dependency-gen-extradeps-phony.c
+++ cfe/trunk/test/Frontend/dependency-gen-extradeps-phony.c
@@ -0,0 +1,10 @@
+// RUN: %clang -MM -MP -Xclang -fdepfile-entry=1.extra -Xclang 
-fdepfile-entry=2.extra -Xclang -fdepfile-entry=2.extra %s | \
+// RUN: FileCheck %s --implicit-check-not=.c:
+//
+// Verify that phony targets are only created for the extra dependency files,
+// and not the input file.
+
+// CHECK: dependency-gen-extradeps-phony.o: 1.extra 2.extra \
+// CHECK-NEXT: dependency-gen-extradeps-phony.c
+// CHECK: 1.extra:
+// CHECK: 2.extra:
Index: cfe/trunk/lib/Frontend/DependencyFile.cpp
===
--- cfe/trunk/lib/Frontend/DependencyFile.cpp
+++ cfe/trunk/lib/Frontend/DependencyFile.cpp
@@ -163,6 +163,7 @@
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
   DependencyOutputFormat OutputFormat;
+  unsigned InputFileIndex;
 
 private:
   bool FileMatchesDepCriteria(const char *Filename,
@@ -177,9 +178,11 @@
   AddMissingHeaderDeps(Opts.AddMissingHeaderDeps),
   SeenMissingHeader(false),
   IncludeModuleFiles(Opts.IncludeModuleFiles),
-  OutputFormat(Opts.OutputFormat) {
+  OutputFormat(Opts.OutputFormat),
+  InputFileIndex(0) {
 for (const auto &ExtraDep : Opts.ExtraDeps) {
-  AddFilename(ExtraDep);
+  if (AddFilename(ExtraDep))
+++InputFileIndex;
 }
   }
 
@@ -201,7 +204,7 @@
 OutputDependencyFile();
   }
 
-  void AddFilename(StringRef Filename);
+  bool AddFilename(StringRef Filename);
   bool includeSystemHeaders() const { return IncludeSystemHeaders; }
   bool includeModuleFiles() const { return IncludeModuleFiles; }
 };
@@ -325,9 +328,12 @@
   }
 }
 
-void DFGImpl::AddFilename(StringRef Filename) {
-  if (FilesSet.insert(Filename).second)
+bool DFGImpl::AddFilename(StringRef Filename) {
+  if (FilesSet.insert(Filename).second) {
 Files.push_back(Filename);
+return true;
+  }
+  return false;
 }
 
 /// Print the filename, with escaping or quoting that accommodates the three
@@ -463,8 +469,10 @@
 
   // Create phony targets if requested.
   if (PhonyTarget && !Files.empty()) {
-// Skip the first entry, this is always the input file itself.
-for (auto I = Files.begin() + 1, E = Files.end(); I != E; ++I) {
+unsigned Index = 0;
+for (auto I = Files.begin(), E = Files.end(); I != E; ++I) {
+  if (Index++ == InputFileIndex)
+continue;
   OS << '\n';
   PrintFilename(OS, *I, OutputFormat);
   OS << ":\n";


Index: cfe/trunk/test/Frontend/dependency-gen-extradeps-phony.c
===
--- cfe/trunk/test/Frontend/dependency-gen-extradeps-phony.c
+++ cfe/trunk/test/Frontend/dependency-gen-extradeps-phony.c
@@ -0,0 +1,10 @@
+// RUN: %clang -MM -MP -Xclang -fdepfile-entry=1.extra -Xclang -fdepfile-entry=2.extra -Xclang -fdepfile-entry=2.extra %s | \
+// RUN: FileCheck %s --implicit-check-not=.c:
+//
+// Verify that phony targets are only created for the extra dependency files,
+// and not the input file.
+
+// CHECK: dependency-gen-extradeps-phony.o: 1.extra 2.extra \
+// CHECK-NEXT: dependency-gen-extradeps-phony.c
+// CHECK: 1.extra:
+// CHECK: 2.extra:
Index: cfe/trunk/lib/Frontend/DependencyFile.cpp
===
--- cfe/trunk/lib/Frontend/DependencyFile.cpp
+++ cfe/trunk/lib/Frontend/DependencyFile.cpp
@@ -163,6 +163,7 @@
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
   DependencyOutputFormat OutputFormat;
+  unsigned InputFileIndex;
 
 private:
   bool FileMatchesDepCriteria(const char *Filename,
@@ -177,9 +178,11 @@
   AddMissingHeaderDeps(Opts.AddMissingHeaderDeps),
   SeenMissingHeader(false),
   IncludeModuleFiles(Opts.IncludeModuleFiles),
-  OutputFormat(Opts.OutputFormat) {
+  OutputFormat(Opts.OutputFormat),
+  InputFileIndex(0) {
 for (const auto &ExtraDep : Opts.ExtraDeps) {
-  AddFilename(ExtraDep);
+  if (AddFilename(ExtraDep))
+++InputFileIndex;
 }
   }
 
@@ -201,7 +204,7 @@
 OutputDependencyFile();
   }
 
-  void AddFilename(StringRef Filename);
+  bool AddFilename(StringRef Filename);
   bool includeSystemHeaders() const { return IncludeSystemHeaders; }
   bool includeModuleFiles() const { return IncludeModuleFiles; }

[PATCH] D44568: Fix emission of phony dependency targets when adding extra deps

[David Stenberg via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC333413: Fix emission of phony dependency targets when adding 
extra deps (authored by dstenb, committed by ).

Repository:
  rL LLVM

https://reviews.llvm.org/D44568

Files:
  lib/Frontend/DependencyFile.cpp
  test/Frontend/dependency-gen-extradeps-phony.c


Index: test/Frontend/dependency-gen-extradeps-phony.c
===
--- test/Frontend/dependency-gen-extradeps-phony.c
+++ test/Frontend/dependency-gen-extradeps-phony.c
@@ -0,0 +1,10 @@
+// RUN: %clang -MM -MP -Xclang -fdepfile-entry=1.extra -Xclang 
-fdepfile-entry=2.extra -Xclang -fdepfile-entry=2.extra %s | \
+// RUN: FileCheck %s --implicit-check-not=.c:
+//
+// Verify that phony targets are only created for the extra dependency files,
+// and not the input file.
+
+// CHECK: dependency-gen-extradeps-phony.o: 1.extra 2.extra \
+// CHECK-NEXT: dependency-gen-extradeps-phony.c
+// CHECK: 1.extra:
+// CHECK: 2.extra:
Index: lib/Frontend/DependencyFile.cpp
===
--- lib/Frontend/DependencyFile.cpp
+++ lib/Frontend/DependencyFile.cpp
@@ -163,6 +163,7 @@
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
   DependencyOutputFormat OutputFormat;
+  unsigned InputFileIndex;
 
 private:
   bool FileMatchesDepCriteria(const char *Filename,
@@ -177,9 +178,11 @@
   AddMissingHeaderDeps(Opts.AddMissingHeaderDeps),
   SeenMissingHeader(false),
   IncludeModuleFiles(Opts.IncludeModuleFiles),
-  OutputFormat(Opts.OutputFormat) {
+  OutputFormat(Opts.OutputFormat),
+  InputFileIndex(0) {
 for (const auto &ExtraDep : Opts.ExtraDeps) {
-  AddFilename(ExtraDep);
+  if (AddFilename(ExtraDep))
+++InputFileIndex;
 }
   }
 
@@ -201,7 +204,7 @@
 OutputDependencyFile();
   }
 
-  void AddFilename(StringRef Filename);
+  bool AddFilename(StringRef Filename);
   bool includeSystemHeaders() const { return IncludeSystemHeaders; }
   bool includeModuleFiles() const { return IncludeModuleFiles; }
 };
@@ -325,9 +328,12 @@
   }
 }
 
-void DFGImpl::AddFilename(StringRef Filename) {
-  if (FilesSet.insert(Filename).second)
+bool DFGImpl::AddFilename(StringRef Filename) {
+  if (FilesSet.insert(Filename).second) {
 Files.push_back(Filename);
+return true;
+  }
+  return false;
 }
 
 /// Print the filename, with escaping or quoting that accommodates the three
@@ -463,8 +469,10 @@
 
   // Create phony targets if requested.
   if (PhonyTarget && !Files.empty()) {
-// Skip the first entry, this is always the input file itself.
-for (auto I = Files.begin() + 1, E = Files.end(); I != E; ++I) {
+unsigned Index = 0;
+for (auto I = Files.begin(), E = Files.end(); I != E; ++I) {
+  if (Index++ == InputFileIndex)
+continue;
   OS << '\n';
   PrintFilename(OS, *I, OutputFormat);
   OS << ":\n";


Index: test/Frontend/dependency-gen-extradeps-phony.c
===
--- test/Frontend/dependency-gen-extradeps-phony.c
+++ test/Frontend/dependency-gen-extradeps-phony.c
@@ -0,0 +1,10 @@
+// RUN: %clang -MM -MP -Xclang -fdepfile-entry=1.extra -Xclang -fdepfile-entry=2.extra -Xclang -fdepfile-entry=2.extra %s | \
+// RUN: FileCheck %s --implicit-check-not=.c:
+//
+// Verify that phony targets are only created for the extra dependency files,
+// and not the input file.
+
+// CHECK: dependency-gen-extradeps-phony.o: 1.extra 2.extra \
+// CHECK-NEXT: dependency-gen-extradeps-phony.c
+// CHECK: 1.extra:
+// CHECK: 2.extra:
Index: lib/Frontend/DependencyFile.cpp
===
--- lib/Frontend/DependencyFile.cpp
+++ lib/Frontend/DependencyFile.cpp
@@ -163,6 +163,7 @@
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
   DependencyOutputFormat OutputFormat;
+  unsigned InputFileIndex;
 
 private:
   bool FileMatchesDepCriteria(const char *Filename,
@@ -177,9 +178,11 @@
   AddMissingHeaderDeps(Opts.AddMissingHeaderDeps),
   SeenMissingHeader(false),
   IncludeModuleFiles(Opts.IncludeModuleFiles),
-  OutputFormat(Opts.OutputFormat) {
+  OutputFormat(Opts.OutputFormat),
+  InputFileIndex(0) {
 for (const auto &ExtraDep : Opts.ExtraDeps) {
-  AddFilename(ExtraDep);
+  if (AddFilename(ExtraDep))
+++InputFileIndex;
 }
   }
 
@@ -201,7 +204,7 @@
 OutputDependencyFile();
   }
 
-  void AddFilename(StringRef Filename);
+  bool AddFilename(StringRef Filename);
   bool includeSystemHeaders() const { return IncludeSystemHeaders; }
   bool includeModuleFiles() const { return IncludeModuleFiles; }
 };
@@ -325,9 +328,12 @@
   }
 }
 
-void DFGImpl::AddFilename(StringRef Filename) {
-  if (FilesSet.insert(Filename).second)
+bool DFGImpl::AddFilename(StringRef Filename) {
+  if (FilesSet.insert(Filename).second) {
 Files.push_back(Filename);
+return true

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

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


https://reviews.llvm.org/D45686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44568: Fix emission of phony dependency targets when adding extra deps

[David Stenberg via Phabricator via cfe-commits]

dstenb created this revision.
dstenb added reviewers: rsmith, pcc, krasin.
Herald added a subscriber: cfe-commits.

This commit fixes a bug where passing extra dependency entries
(using -fdepfile-entry) would result in -MP incorrectly emitting
a phony target for the input file, and no phony target for the
first extra dependency.

  

The extra dependencies are added first to the filename vector in
DFGImpl. That clashed with the emission of the phony targets, as
the code assumed that the first index always correspond to the
input file.


Repository:
  rC Clang

https://reviews.llvm.org/D44568

Files:
  lib/Frontend/DependencyFile.cpp
  test/Frontend/dependency-gen-extradeps-phony.c


Index: test/Frontend/dependency-gen-extradeps-phony.c
===
--- /dev/null
+++ test/Frontend/dependency-gen-extradeps-phony.c
@@ -0,0 +1,11 @@
+// RUN: %clang -MM -MP -Xclang -fdepfile-entry=1.extra -Xclang 
-fdepfile-entry=2.extra %s | FileCheck %s
+//
+// Verify that phony targets are only created for the extra dependency files,
+// and not the input file.
+
+// CHECK: dependency-gen-extradeps-phony.o: 1.extra 2.extra
+// CHECK-NOT: .c:
+// CHECK: 1.extra:
+// CHECK-NOT: .c:
+// CHECK: 2.extra:
+// CHECK-NOT: .c:
Index: lib/Frontend/DependencyFile.cpp
===
--- lib/Frontend/DependencyFile.cpp
+++ lib/Frontend/DependencyFile.cpp
@@ -162,6 +162,7 @@
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
   DependencyOutputFormat OutputFormat;
+  unsigned InputFileIndex;
 
 private:
   bool FileMatchesDepCriteria(const char *Filename,
@@ -176,9 +177,11 @@
   AddMissingHeaderDeps(Opts.AddMissingHeaderDeps),
   SeenMissingHeader(false),
   IncludeModuleFiles(Opts.IncludeModuleFiles),
-  OutputFormat(Opts.OutputFormat) {
+  OutputFormat(Opts.OutputFormat),
+  InputFileIndex(0) {
 for (const auto &ExtraDep : Opts.ExtraDeps) {
   AddFilename(ExtraDep);
+  ++InputFileIndex;
 }
   }
 
@@ -446,8 +449,10 @@
 
   // Create phony targets if requested.
   if (PhonyTarget && !Files.empty()) {
-// Skip the first entry, this is always the input file itself.
-for (auto I = Files.begin() + 1, E = Files.end(); I != E; ++I) {
+unsigned Index = 0;
+for (auto I = Files.begin(), E = Files.end(); I != E; ++I) {
+  if (Index++ == InputFileIndex)
+continue;
   OS << '\n';
   PrintFilename(OS, *I, OutputFormat);
   OS << ":\n";


Index: test/Frontend/dependency-gen-extradeps-phony.c
===
--- /dev/null
+++ test/Frontend/dependency-gen-extradeps-phony.c
@@ -0,0 +1,11 @@
+// RUN: %clang -MM -MP -Xclang -fdepfile-entry=1.extra -Xclang -fdepfile-entry=2.extra %s | FileCheck %s
+//
+// Verify that phony targets are only created for the extra dependency files,
+// and not the input file.
+
+// CHECK: dependency-gen-extradeps-phony.o: 1.extra 2.extra
+// CHECK-NOT: .c:
+// CHECK: 1.extra:
+// CHECK-NOT: .c:
+// CHECK: 2.extra:
+// CHECK-NOT: .c:
Index: lib/Frontend/DependencyFile.cpp
===
--- lib/Frontend/DependencyFile.cpp
+++ lib/Frontend/DependencyFile.cpp
@@ -162,6 +162,7 @@
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
   DependencyOutputFormat OutputFormat;
+  unsigned InputFileIndex;
 
 private:
   bool FileMatchesDepCriteria(const char *Filename,
@@ -176,9 +177,11 @@
   AddMissingHeaderDeps(Opts.AddMissingHeaderDeps),
   SeenMissingHeader(false),
   IncludeModuleFiles(Opts.IncludeModuleFiles),
-  OutputFormat(Opts.OutputFormat) {
+  OutputFormat(Opts.OutputFormat),
+  InputFileIndex(0) {
 for (const auto &ExtraDep : Opts.ExtraDeps) {
   AddFilename(ExtraDep);
+  ++InputFileIndex;
 }
   }
 
@@ -446,8 +449,10 @@
 
   // Create phony targets if requested.
   if (PhonyTarget && !Files.empty()) {
-// Skip the first entry, this is always the input file itself.
-for (auto I = Files.begin() + 1, E = Files.end(); I != E; ++I) {
+unsigned Index = 0;
+for (auto I = Files.begin(), E = Files.end(); I != E; ++I) {
+  if (Index++ == InputFileIndex)
+continue;
   OS << '\n';
   PrintFilename(OS, *I, OutputFormat);
   OS << ":\n";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44568: Fix emission of phony dependency targets when adding extra deps

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

A small caveat with this patch is that it does not fix the case where the input 
file as also added as an extra dependency with -fdepfile-entry; however, I 
reasoned that it shouldn't really be a problem in practice. I thought that it 
was a good trade-off ignoring that for slightly simpler code.


Repository:
  rC Clang

https://reviews.llvm.org/D44568



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58665: [analyzer] Handle comparison between non-default AS symbol and constant

[David Stenberg via Phabricator via cfe-commits]

dstenb created this revision.
dstenb added reviewers: NoQ, zaks.anna, george.karpenkov.
Herald added subscribers: cfe-commits, Charusso, jdoerfert, dkrupp, donat.nagy, 
Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: clang.

When comparing a symbolic region and a constant, the constant would be
widened or truncated to the width of a void pointer, meaning that the
constant could be incorrectly truncated when handling symbols for
non-default address spaces. In the attached test case this resulted in a
false positive since the constant was truncated to zero. To fix this,
widen/truncate the constant to the width of the symbol expression's
type.

This commit does not consider non-symbolic regions as I'm not sure how
to generalize getting the type there.

This fixes PR40814.


Repository:
  rC Clang

https://reviews.llvm.org/D58665

Files:
  lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  test/Analysis/ptr-cmp-const-trunc.cl


Index: test/Analysis/ptr-cmp-const-trunc.cl
===
--- /dev/null
+++ test/Analysis/ptr-cmp-const-trunc.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown -analyze 
-analyzer-checker=core -verify %s
+// expected-no-diagnostics
+
+#include 
+
+void bar(__global int *p) __attribute__((nonnull(1)));
+
+void foo(__global int *p) {
+  if ((uint64_t)p <= 1UL << 32)
+bar(p);
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -571,7 +571,11 @@
   // add 1 to a LocAsInteger, we'd better unpack the Loc and add to it,
   // then pack it back into a LocAsInteger.
   llvm::APSInt i = rhs.castAs().getValue();
-  BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
+  // FIXME: Handle non-default address spaces for non-symbolic regions.
+  if (SymbolRef lSym = lhs.getAsLocSymbol(true))
+BasicVals.getAPSIntType(lSym->getType()).apply(i);
+  else
+BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
   return evalBinOpLL(state, op, lhsL, makeLoc(i), resultTy);
 }
 default:


Index: test/Analysis/ptr-cmp-const-trunc.cl
===
--- /dev/null
+++ test/Analysis/ptr-cmp-const-trunc.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown -analyze -analyzer-checker=core -verify %s
+// expected-no-diagnostics
+
+#include 
+
+void bar(__global int *p) __attribute__((nonnull(1)));
+
+void foo(__global int *p) {
+  if ((uint64_t)p <= 1UL << 32)
+bar(p);
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -571,7 +571,11 @@
   // add 1 to a LocAsInteger, we'd better unpack the Loc and add to it,
   // then pack it back into a LocAsInteger.
   llvm::APSInt i = rhs.castAs().getValue();
-  BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
+  // FIXME: Handle non-default address spaces for non-symbolic regions.
+  if (SymbolRef lSym = lhs.getAsLocSymbol(true))
+BasicVals.getAPSIntType(lSym->getType()).apply(i);
+  else
+BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
   return evalBinOpLL(state, op, lhsL, makeLoc(i), resultTy);
 }
 default:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58665: [analyzer] Handle comparison between non-default AS symbol and constant

[David Stenberg via Phabricator via cfe-commits]

dstenb updated this revision to Diff 188538.
dstenb added a comment.

Address comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58665/new/

https://reviews.llvm.org/D58665

Files:
  lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  test/Analysis/ptr-cmp-const-trunc.cl


Index: test/Analysis/ptr-cmp-const-trunc.cl
===
--- /dev/null
+++ test/Analysis/ptr-cmp-const-trunc.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown -analyze 
-analyzer-checker=core -verify %s
+// expected-no-diagnostics
+
+#include 
+
+void bar(__global int *p) __attribute__((nonnull(1)));
+
+void foo(__global int *p) {
+  if ((uint64_t)p <= 1UL << 32)
+bar(p); // no-warning
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -571,7 +571,15 @@
   // add 1 to a LocAsInteger, we'd better unpack the Loc and add to it,
   // then pack it back into a LocAsInteger.
   llvm::APSInt i = rhs.castAs().getValue();
-  BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
+  // If the region has a symbolic base, pay attention to the type; it
+  // might be coming from a non-default address space. For non-symbolic
+  // regions it doesn't matter that much because such comparisons would
+  // most likely evaluate to concrete false anyway. FIXME: We might
+  // still need to handle the non-comparison case.
+  if (SymbolRef lSym = lhs.getAsLocSymbol(true))
+BasicVals.getAPSIntType(lSym->getType()).apply(i);
+  else
+BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
   return evalBinOpLL(state, op, lhsL, makeLoc(i), resultTy);
 }
 default:


Index: test/Analysis/ptr-cmp-const-trunc.cl
===
--- /dev/null
+++ test/Analysis/ptr-cmp-const-trunc.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown -analyze -analyzer-checker=core -verify %s
+// expected-no-diagnostics
+
+#include 
+
+void bar(__global int *p) __attribute__((nonnull(1)));
+
+void foo(__global int *p) {
+  if ((uint64_t)p <= 1UL << 32)
+bar(p); // no-warning
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -571,7 +571,15 @@
   // add 1 to a LocAsInteger, we'd better unpack the Loc and add to it,
   // then pack it back into a LocAsInteger.
   llvm::APSInt i = rhs.castAs().getValue();
-  BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
+  // If the region has a symbolic base, pay attention to the type; it
+  // might be coming from a non-default address space. For non-symbolic
+  // regions it doesn't matter that much because such comparisons would
+  // most likely evaluate to concrete false anyway. FIXME: We might
+  // still need to handle the non-comparison case.
+  if (SymbolRef lSym = lhs.getAsLocSymbol(true))
+BasicVals.getAPSIntType(lSym->getType()).apply(i);
+  else
+BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
   return evalBinOpLL(state, op, lhsL, makeLoc(i), resultTy);
 }
 default:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58665: [analyzer] Handle comparison between non-default AS symbol and constant

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

Thanks for the review! I'll submit this shortly then.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58665/new/

https://reviews.llvm.org/D58665



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58665: [analyzer] Handle comparison between non-default AS symbol and constant

[David Stenberg via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC355592: [analyzer] Handle comparison between non-default AS 
symbol and constant (authored by dstenb, committed by ).

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58665/new/

https://reviews.llvm.org/D58665

Files:
  lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  test/Analysis/ptr-cmp-const-trunc.cl


Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -571,7 +571,15 @@
   // add 1 to a LocAsInteger, we'd better unpack the Loc and add to it,
   // then pack it back into a LocAsInteger.
   llvm::APSInt i = rhs.castAs().getValue();
-  BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
+  // If the region has a symbolic base, pay attention to the type; it
+  // might be coming from a non-default address space. For non-symbolic
+  // regions it doesn't matter that much because such comparisons would
+  // most likely evaluate to concrete false anyway. FIXME: We might
+  // still need to handle the non-comparison case.
+  if (SymbolRef lSym = lhs.getAsLocSymbol(true))
+BasicVals.getAPSIntType(lSym->getType()).apply(i);
+  else
+BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
   return evalBinOpLL(state, op, lhsL, makeLoc(i), resultTy);
 }
 default:
Index: test/Analysis/ptr-cmp-const-trunc.cl
===
--- test/Analysis/ptr-cmp-const-trunc.cl
+++ test/Analysis/ptr-cmp-const-trunc.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown -analyze 
-analyzer-checker=core -verify %s
+// expected-no-diagnostics
+
+#include 
+
+void bar(__global int *p) __attribute__((nonnull(1)));
+
+void foo(__global int *p) {
+  if ((uint64_t)p <= 1UL << 32)
+bar(p); // no-warning
+}


Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -571,7 +571,15 @@
   // add 1 to a LocAsInteger, we'd better unpack the Loc and add to it,
   // then pack it back into a LocAsInteger.
   llvm::APSInt i = rhs.castAs().getValue();
-  BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
+  // If the region has a symbolic base, pay attention to the type; it
+  // might be coming from a non-default address space. For non-symbolic
+  // regions it doesn't matter that much because such comparisons would
+  // most likely evaluate to concrete false anyway. FIXME: We might
+  // still need to handle the non-comparison case.
+  if (SymbolRef lSym = lhs.getAsLocSymbol(true))
+BasicVals.getAPSIntType(lSym->getType()).apply(i);
+  else
+BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
   return evalBinOpLL(state, op, lhsL, makeLoc(i), resultTy);
 }
 default:
Index: test/Analysis/ptr-cmp-const-trunc.cl
===
--- test/Analysis/ptr-cmp-const-trunc.cl
+++ test/Analysis/ptr-cmp-const-trunc.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown -analyze -analyzer-checker=core -verify %s
+// expected-no-diagnostics
+
+#include 
+
+void bar(__global int *p) __attribute__((nonnull(1)));
+
+void foo(__global int *p) {
+  if ((uint64_t)p <= 1UL << 32)
+bar(p); // no-warning
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55702: [clangd] Fix memory leak in ClangdTests.

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

With this patch applied we don't see any issues on our end. Thanks for the help!


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55702/new/

https://reviews.llvm.org/D55702



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44568: Fix emission of phony dependency targets when adding extra deps

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

Ping.


Repository:
  rC Clang

https://reviews.llvm.org/D44568



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45686: [Tooling] Clean up tmp files when creating a fixed compilation database

[David Stenberg via Phabricator via cfe-commits]

dstenb created this revision.
dstenb added reviewers: klimek, sepavloff, arphaman.
Herald added a subscriber: cfe-commits.

In https://reviews.llvm.org/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 the Compilation object's temporary
file list before the object is deleted at end-of-scope in
stripPositionalArgs().

I am not very well-versed in Driver/Tooling, so I don't know if this
should be seen as a proper fix, or as a temporary workaround.
I at least assume that we want to use a race-free variant rather
than getPotentiallyUniqueTempFileName() in
Driver::GetNamedOutputPath().

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


Repository:
  rC Clang

https://reviews.llvm.org/D45686

Files:
  lib/Tooling/CompilationDatabase.cpp


Index: lib/Tooling/CompilationDatabase.cpp
===
--- lib/Tooling/CompilationDatabase.cpp
+++ lib/Tooling/CompilationDatabase.cpp
@@ -299,6 +299,9 @@
 }
   }
 
+  // Remove temp files.
+  Compilation->CleanupFileList(Compilation->getTempFiles());
+
   if (CompileAnalyzer.Inputs.empty()) {
 ErrorMsg = "warning: no compile jobs found\n";
 return false;


Index: lib/Tooling/CompilationDatabase.cpp
===
--- lib/Tooling/CompilationDatabase.cpp
+++ lib/Tooling/CompilationDatabase.cpp
@@ -299,6 +299,9 @@
 }
   }
 
+  // Remove temp files.
+  Compilation->CleanupFileList(Compilation->getTempFiles());
+
   if (CompileAnalyzer.Inputs.empty()) {
 ErrorMsg = "warning: no compile jobs found\n";
 return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45686: [Tooling] Clean up tmp files when creating a fixed compilation database

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D45686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91519: [AST][Mach0] Fix unused-variable warnings

[David Stenberg via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2d1f471e45af: [Mach0] Fix unused-variable warnings (authored 
by ehjogab, committed by dstenb).
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D91519?vs=305984&id=306339#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91519/new/

https://reviews.llvm.org/D91519

Files:
  lld/MachO/SymbolTable.cpp


Index: lld/MachO/SymbolTable.cpp
===
--- lld/MachO/SymbolTable.cpp
+++ lld/MachO/SymbolTable.cpp
@@ -134,7 +134,7 @@
 // FIXME: Make every symbol (including absolute symbols) contain a
 // reference to their originating file, then add that file name to this
 // error message.
-if (auto *defined = dyn_cast(s))
+if (isa(s))
   error("found defined symbol with illegal name " + DSOHandle::name);
   }
   replaceSymbol(s, header);


Index: lld/MachO/SymbolTable.cpp
===
--- lld/MachO/SymbolTable.cpp
+++ lld/MachO/SymbolTable.cpp
@@ -134,7 +134,7 @@
 // FIXME: Make every symbol (including absolute symbols) contain a
 // reference to their originating file, then add that file name to this
 // error message.
-if (auto *defined = dyn_cast(s))
+if (isa(s))
   error("found defined symbol with illegal name " + DSOHandle::name);
   }
   replaceSymbol(s, header);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114034: [clang-tidy] fix debug-only test failure

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

In D114034#3136057 , @mattbeardsley 
wrote:

> @dstenb - wanted to check with you too to make sure that this change to 
> pr37091.cpp seems like it won't interfere with the original intent of the 
> test (https://reviews.llvm.org/D45686 )
>
> The lingering file issue you'd fixed seemed to be independent of any 
> particular clang-tidy check, but if not, hopefully it can at least get away 
> with not relying on the top-level .clang-tidy anymore!

Thanks for asking!

Yes, I think that the issue was seen independently of the checks used. We saw 
that issue originally with clang-tidy invocations with explicit -checks 
arguments (specifying clang-analyzer-* and some more checks). So, I guess that 
just specifying some check here should be fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114034/new/

https://reviews.llvm.org/D114034

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

In D73534#1881353 , @nickdesaulniers 
wrote:

> As a heads up, Linaro's ToolChain Working Group's Linux kernel CI lit up on 
> this change. I see it's been reverted (thank you). Please cc me on the 
> updated patch and I can help test it against the Linux kernel.
>
> Edit: Link: 
> https://ci.linaro.org/job/tcwg_kernel-bisect-llvm-master-arm-mainline-allyesconfig/25/artifact/artifacts/build-a82d3e8a6e67473c94a5ce6345372748e9b61718/03-build_linux/console.log


Its hard to tell from the backtrace, but looking at the code, I think this 
might be a bug that sneaked in when I did D70431 
. Sorry if that is the case!

`ARMBaseInstrInfo::isAddImmediate()` does a `getReg()` without any `isReg()` 
guard:

  Optional ARMBaseInstrInfo::isAddImmediate(const MachineInstr &MI, 



 
Register Reg) const {   



 
  [...]
// TODO: Handle cases where Reg is a super- or sub-register of the  



 
// destination register.
if (Reg != MI.getOperand(0).getReg())
  return None;


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73534/new/

https://reviews.llvm.org/D73534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

In D73534#1882127 , @djtodoro wrote:

> @dstenb No problem, we have already addressed the issue. Thanks to @vsk and 
> @nickdesaulniers! I'll update the patch.


Great, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73534/new/

https://reviews.llvm.org/D73534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71508: [DebugInfo] Duplicate file names in debug info

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

It should be possible to create a test for this using something like:

  RUN: rm -rf %t && mkdir %t
  RUN: cp %s %t/filename.c
  RUN: cd %t
  RUN: clang ./filename.c [...]




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71508/new/

https://reviews.llvm.org/D71508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

In D73534#1918890 , @manojgupta wrote:

> Hi,
>
> I see another crash with this change when building gdb.
>
> Reduced test case:
>  struct type *a(type *, type *, long, long);
>  enum b {};
>  static int empty_array(type *, int c) { type *d = a(__null, d, c, c - 1); }
>  long e;
>  b f() { empty_array(0, e); }
>
> Repros with:
>  clang -cc1 -triple x86_64-linux-gnu -emit-obj -disable-free 
> -mrelocation-model pic -pic-level 2 -pic-is-pie -mthread-model posix 
> -mframe-pointer=all  -mconstructor-aliases -munwind-tables  
> -dwarf-column-info  -debug-info-kind=limited -dwarf-version=4 
> -debugger-tuning=gdb  -O2  -x c++
>
> Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1060788


Oh, that assertion is related to D75036  which 
I did. I can have a look at that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73534/new/

https://reviews.llvm.org/D73534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

In D73534#1918940 , @dstenb wrote:

> In D73534#1918890 , @manojgupta 
> wrote:
>
> > Hi,
> >
> > I see another crash with this change when building gdb.
> >
> > Reduced test case:
> >  struct type *a(type *, type *, long, long);
> >  enum b {};
> >  static int empty_array(type *, int c) { type *d = a(__null, d, c, c - 1); }
> >  long e;
> >  b f() { empty_array(0, e); }
> >
> > Repros with:
> >  clang -cc1 -triple x86_64-linux-gnu -emit-obj -disable-free 
> > -mrelocation-model pic -pic-level 2 -pic-is-pie -mthread-model posix 
> > -mframe-pointer=all  -mconstructor-aliases -munwind-tables  
> > -dwarf-column-info  -debug-info-kind=limited -dwarf-version=4 
> > -debugger-tuning=gdb  -O2  -x c++
> >
> > Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1060788
>
>
> Oh, that assertion is related to D75036  
> which I did. I can have a look at that.


I wrote a PR for that: https://bugs.llvm.org/show_bug.cgi?id=45181.

I'll see if I'm able to put together something for that today.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73534/new/

https://reviews.llvm.org/D73534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[David Stenberg via Phabricator via cfe-commits]

dstenb added inline comments.



Comment at: llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:870
 if (MI->isCandidateForCallSiteEntry() &&
-DAG->getTarget().Options.EnableDebugEntryValues)
+DAG->getTarget().Options.SupportsDebugEntryValues)
   MF.addCallArgsForwardingRegs(MI, DAG->getSDCallSiteInfo(Node));

I'm sorry for commenting on this so late, but when now adapting this patch for 
our downstream target, for which we will not enable call site info by default 
yet, I noticed that the call site information wasn't added. I think that this 
should be `ShouldEmitDebugEntryValues()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73534/new/

https://reviews.llvm.org/D73534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[David Stenberg via Phabricator via cfe-commits]

dstenb added inline comments.



Comment at: llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:870
 if (MI->isCandidateForCallSiteEntry() &&
-DAG->getTarget().Options.EnableDebugEntryValues)
+DAG->getTarget().Options.SupportsDebugEntryValues)
   MF.addCallArgsForwardingRegs(MI, DAG->getSDCallSiteInfo(Node));

djtodoro wrote:
> dstenb wrote:
> > I'm sorry for commenting on this so late, but when now adapting this patch 
> > for our downstream target, for which we will not enable call site info by 
> > default yet, I noticed that the call site information wasn't added. I think 
> > that this should be `ShouldEmitDebugEntryValues()`.
> I thought to restrict the production of the call site info here and only to 
> allow a hand-made testing, but I think you are right, we should use the 
> `ShouldEmitDebugEntryValues()`. I'll update the patch, thanks!
Thanks! It is perhaps a bit of a corner case, and we're moving towards enabling 
call site info by default, but I guess this might be useful when experimenting 
with adding call site info for other targets in the future.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73534/new/

https://reviews.llvm.org/D73534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44774: [Driver] Allow use of -fsyntax-only together with -MJ

[David Stenberg via Phabricator via cfe-commits]

dstenb created this revision.
dstenb added reviewers: joerg, klimek, rsmith.
Herald added a subscriber: cfe-commits.

When using -MJ together with -fsyntax-only, clang would hit an assert in
DumpCompilationDatabase() when trying to get the filename for the output
field. This patch fixes that by amending DumpCompilationDatabase() so
that it accepts the `nothing' output class, and it will in that case
simply omit the output field.

The JSON Compilation Database Format Specification specifies that the
output field is optional.


Repository:
  rC Clang

https://reviews.llvm.org/D44774

Files:
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/compilation_database.c


Index: test/Driver/compilation_database.c
===
--- test/Driver/compilation_database.c
+++ test/Driver/compilation_database.c
@@ -1,10 +1,12 @@
 // RUN: mkdir -p %t && cd %t
 // RUN: %clang -MD -MP --sysroot=somewhere -c -x c %s -xc++ %s -Wall -MJ - 
-no-canonical-prefixes 2>&1 | FileCheck %s
 // RUN: not %clang -c -x c %s -MJ %s/non-existant -no-canonical-prefixes 2>&1 
| FileCheck --check-prefix=ERROR %s
+// RUN: %clang %s --sysroot=somewhere -MJ - -fsyntax-only 
-no-canonical-prefixes 2>&1 | FileCheck --check-prefix=FSYNTAX-ONLY %s
 
 // CHECK: { "directory": "{{.*}}",  "file": 
"[[SRC:[^"]+[/|\\]compilation_database.c]]", "output": 
"compilation_database.o", "arguments": ["{{[^"]*}}clang{{[^"]*}}", "-xc", 
"[[SRC]]", "--sysroot=somewhere", "-c", "-Wall",{{.*}} "--target={{[^"]+}}"]},
 // CHECK: { "directory": "{{.*}}",  "file": 
"[[SRC:[^"]+[/|\\]compilation_database.c]]", "output": 
"compilation_database.o", "arguments": ["{{[^"]*}}clang{{[^"]*}}", "-xc++", 
"[[SRC]]", "--sysroot=somewhere", "-c", "-Wall",{{.*}} "--target={{[^"]+}}"]},
 // ERROR: error: compilation database '{{.*}}/non-existant' could not be 
opened:
+// FSYNTAX-ONLY: { "directory": "{{.*}}",  "file": 
"[[SRC:[^"]+[/|\\]compilation_database.c]]", "arguments": 
["{{[^"]*}}clang{{[^"]*}}", "-xc", "[[SRC]]", "--sysroot=somewhere", {{.*}} 
"--target={{[^"]+}}"]},
 
 int main(void) {
   return 0;
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -1856,7 +1856,8 @@
 Buf = ".";
   CDB << "{ \"directory\": \"" << escape(Buf) << "\"";
   CDB << ", \"file\": \"" << escape(Input.getFilename()) << "\"";
-  CDB << ", \"output\": \"" << escape(Output.getFilename()) << "\"";
+  if (!Output.isNothing())
+CDB << ", \"output\": \"" << escape(Output.getFilename()) << "\"";
   CDB << ", \"arguments\": [\"" << escape(D.ClangExecutable) << "\"";
   Buf = "-x";
   Buf += types::getTypeName(Input.getType());


Index: test/Driver/compilation_database.c
===
--- test/Driver/compilation_database.c
+++ test/Driver/compilation_database.c
@@ -1,10 +1,12 @@
 // RUN: mkdir -p %t && cd %t
 // RUN: %clang -MD -MP --sysroot=somewhere -c -x c %s -xc++ %s -Wall -MJ - -no-canonical-prefixes 2>&1 | FileCheck %s
 // RUN: not %clang -c -x c %s -MJ %s/non-existant -no-canonical-prefixes 2>&1 | FileCheck --check-prefix=ERROR %s
+// RUN: %clang %s --sysroot=somewhere -MJ - -fsyntax-only -no-canonical-prefixes 2>&1 | FileCheck --check-prefix=FSYNTAX-ONLY %s
 
 // CHECK: { "directory": "{{.*}}",  "file": "[[SRC:[^"]+[/|\\]compilation_database.c]]", "output": "compilation_database.o", "arguments": ["{{[^"]*}}clang{{[^"]*}}", "-xc", "[[SRC]]", "--sysroot=somewhere", "-c", "-Wall",{{.*}} "--target={{[^"]+}}"]},
 // CHECK: { "directory": "{{.*}}",  "file": "[[SRC:[^"]+[/|\\]compilation_database.c]]", "output": "compilation_database.o", "arguments": ["{{[^"]*}}clang{{[^"]*}}", "-xc++", "[[SRC]]", "--sysroot=somewhere", "-c", "-Wall",{{.*}} "--target={{[^"]+}}"]},
 // ERROR: error: compilation database '{{.*}}/non-existant' could not be opened:
+// FSYNTAX-ONLY: { "directory": "{{.*}}",  "file": "[[SRC:[^"]+[/|\\]compilation_database.c]]", "arguments": ["{{[^"]*}}clang{{[^"]*}}", "-xc", "[[SRC]]", "--sysroot=somewhere", {{.*}} "--target={{[^"]+}}"]},
 
 int main(void) {
   return 0;
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -1856,7 +1856,8 @@
 Buf = ".";
   CDB << "{ \"directory\": \"" << escape(Buf) << "\"";
   CDB << ", \"file\": \"" << escape(Input.getFilename()) << "\"";
-  CDB << ", \"output\": \"" << escape(Output.getFilename()) << "\"";
+  if (!Output.isNothing())
+CDB << ", \"output\": \"" << escape(Output.getFilename()) << "\"";
   CDB << ", \"arguments\": [\"" << escape(D.ClangExecutable) << "\"";
   Buf = "-x";
   Buf += types::getTypeName(Input.getType());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44774: [Driver] Allow use of -fsyntax-only together with -MJ

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

Downstream we use -MJ in a bit of an idiosyncratic way, as we're in a 
transition period where we, for a subset of the code base, only use the clang 
frontend for diagnostics, and not for the code generation. However, if you 
don't think that using -fsyntax-only and -MJ makes sense in any upstream 
application, I'll drop from this change. I'm leaving the assertion as-is.


Repository:
  rC Clang

https://reviews.llvm.org/D44774



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44774: [Driver] Allow use of -fsyntax-only together with -MJ

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

Our legacy frontend does not support -MJ, so when using that frontend for code 
generation, we invoke clang with -MJ, and at the same use -fsyntax-only to get 
the improved diagnostics that clang provides. This is idiosyncratic and 
probably hacky, I know, but it works well enough to for example for getting 
access to defines and include flags from the compilation database, and being 
able to run clang-tidy. So (1) does not fit our use case, unfortunately.

Our way of running should of course in itself not  affect what is done 
upstream, but in general I don't see why someone shouldn't be allowed to do, 
for example, the following in their build chain:

  clang -MJ - -DFOO=usual-value foo.c -o foo.out # code generation
  clang -MJ - -DFOO=unusual-value -fsyntax-value foo.c # quick sanity check

and then run a tool like clang-tidy with the resulting compilation database.

Disregarding the above, I agree with the POLA issue with (1), so out of those 
two options the latter seems better to me.


Repository:
  rC Clang

https://reviews.llvm.org/D44774



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits