Add a callback for __has_include and use it for dependency scanning

[vsapsai]

Summary

This adds a PP callback for the has_include and has_include_next directives.

Checking for the presence of a header should add it to the list of header dependencies so this overrides the callback in the dependency scanner.

I kept the callback intentionally simple for now. If we want to extend it to track things like missing headers, just as PP->FileNotFound() does today, then that wouldn't be hard to add later.

[bruno]

Hi Pete, thanks for working on this!

LGTM with the minor comments below.

include/clang/Lex/PPCallbacks.h

264 ↗

(On Diff #91520)

clang-format this one!

test/Frontend/dependency-gen-has-include.c

17 ↗

(On Diff #91520)

Can you also add a testcase for __has_include_next?

[dexonsmith]

I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.

Given t.cpp:

#if __has_include("missing.h")
#endif

t.d will be:

t.o: missing.h

Since the build system doesn't know how to generate missing.h, when t.cpp changes the build system will stop dead in its tracks.

Knowing the list of files that are potential inputs to t.o seems useful, but we should probably do that under a separate flag. And if it's a separate flag, we might consider giving it a better name than -MF...

[• ddunbar]

In D30882#1074822, @dexonsmith wrote:

I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.

This is actually not a novel problem w.r.t. this patch. The exact same situation comes up with Makefile-included .d files and when one of the referenced headers is removed.

This is typically solved somewhere in the build system, for example Make has -include, and Ninja and llbuild have explicit support for this situation.

I agree we might want to tread cautiously on how we change the .d output, but in this case I think it might be safe.

If we decide this isn't safe, then we may want to consider a new flag which tracks *all* anti-dependencies (file's for which Clang checked existence but did not exist), and include that here. The major concern with that approach is it is a much larger list, and while it would support being substantially more correct, it is also well beyond what people currently expect out of the build system + compiler generated deps approaches.

Given t.cpp:

#if __has_include("missing.h")
#endif

t.d will be:

t.o: missing.h

Since the build system doesn't know how to generate missing.h, when t.cpp changes the build system will stop dead in its tracks.

Knowing the list of files that are potential inputs to t.o seems useful, but we should probably do that under a separate flag. And if it's a separate flag, we might consider giving it a better name than -MF...

[dexonsmith]

In D30882#1075407, @ddunbar wrote:

In D30882#1074822, @dexonsmith wrote:

I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.

This is actually not a novel problem w.r.t. this patch. The exact same situation comes up with Makefile-included .d files and when one of the referenced headers is removed.

This is typically solved somewhere in the build system, for example Make has -include, and Ninja and llbuild have explicit support for this situation.

Of course, yes, I was wrong. Thanks for the correction.

I agree we might want to tread cautiously on how we change the .d output, but in this case I think it might be safe.

If we decide this isn't safe, then we may want to consider a new flag which tracks *all* anti-dependencies (file's for which Clang checked existence but did not exist), and include that here. The major concern with that approach is it is a much larger list, and while it would support being substantially more correct, it is also well beyond what people currently expect out of the build system + compiler generated deps approaches.

Even though this is likely safe, it seems really noisy. Consider the case where someone has __has_include(<missing>) -- we'll get an entry for every -I, -F, -isystem, and -iframework. If we're going to add that noise, I prefer a separate flag that covers all anti-dependencies.

test/Frontend/dependency-gen-has-include.c

11–15 ↗

(On Diff #91520)

This covers quoted includes when there is a single -I. I think there are a couple of other cases worth testing:

• Multiple -I arguments: all should be listed.
• Angle includes: all -isystem should be listed too.
• Also -F and -iframework.

[pete]

In D30882#1075461, @dexonsmith wrote:

In D30882#1075407, @ddunbar wrote:

In D30882#1074822, @dexonsmith wrote:

I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.

This is actually not a novel problem w.r.t. this patch. The exact same situation comes up with Makefile-included .d files and when one of the referenced headers is removed.

This is typically solved somewhere in the build system, for example Make has -include, and Ninja and llbuild have explicit support for this situation.

Of course, yes, I was wrong. Thanks for the correction.

I agree we might want to tread cautiously on how we change the .d output, but in this case I think it might be safe.

If we decide this isn't safe, then we may want to consider a new flag which tracks *all* anti-dependencies (file's for which Clang checked existence but did not exist), and include that here. The major concern with that approach is it is a much larger list, and while it would support being substantially more correct, it is also well beyond what people currently expect out of the build system + compiler generated deps approaches.

Even though this is likely safe, it seems really noisy. Consider the case where someone has __has_include(<missing>) -- we'll get an entry for every -I, -F, -isystem, and -iframework. If we're going to add that noise, I prefer a separate flag that covers all anti-dependencies.

Oh, that actually wasn't my intention when I wrote it.

Honestly I didn't expect it to log anything for missing paths at all, as we don't currently log all the missing (but attempted) paths for regular #include's.

Would it be ok to turn this on by default, without a flag, only in the case of the path actually existing, and only the found path being the one we add to the .d?

[dexonsmith]

In D30882#1075576, @pete wrote:

Oh, that actually wasn't my intention when I wrote it.

Honestly I didn't expect it to log anything for missing paths at all, as we don't currently log all the missing (but attempted) paths for regular #include's.

Then I misunderstood the patch. Maybe add a test or two that confirms it only adds dependencies, not anti-dependencies?

Would it be ok to turn this on by default, without a flag, only in the case of the path actually existing, and only the found path being the one we add to the .d?

I think that pessimizes some incremental builds:

• You have a __has_include("missing.h"), but don't include missing.h.
• Change "missing.h" (but don't delete it).
• An incremental build now has to rebuild the object file, even though nothing will have changed.

However, it's fixing an actual bug, so it makes sense to me to be more conservative.

[dexonsmith]

In D30882#1075589, @dexonsmith wrote:

In D30882#1075576, @pete wrote:

Would it be ok to turn this on by default, without a flag, only in the case of the path actually existing, and only the found path being the one we add to the .d?

I think that pessimizes some incremental builds:

• You have a __has_include("missing.h"), but don't include missing.h.
• Change "missing.h" (but don't delete it).
• An incremental build now has to rebuild the object file, even though nothing will have changed.

However, it's fixing an actual bug, so it makes sense to me to be more conservative.

To be clear, I meant "yes" by that!

[rsmith]

include/clang/Lex/PPCallbacks.h

263 ↗

(On Diff #91520)

This callback seems pretty unhelpful in the case where lookup of the file failed (you just get a source location). Could we pass in the FileName and IsAngled flag too (like we do for InclusionDirective)? I think that's what (eg) a callback tracking anti-dependencies (for a more sophisticated build system that can handle them) would want.

lib/Frontend/DependencyFile.cpp

325 ↗

(On Diff #91520)

Have you thought about whether we should add a dependency even for a missing file under -MG (AddMissingHeaderDeps) mode? I think it's probably better to not do so (ie, the behavior in this patch), but it seems worth considering.

[vsapsai]

Taking over the change, I'll address the reviewers' comments.

[vsapsai]

• Improve tests, fix -MMD, address comments.

[vsapsai]

include/clang/Lex/PPCallbacks.h

263 ↗

(On Diff #91520)

Added. Also added FileType so we can ignore system headers for -MMD. And tweaked doxygen comment to be more consistent with other callbacks.

test/Frontend/dependency-gen-has-include.c

11–15 ↗

(On Diff #91520)

I've added a test with system headers. Overall, my testing strategy was to test all cases for the following dimensions:

• file exists/file doesn't exist;
• file is user/system header;
• __has_include/__has_include_next is used.

But I don't test the full matrix as I don't think it provides enough value.

17 ↗

(On Diff #91520)

Added.

[vsapsai]

lib/Frontend/DependencyFile.cpp

325 ↗

(On Diff #91520)

Do you know how Make uses these missing files? Or maybe where I can find more. The only documentation I found says

This feature is used in automatic updating of makefiles.

Which is not particularly illuminating.

Currently I prefer not to include not found __has_include files because they aren't really missing, it's OK if they aren't there and nothing has to be done to fix that. But I'd like to confirm if my understanding aligns with Make behaviour.

[rsmith]

lib/Frontend/DependencyFile.cpp

325 ↗

(On Diff #91520)

I believe the setup is this:

• your project uses generated headers
• in order to do the first compilation, you need to know which source files might include those generated headers

So you use -M -MG to build your make file dependencies as a separate step before your first "real" compilation, and the -MG flag causes dependencies on the not-yet-existing generated files to be emitted by the compiler.

Given that the purpose of -MG (as I understand it) is to cope with generated files, and that using __has_include to find a generated file would be peculiar (it's much more likely that the named file simply doesn't exist and shouldn't have a dependency generated for it), I think it's better to omit dependencies for nonexistent files for __has_include even under -MG.

[rsmith]

clang/lib/Frontend/DependencyFile.cpp

340

Should we really be using the (arbitrary) name of the file from the FileEntry rather than the name as written? It looks like the corresponding code for InclusionDirective uses the name as written instead.

[vsapsai]

clang/lib/Frontend/DependencyFile.cpp

300–304

This was the role model.

340

InclusionDirective covers only cases when file is not found. Existing files are handled in FileChanged. Advantage of using File->getName() is that for input with absolute paths file name will be absolute too.

[rsmith]

clang/lib/Frontend/DependencyFile.cpp

340

Ah right, that makes sense.

[vsapsai]

@dexonsmith, does my change address your concerns?

[dexonsmith]

LGTM too.

clang/include/clang/Lex/PPCallbacks.h

public:
   }
 
   /// Hook called whenever the 'defined' operator is seen.
   /// \param MD The MacroDirective if the name was a macro, null otherwise.
   virtual void Defined(const Token &MacroNameTok, const MacroDefinition &MD,
                        SourceRange Range) {
   }
 
+  /// Hook called when a '__has_include' or '__has_include_next' directive is
+  /// read.
+  virtual void HasInclude(SourceLocation Loc, StringRef FileName, bool IsAngled,
+                          const FileEntry *File,
+                          SrcMgr::CharacteristicKind FileType) {}
+
   /// Hook called when a source range is skipped.
   /// \param Range The SourceRange that was skipped. The range begins at the
   /// \#if/\#else directive and ends after the \#endif/\#else directive.
   /// \param EndifLoc The end location of the 'endif' token, which may precede
   /// the range skipped by the directive (e.g excluding comments after an
   /// 'endif').
   virtual void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) {
   }
public:
   }
 
   void PragmaDiagnostic(SourceLocation Loc, StringRef Namespace,
                         diag::Severity mapping, StringRef Str) override {
     First->PragmaDiagnostic(Loc, Namespace, mapping, Str);
     Second->PragmaDiagnostic(Loc, Namespace, mapping, Str);
   }
 
+  void HasInclude(SourceLocation Loc, StringRef FileName, bool IsAngled,
+                  const FileEntry *File,
+                  SrcMgr::CharacteristicKind FileType) override {
+    First->HasInclude(Loc, FileName, IsAngled, File, FileType);
+    Second->HasInclude(Loc, FileName, IsAngled, File, FileType);
+  }
+
   void PragmaOpenCLExtension(SourceLocation NameLoc, const IdentifierInfo *Name,
                              SourceLocation StateLoc, unsigned State) override {
     First->PragmaOpenCLExtension(NameLoc, Name, StateLoc, State);
     Second->PragmaOpenCLExtension(NameLoc, Name, StateLoc, State);
   }
 
   void PragmaWarning(SourceLocation Loc, StringRef WarningSpec,
                      ArrayRef<int> Ids) override {

clang/lib/Frontend/DependencyFile.cpp

public:
 
   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 HasInclude(SourceLocation Loc, StringRef SpelledFilename, bool IsAngled,
+                  const FileEntry *File,
+                  SrcMgr::CharacteristicKind FileType) override;
+
   void EndOfMainFile() override {
     OutputDependencyFile();
   }
 
   bool AddFilename(StringRef Filename);
   bool includeSystemHeaders() const { return IncludeSystemHeaders; }
   bool includeModuleFiles() const { return IncludeModuleFiles; }
 };
void DFGImpl::FileChanged(SourceLocation Loc,
   // file entry for a source location to find out what is depended on.
   // We do not want #line markers to affect dependency generation!
   SourceManager &SM = PP->getSourceManager();
 
   const FileEntry *FE =
     SM.getFileEntryForID(SM.getFileID(SM.getExpansionLoc(Loc)));
   if (!FE) return;
 
   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;
void DFGImpl::InclusionDirective(SourceLocation HashLoc,
   if (!File) {
     if (AddMissingHeaderDeps)
       AddFilename(FileName);
     else
       SeenMissingHeader = true;
   }
 }
 
+void DFGImpl::HasInclude(SourceLocation Loc, StringRef SpelledFilename,
+                         bool IsAngled, const FileEntry *File,
+                         SrcMgr::CharacteristicKind FileType) {
+  if (!File)
+    return;
+  StringRef Filename = File->getName();
+  if (!FileMatchesDepCriteria(Filename.data(), FileType))
+    return;
+  AddFilename(llvm::sys::path::remove_leading_dotslash(Filename));
+}
+
 bool DFGImpl::AddFilename(StringRef Filename) {
   if (FilesSet.insert(Filename).second) {
     Files.push_back(Filename);
     return true;
   }
   return false;
 }
 

clang/lib/Lex/PPMacroExpansion.cpp

 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/ObjCRuntime.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/CodeCompletionHandler.h"
 #include "clang/Lex/DirectoryLookup.h"
 #include "clang/Lex/ExternalPreprocessorSource.h"
+#include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/LexDiagnostic.h"
 #include "clang/Lex/MacroArgs.h"
 #include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorLexer.h"
 #include "clang/Lex/PTHLexer.h"
 #include "clang/Lex/Token.h"
 #include "llvm/ADT/ArrayRef.h"
if (Filename.empty())
     return false;
 
   // Search include directories.
   const DirectoryLookup *CurDir;
   const FileEntry *File =
       PP.LookupFile(FilenameLoc, Filename, isAngled, LookupFrom, LookupFromFile,
                     CurDir, nullptr, nullptr, nullptr, nullptr);
 
+  if (PPCallbacks *Callbacks = PP.getPPCallbacks()) {
+    SrcMgr::CharacteristicKind FileType = SrcMgr::C_User;
+    if (File)
+      FileType = PP.getHeaderSearchInfo().getFileDirFlavor(File);
+    Callbacks->HasInclude(FilenameLoc, Filename, isAngled, File, FileType);
+  }
+
   // Get the result value.  A result of true means the file exists.
   return File != nullptr;
 }
 
 /// EvaluateHasInclude - Process a '__has_include("path")' expression.
 /// Returns true if successful.
 static bool EvaluateHasInclude(Token &Tok, IdentifierInfo *II,
                                Preprocessor &PP) {

clang/test/Frontend/dependency-gen-has-include.c

+// REQUIRES: shell
+
+// Basic test
+// RUN: rm -rf %t.dir
+// RUN: mkdir %t.dir
+// RUN: mkdir %t.dir/a
+// 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: mkdir %t.dir/system
+// RUN: echo "#define SYSTEM_HEADER" > %t.dir/system/system-header.h
+// RUN: mkdir %t.dir/next-a
+// RUN: echo "#if __has_include_next(<next-header.h>)" > %t.dir/next-a/next-header.h
+// RUN: echo "#endif" >> %t.dir/next-a/next-header.h
+// RUN: mkdir %t.dir/next-b
+// RUN: echo "#define NEXT_HEADER" > %t.dir/next-b/next-header.h
+
+// RUN: %clang -MD -MF %t.dir/file.deps %s -fsyntax-only -I %t.dir -isystem %t.dir/system -I %t.dir/next-a -I %t.dir/next-b
+// RUN: FileCheck -input-file=%t.dir/file.deps %s
+// CHECK: dependency-gen-has-include.o
+// CHECK: dependency-gen-has-include.c
+// CHECK: a/header.h
+// CHECK-NOT: missing/file.h
+// CHECK: system/system-header.h
+// CHECK: next-a/next-header.h
+// CHECK: next-b/next-header.h
+
+// Verify that we ignore system headers in user-only headers mode.
+// RUN: %clang -MMD -MF %t.dir/user-headers.deps %s -fsyntax-only -I %t.dir -isystem %t.dir/system -I %t.dir/next-a -I %t.dir/next-b
+// RUN: FileCheck -input-file=%t.dir/user-headers.deps --check-prefix CHECK-USER-HEADER %s
+// CHECK-USER-HEADER-NOT: system/system-header.h
+
+#if __has_include("a/header.h")
+#endif
+#if __has_include("missing/file.h")
+#endif
+#if __has_include(<system-header.h>)
+#endif
+
+#include <next-header.h>