Clang/Preprocessor: Not add headers of __has_include into DepColloctor

[wzssyqa]

When we preprocess the bellow code with
clang -E -MD -MF

#if __has_include(<limits.h>)
// DO NOTHING
#endif
#if 0 && __has_include(<limits.h>)
#include <limits.h>
#endif

It will list limits.h in the dependencies.

The same case with #__has_include_next

Reference: https://reviews.llvm.org/D70936
Ref-Commit: 3895305

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang

Author: YunQiang Su (wzssyqa)

Changes

When we preprocess the bellow code with
clang -E -MD -MF

#if __has_include(<limits.h>)
// DO NOTHING
#endif
#if 0 && __has_include(<limits.h>)
#include <limits.h>
#endif

It will list limits.h in the dependencies.

The same case with #__has_include_next

---

Full diff: https://github.com/llvm/llvm-project/pull/120673.diff

5 Files Affected:

• (modified) clang/include/clang/Lex/PPCallbacks.h (+4-2)
• (modified) clang/lib/Frontend/DependencyFile.cpp (+7-5)
• (modified) clang/lib/Lex/PPCallbacks.cpp (+7-4)
• (modified) clang/lib/Lex/PPMacroExpansion.cpp (+2-1)
• (added) clang/test/Preprocessor/dependencies-on-has-include.c (+23)

diff --git a/clang/include/clang/Lex/PPCallbacks.h b/clang/include/clang/Lex/PPCallbacks.h
index 46cc564086f1c5..8e03c2b9a76dbb 100644
--- a/clang/include/clang/Lex/PPCallbacks.h
+++ b/clang/include/clang/Lex/PPCallbacks.h
@@ -370,7 +370,8 @@ class PPCallbacks {
   /// read.
   virtual void HasInclude(SourceLocation Loc, StringRef FileName, bool IsAngled,
                           OptionalFileEntryRef File,
-                          SrcMgr::CharacteristicKind FileType);
+                          SrcMgr::CharacteristicKind FileType,
+                          bool AddToDepCollector = true);
 
   /// Hook called when a source range is skipped.
   /// \param Range The SourceRange that was skipped. The range begins at the
@@ -621,7 +622,8 @@ class PPChainedCallbacks : public PPCallbacks {
 
   void HasInclude(SourceLocation Loc, StringRef FileName, bool IsAngled,
                   OptionalFileEntryRef File,
-                  SrcMgr::CharacteristicKind FileType) override;
+                  SrcMgr::CharacteristicKind FileType,
+                  bool AddToDepCollector = true) override;
 
   void PragmaOpenCLExtension(SourceLocation NameLoc, const IdentifierInfo *Name,
                              SourceLocation StateLoc, unsigned State) override {
diff --git a/clang/lib/Frontend/DependencyFile.cpp b/clang/lib/Frontend/DependencyFile.cpp
index 528eae2c5283ea..931e29cd352211 100644
--- a/clang/lib/Frontend/DependencyFile.cpp
+++ b/clang/lib/Frontend/DependencyFile.cpp
@@ -104,15 +104,17 @@ struct DepCollectorPPCallbacks : public PPCallbacks {
 
   void HasInclude(SourceLocation Loc, StringRef SpelledFilename, bool IsAngled,
                   OptionalFileEntryRef File,
-                  SrcMgr::CharacteristicKind FileType) override {
+                  SrcMgr::CharacteristicKind FileType,
+                  bool AddToDepCollector = true) override {
     if (!File)
       return;
     StringRef Filename =
         llvm::sys::path::remove_leading_dotslash(File->getName());
-    DepCollector.maybeAddDependency(Filename, /*FromModule=*/false,
-                                    /*IsSystem=*/isSystem(FileType),
-                                    /*IsModuleFile=*/false,
-                                    /*IsMissing=*/false);
+    if (AddToDepCollector)
+      DepCollector.maybeAddDependency(Filename, /*FromModule=*/false,
+                                      /*IsSystem=*/isSystem(FileType),
+                                      /*IsModuleFile=*/false,
+                                      /*IsMissing=*/false);
   }
 
   void EndOfMainFile() override {
diff --git a/clang/lib/Lex/PPCallbacks.cpp b/clang/lib/Lex/PPCallbacks.cpp
index cf473aa4df6564..79bb64892f6de6 100644
--- a/clang/lib/Lex/PPCallbacks.cpp
+++ b/clang/lib/Lex/PPCallbacks.cpp
@@ -15,14 +15,17 @@ PPCallbacks::~PPCallbacks() = default;
 
 void PPCallbacks::HasInclude(SourceLocation Loc, StringRef FileName,
                              bool IsAngled, OptionalFileEntryRef File,
-                             SrcMgr::CharacteristicKind FileType) {}
+                             SrcMgr::CharacteristicKind FileType,
+                             bool AddToDepCollector) {}
 
 // Out of line key method.
 PPChainedCallbacks::~PPChainedCallbacks() = default;
 
 void PPChainedCallbacks::HasInclude(SourceLocation Loc, StringRef FileName,
                                     bool IsAngled, OptionalFileEntryRef File,
-                                    SrcMgr::CharacteristicKind FileType) {
-  First->HasInclude(Loc, FileName, IsAngled, File, FileType);
-  Second->HasInclude(Loc, FileName, IsAngled, File, FileType);
+                                    SrcMgr::CharacteristicKind FileType,
+                                    bool AddToDepCollector) {
+  First->HasInclude(Loc, FileName, IsAngled, File, FileType, AddToDepCollector);
+  Second->HasInclude(Loc, FileName, IsAngled, File, FileType,
+                     AddToDepCollector);
 }
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp b/clang/lib/Lex/PPMacroExpansion.cpp
index 347c13da0ad215..3429f08f45a2a5 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1256,7 +1256,8 @@ static bool EvaluateHasIncludeCommon(Token &Tok, IdentifierInfo *II,
     SrcMgr::CharacteristicKind FileType = SrcMgr::C_User;
     if (File)
       FileType = PP.getHeaderSearchInfo().getFileDirFlavor(*File);
-    Callbacks->HasInclude(FilenameLoc, Filename, isAngled, File, FileType);
+    Callbacks->HasInclude(FilenameLoc, Filename, isAngled, File, FileType,
+                          false);
   }
 
   // Get the result value.  A result of true means the file exists.
diff --git a/clang/test/Preprocessor/dependencies-on-has-include.c b/clang/test/Preprocessor/dependencies-on-has-include.c
new file mode 100644
index 00000000000000..b6efbb22e18477
--- /dev/null
+++ b/clang/test/Preprocessor/dependencies-on-has-include.c
@@ -0,0 +1,23 @@
+// Test -MF and -E flags with has_include
+
+#ifdef TEST_HAS_INCLUDE_NEXT
+#if __has_include_next(<limits.h>)
+// DO NOTHING
+#endif
+#endif
+
+#ifdef TEST_HAS_INCLUDE
+#if __has_include(<limits.h>)
+// DO NOTHING
+#endif
+#endif
+
+// RUN: %clang -DTEST_HAS_INCLUDE -E -MD -MF - %s \
+// RUN:    | FileCheck -check-prefix=TEST-HAS %s
+// TEST-HAS: dependencies-on-has-include.o:
+// TEST-HAS-NOT: limits.h
+
+// RUN: %clang -Wno-include-next-outside-header -DTEST_HAS_INCLUDE_NEXT -E -MD -MF - %s \
+// RUN:    | FileCheck -check-prefix=TEST-HAS-N %s
+// TEST-HAS-N: dependencies-on-has-include.o:
+// TEST-HAS-N-NOT: limits.h

[wzssyqa]

ping

[MaskRay]

I think skipping header1.h is undesired. header1.h is not inside #if 0 ... #endif . Whether the file is present affects the build.

[wzssyqa]

Do you mean that, for this case

#if __has_include(<limits.h>)
// DO NOTHING
#endif

limits.h should be listed in the dependencies?

[wzssyqa]

gcc doesn't, thus glibc depends on this behavior. I guess that we need to make it clear.

[Bigcheese]

It's intentional that Clang reports these as dependencies. What issue do you have where you think this is the wrong behavior?

More specifically, https://reviews.llvm.org/D30882 originally added the behavior. This references an internal Apple bug that occurred when you have code that checks for the presence of a header but does not include it. Not reporting that dependency can lead to failed builds or miscompiles in incremental builds.

[wzssyqa]

It's intentional that Clang reports these as dependencies. What issue do you have where you think this is the wrong behavior?

A test case of glibc emit error for this behavior:

• check-local-headers

More specifically, https://reviews.llvm.org/D30882 originally added the behavior. This references an internal Apple bug that occurred when you have code that checks for the presence of a header but does not include it. Not reporting that dependency can lead to failed builds or miscompiles in incremental builds.

So, maybe we need to add an OS-specific check?

[Bigcheese]

It's intentional that Clang reports these as dependencies. What issue do you have where you think this is the wrong behavior?

A test case of glibc emit error for this behavior:

• check-local-headers

More specifically, https://reviews.llvm.org/D30882 originally added the behavior. This references an internal Apple bug that occurred when you have code that checks for the presence of a header but does not include it. Not reporting that dependency can lead to failed builds or miscompiles in incremental builds.

So, maybe we need to add an OS-specific check?

There's nothing OS specific about the issue, it's just that we ran into it. It's a fundamental issue of how __has_include works. For example in a normal incremental build, if you remove the the header file that was only referenced by a __has_include, if it's not in the dependency list then nothing will be rebuilt. This is wrong, as it should be rebuilt. You can also run into cases where a separate source file that does actually conditionally include that header is rebuilt, and now you have out of sync object files that can lead to a broken program.

This also causes problems for tools like ccache or some types of distributed builds. They need to know about all inputs.

I believe that gcc is wrong to not report these as dependencies, and thus glibc's check-local-headers.sh script is wrong too.

I do think it's fine to restrict when we report the dependency some though. For example #if 0 && __has_include(...) is always false, and so the presence of the file doesn't matter. Same with a truly empty #if body. However I view these as a very minor incremental build optimization. No actual tool I'm aware of has issues with reporting a file that exists and you have access to as a dependency.

[wzssyqa]

See: #123912