Fix emission of phony dependency targets when adding extra deps

[dstenb]

Summary

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.

[dstenb]

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.

[dstenb]

Ping.

[dstenb]

Ping.

[dstenb]

@bruno, @vsapsai: I added you since you I saw that you recently reviewed, respectively delivered, 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)!

[vsapsai]

lib/Frontend/DependencyFile.cpp

183–186

This is incorrect if there are duplicates in Opts.ExtraDeps. Also please update the test to have duplicate extra dependencies.

test/Frontend/dependency-gen-extradeps-phony.c

7–8

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.

10–12

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.

[dstenb]

Addressed vsapsai's comments.

[dstenb]

test/Frontend/dependency-gen-extradeps-phony.c

7–8

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.

10–12

I'll add that in the next patch (and I'll keep that in mind for future changes). Thanks!

[vsapsai]

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

As for the case when the input file is also an extra dependency, I think we can ignore that for now because extra dependencies are supposed to be sanitizer blacklists. Even if that changes in the future, I expect extra dependencies to stay orthogonal to the input.

test/Frontend/dependency-gen-extradeps-phony.c

7–8

You are right, you need to be careful to make sure that we match .c file only as a dependency and not as a target. Good solution with CHECK-NEXT. Now I'm happy with the test as expected output is clearly visible and we protect from undesired output.

[dstenb]

In 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!

lib/Frontend/DependencyFile.cpp

class DFGImpl : public PPCallbacks {
   std::string OutputFile;
   std::vector<std::string> Targets;
   bool IncludeSystemHeaders;
   bool PhonyTarget;
   bool AddMissingHeaderDeps;
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
   DependencyOutputFormat OutputFormat;
+  unsigned InputFileIndex;
 
 private:
   bool FileMatchesDepCriteria(const char *Filename,
                               SrcMgr::CharacteristicKind FileType);
   void OutputDependencyFile();
 
 public:
   DFGImpl(const Preprocessor *_PP, const DependencyOutputOptions &Opts)
     : PP(_PP), OutputFile(Opts.OutputFile), Targets(Opts.Targets),
       IncludeSystemHeaders(Opts.IncludeSystemHeaders),
       PhonyTarget(Opts.UsePhonyTargets),
       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;
     }
   }
 
   void FileChanged(SourceLocation Loc, FileChangeReason Reason,
                    SrcMgr::CharacteristicKind FileType,
                    FileID PrevFID) override;
 
   void FileSkipped(const FileEntry &SkippedFile, const Token &FilenameTok,
                    SrcMgr::CharacteristicKind FileType) override;
 
   void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
                           StringRef FileName, bool IsAngled,
                           CharSourceRange FilenameRange, const FileEntry *File,
                           StringRef SearchPath, StringRef RelativePath,
                           const Module *Imported,
                           SrcMgr::CharacteristicKind FileType) override;
 
   void EndOfMainFile() override {
     OutputDependencyFile();
   }
 
-  void AddFilename(StringRef Filename);
+  bool AddFilename(StringRef Filename);
   bool includeSystemHeaders() const { return IncludeSystemHeaders; }
   bool includeModuleFiles() const { return IncludeModuleFiles; }
 };
 
 class DFGMMCallback : public ModuleMapCallbacks {
   DFGImpl &Parent;
 public:
   DFGMMCallback(DFGImpl &Parent) : Parent(Parent) {}
void DFGImpl::InclusionDirective(SourceLocation HashLoc,
   if (!File) {
     if (AddMissingHeaderDeps)
       AddFilename(FileName);
     else
       SeenMissingHeader = true;
   }
 }
 
-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
 /// most likely tools that use dependency files: GNU Make, BSD Make, and
 /// NMake/Jom.
 ///
 /// BSD Make is the simplest case: It does no escaping at all.  This means
 /// characters that are normally delimiters, i.e. space and # (the comment
for (StringRef File : Files) {
     OS << ' ';
     PrintFilename(OS, File, OutputFormat);
     Columns += N + 1;
   }
   OS << '\n';
 
   // 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";
     }
   }
 }
 
 bool DFGASTReaderListener::visitInputFile(llvm::StringRef Filename,

test/Frontend/dependency-gen-extradeps-phony.c

+// 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: