Track skipped files in dependency scanning

[vsapsai]

Summary

It's possible for a header to be a symlink to another header. In this
case both will be represented by clang::FileEntry with the same UID and
they'll use the same clang::HeaderFileInfo.

If you include both headers and use some single-inclusion mechanism
like a header guard or #import, one header will get a FileChanged
callback, and another FileSkipped.

So that we get an accurate dependency file, we therefore need to also
implement the FileSkipped callback in dependency scanning.

Patch by Pete Cooper.

[bruno]

Hi Pete,

lib/Frontend/DependencyFile.cpp

191 ↗

(On Diff #91518)

Is there any FileSkipped callback invocation that might trigger an unwanted file to be added as a dependency or it will only trigger for the symlink case?

test/Frontend/dependency-gen-symlink.c

11 ↗

(On Diff #91518)

It seems that it works for hard links as well right? Is this intended or do you miss a -s?

[vsapsai]

I plan to look into cleaning this up and addressing the review comments.

Need to check if DepCollectorPPCallbacks has to be updated too.

lib/Frontend/DependencyFile.cpp

302 ↗

(On Diff #91518)

I suspect we need to call FileMatchesDepCriteria here too.

[vsapsai]

• Fix FileSkipped adding system headers when it shouldn't.

[vsapsai]

lib/Frontend/DependencyFile.cpp

191 ↗

(On Diff #91518)

FileSkipped is called when a file has a mechanism for single inclusion (header guards or #import) and it was already included. The callback will be triggered also for

// a.h
#ifndef A_H_
#define A_H_
#include "b.h"
#endif

// b.h
#ifndef B_H_
#define B_H_
#endif

// test.c
#include "a.h"
#include "b.h"

But AddFilename performs Filename uniqueness check, so there will be no duplicate b.h in dependencies.

If the same file is included by multiple paths, all paths can end up in dependency file. It is actually what we want to achieve and it shouldn't cause unnecessary rebuilds.

test/Frontend/dependency-gen-symlink.c

11 ↗

(On Diff #91518)

It works when files have the same inode. Added -s to capture more common use case.

[bruno]

Thanks for the detailed answers. LGTM

cfe/trunk/lib/Frontend/DependencyFile.cpp

DFGImpl(const Preprocessor *_PP, const DependencyOutputOptions &Opts)
     for (const auto &ExtraDep : Opts.ExtraDeps) {
       AddFilename(ExtraDep);
     }
   }
 
   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) override;
 
   void EndOfMainFile() override {
     OutputDependencyFile();
void DFGImpl::FileChanged(SourceLocation Loc,
 
   StringRef Filename = FE->getName();
   if (!FileMatchesDepCriteria(Filename.data(), FileType))
     return;
 
   AddFilename(llvm::sys::path::remove_leading_dotslash(Filename));
 }
 
+void DFGImpl::FileSkipped(const FileEntry &SkippedFile,
+                          const Token &FilenameTok,
+                          SrcMgr::CharacteristicKind FileType) {
+  StringRef Filename = SkippedFile.getName();
+  if (!FileMatchesDepCriteria(Filename.data(), FileType))
+    return;
+
+  AddFilename(llvm::sys::path::remove_leading_dotslash(Filename));
+}
+
 void DFGImpl::InclusionDirective(SourceLocation HashLoc,
                                  const Token &IncludeTok,
                                  StringRef FileName,
                                  bool IsAngled,
                                  CharSourceRange FilenameRange,
                                  const FileEntry *File,
                                  StringRef SearchPath,
                                  StringRef RelativePath,

cfe/trunk/test/Frontend/Inputs/SystemHeaderPrefix/with-header-guard.h

+#ifndef HEADER_GUARD
+#define HEADER_GUARD
+#endif

cfe/trunk/test/Frontend/dependency-gen-symlink.c

+// REQUIRES: shell
+
+// Basic test
+// RUN: rm -rf %t.dir
+// RUN: mkdir %t.dir
+// RUN: mkdir %t.dir/a
+// RUN: mkdir %t.dir/b
+// RUN: echo "#ifndef HEADER_A" > %t.dir/a/header.h
+// RUN: echo "#define HEADER_A" >> %t.dir/a/header.h
+// RUN: echo "#endif" >> %t.dir/a/header.h
+// RUN: ln -s %t.dir/a/header.h %t.dir/b/header.h
+
+// RUN: %clang_cc1 -dependency-file %t.dir/file.deps -MT %s.o %s -fsyntax-only -I %t.dir -isystem %S/Inputs/SystemHeaderPrefix
+// RUN: FileCheck -input-file=%t.dir/file.deps %s
+// CHECK: dependency-gen-symlink.c.o
+// CHECK: dependency-gen-symlink.c
+// CHECK: a/header.h
+// CHECK: b/header.h
+// CHECK-NOT: with-header-guard.h
+#include "a/header.h"
+#include "b/header.h"
+// System header shouldn't be included in dependencies.
+#include <with-header-guard.h>
+#include <with-header-guard.h>