[Frontend] Don't output skipped includes from predefines

[smeenai]

Summary

-H displays a tree of included header files, but that tree is supposed
to omit two categories of header files:

1. Any header files pulled in via -include, which the code refers to as the "predefines".
2. Any header files whose inclusion was skipped because they'd already been included (assuming header guards or #pragma once).

-fshow-skipped-includes was intended to make -H display the second
category of files. It wasn't checking for the first category, however,
so you could end up with only the middle of the -include hierarchy
displayed, e.g. the added test would previously output:

... /data/users/smeenai/llvm-project/clang/test/Frontend/Inputs/test2.h
. /data/users/smeenai/llvm-project/clang/test/Frontend/Inputs/test.h

This diff adds a check to prevent that and correctly omit headers from
-include even when -fshow-skipped-includes is passed. While I'm
here, add tests for the interaction between -fshow-skipped-includes
and -sys-header-deps as well.

[hans]

-H is supposed to skip outputting headers from -include command line
arguments, but -fshow-skipped-includes was outputting any skipped
includes encountered via -include.

I was thrown off by the "-H is supposed to skip ... but -fshow-skipped-includes was outputting ..."

I suppose the point is that we do want -fshow-skipped-includes to show skipped includes, but not the ones from -include or system headers (unless -sys-header-deps)?

lgtm if I got that right.

[smeenai]

In D153175#4431923, @hans wrote:

-H is supposed to skip outputting headers from -include command line
arguments, but -fshow-skipped-includes was outputting any skipped
includes encountered via -include.

I was thrown off by the "-H is supposed to skip ... but -fshow-skipped-includes was outputting ..."

I suppose the point is that we do want -fshow-skipped-includes to show skipped includes, but not the ones from -include or system headers (unless -sys-header-deps)?

lgtm if I got that right.

Yup, my description was kinda terrible :D Lemme try to clarify it.

clang/lib/Frontend/HeaderIncludeGen.cpp

public:
   }
 
   void FileChanged(SourceLocation Loc, FileChangeReason Reason,
                    SrcMgr::CharacteristicKind FileType,
                    FileID PrevFID) override;
 
   void FileSkipped(const FileEntryRef &SkippedFile, const Token &FilenameTok,
                    SrcMgr::CharacteristicKind FileType) override;
+
+private:
+  bool ShouldShowHeader(SrcMgr::CharacteristicKind HeaderType) {
+    if (!DepOpts.IncludeSystemHeaders && isSystem(HeaderType))
+      return false;
+
+    // Show the current header if we are (a) past the predefines, or (b) showing
+    // all headers and in the predefines at a depth past the initial file and
+    // command line buffers.
+    return (HasProcessedPredefines ||
+            (ShowAllHeaders && CurrentIncludeDepth > 2));
+  }
 };
 
 /// A callback for emitting header usage information to a file in JSON. Each
 /// line in the file is a JSON object that includes the source file name and
 /// the list of headers directly or indirectly included from it. For example:
 ///
 /// {"source":"/tmp/foo.c",
 ///  "includes":["/usr/include/stdio.h", "/usr/include/stdlib.h"]}
if (CurrentIncludeDepth == 1 && !HasProcessedPredefines) {
       if (!DepOpts.ShowIncludesPretendHeader.empty()) {
         PrintHeaderInfo(OutputFile, DepOpts.ShowIncludesPretendHeader,
                         ShowDepth, 2, MSStyle);
       }
       HasProcessedPredefines = true;
     }
 
     return;
-  } else
+  } else {
+    return;
+  }
+
+  if (!ShouldShowHeader(NewFileType))
     return;
 
-  // Show the header if we are (a) past the predefines, or (b) showing all
-  // headers and in the predefines at a depth past the initial file and command
-  // line buffers.
-  bool ShowHeader = (HasProcessedPredefines ||
-                     (ShowAllHeaders && CurrentIncludeDepth > 2));
   unsigned IncludeDepth = CurrentIncludeDepth;
   if (!HasProcessedPredefines)
     --IncludeDepth; // Ignore indent from <built-in>.
   else if (!DepOpts.ShowIncludesPretendHeader.empty())
     ++IncludeDepth; // Pretend inclusion by ShowIncludesPretendHeader.
 
-  if (!DepOpts.IncludeSystemHeaders && isSystem(NewFileType))
-    ShowHeader = false;
-
-  // Dump the header include information we are past the predefines buffer or
-  // are showing all headers and this isn't the magic implicit <command line>
-  // header.
   // FIXME: Identify headers in a more robust way than comparing their name to
   // "<command line>" and "<built-in>" in a bunch of places.
-  if (ShowHeader && Reason == PPCallbacks::EnterFile &&
+  if (Reason == PPCallbacks::EnterFile &&
       UserLoc.getFilename() != StringRef("<command line>")) {
     PrintHeaderInfo(OutputFile, UserLoc.getFilename(), ShowDepth, IncludeDepth,
                     MSStyle);
   }
 }
 
 void HeaderIncludesCallback::FileSkipped(const FileEntryRef &SkippedFile, const
                                          Token &FilenameTok,
                                          SrcMgr::CharacteristicKind FileType) {
   if (!DepOpts.ShowSkippedHeaderIncludes)
     return;
 
-  if (!DepOpts.IncludeSystemHeaders && isSystem(FileType))
+  if (!ShouldShowHeader(FileType))
     return;
 
   PrintHeaderInfo(OutputFile, SkippedFile.getName(), ShowDepth,
                   CurrentIncludeDepth + 1, MSStyle);
 }
 
 void HeaderIncludesJSONCallback::EndOfMainFile() {
   const FileEntry *FE = SM.getFileEntryForID(SM.getMainFileID());

clang/test/Frontend/print-header-includes.c

 // RUN:     -E -H -fshow-skipped-includes -o /dev/null %s 2> %t.stderr
 // RUN: FileCheck --check-prefix=SKIPPED < %t.stderr %s
 
 // SKIPPED-NOT: . {{.*noline.h}}
 // SKIPPED: . {{.*test.h}}
 // SKIPPED: .. {{.*test2.h}}
 // SKIPPED: .. {{.*test2.h}}
 
+// RUN: %clang_cc1 -isystem %S -isystem %S/Inputs/SystemHeaderPrefix \
+// RUN:     -E -H -fshow-skipped-includes -sys-header-deps -o /dev/null %s 2> %t.stderr
+// RUN: FileCheck --check-prefix=SKIPPED-SYS < %t.stderr %s
+
+// SKIPPED-SYS: . {{.*noline.h}}
+// SKIPPED-SYS: . {{.*test.h}}
+// SKIPPED-SYS: .. {{.*test2.h}}
+// SKIPPED-SYS: .. {{.*test2.h}}
+
+// RUN: %clang_cc1 -I%S -isystem %S/Inputs/SystemHeaderPrefix -include Inputs/test.h \
+// RUN:     -E -H -fshow-skipped-includes -o /dev/null %s 2> %t.stderr
+// RUN: FileCheck --check-prefix=SKIPPED-PREDEFINES < %t.stderr %s
+
+// The skipped include of test2.h from the -include test.h shouldn't be printed.
+// SKIPPED-PREDEFINES-NOT: {{.*test2.h}}
+// SKIPPED-PREDEFINES:     . {{.*test.h}}
+
+// RUN: %clang_cc1 -isystem %S -isystem %S/Inputs/SystemHeaderPrefix \
+// RUN:     -E -H -fshow-skipped-includes -o /dev/null %s 2> %t.stderr
+// RUN: FileCheck --check-prefix=SKIPPED-NO-SYS --allow-empty < %t.stderr %s
+
+// SKIPPED-NO-SYS-NOT: .
+
 // RUN: %clang_cc1 -I%S -include Inputs/test3.h -isystem %S/Inputs/SystemHeaderPrefix \
 // RUN:     -E -H -sys-header-deps -o /dev/null %s 2> %t.stderr
 // RUN: FileCheck --check-prefix SYSHEADERS < %t.stderr %s
 
 // SYSHEADERS-NOT: test3.h
 // SYSHEADERS: . {{.*noline.h}}
 // SYSHEADERS: . {{.*test.h}}
 // SYSHEADERS: .. {{.*test2.h}}

 // RUN:     FileCheck --strict-whitespace --check-prefix=MS-IGNORELIST %s
 // MS-IGNORELIST: Note: including file: {{[^ ]*\.ignorelist}}
 // MS-IGNORELIST-NOT: Note: including file: {{[^ ]*noline.h}}
 // MS-IGNORELIST: Note: including file: {{[^ ]*test.h}}
 // MS-IGNORELIST: Note: including file:  {{[^ ]*test2.h}}
 // MS-IGNORELIST-NOT: Note
 
 #include <noline.h>
-#include "Inputs/test.h"
+#include <Inputs/test.h>