Introducing -Wheader-guard, a warning that checks header guards actually work

properly.  This warning checks that the #ifndef and #define directives at
the beginning of a header refer to the same macro name.  Includes a fix-it
hint to correct the header guard.

llvm-svn: 183867

clang/include/clang/Basic/DiagnosticLexKinds.td

@@ -585,5 +585,10 @@ def warn_forgotten_module_header : Warning<
   InGroup<IncompleteModule>, DefaultIgnore;
 def err_expected_id_building_module : Error<
   "expected a module name in '__building_module' expression">;
-  
+
+def warn_header_guard : Warning<
+  "%0 is used as a header guard here, followed by #define of a different macro">,
+  InGroup<DiagGroup<"header-guard">>;
+def note_header_guard : Note<
+  "%0 is defined here; did you mean %1?">;  
 }

clang/include/clang/Lex/HeaderSearch.h

@@ -425,6 +425,11 @@ public:
     getFileInfo(File).ControllingMacro = ControllingMacro;
   }
 
+  /// \brief Return true if this is the first time encountering this header.
+  bool FirstTimeLexingFile(const FileEntry *File) {
+    return getFileInfo(File).NumIncludes == 1;
+  }
+
   /// \brief Determine whether this file is intended to be safe from
   /// multiple inclusions, e.g., it has \#pragma once or a controlling
   /// macro.

clang/include/clang/Lex/MultipleIncludeOpt.h

@@ -15,6 +15,8 @@
 #ifndef LLVM_CLANG_MULTIPLEINCLUDEOPT_H
 #define LLVM_CLANG_MULTIPLEINCLUDEOPT_H
 
+#include "clang/Basic/SourceLocation.h"
+
 namespace clang {
 class IdentifierInfo;
 
@@ -32,6 +34,11 @@ class MultipleIncludeOpt {
   /// \#endif can be easily detected.
   bool ReadAnyTokens;
 
+  /// ImmediatelyAfterTopLevelIfndef - This is true when the only tokens
+  /// processed in the file so far is an #ifndef and an identifier.  Used in
+  /// the detection of header guards in a file.
+  bool ImmediatelyAfterTopLevelIfndef;
+
   /// ReadAnyTokens - This is set to false when a file is first opened and true
   /// any time a token is returned to the client or a (non-multiple-include)
   /// directive is parsed.  When the final #endif is parsed this is reset back
@@ -42,11 +49,36 @@ class MultipleIncludeOpt {
   /// TheMacro - The controlling macro for a file, if valid.
   ///
   const IdentifierInfo *TheMacro;
+
+  /// DefinedMacro - The macro defined right after TheMacro, if any.
+  const IdentifierInfo *DefinedMacro;
+
+  SourceLocation MacroLoc;
+  SourceLocation DefinedLoc;
 public:
   MultipleIncludeOpt() {
     ReadAnyTokens = false;
+    ImmediatelyAfterTopLevelIfndef = false;
     DidMacroExpansion = false;
     TheMacro = 0;
+    DefinedMacro = 0;
+  }
+
+  SourceLocation GetMacroLocation() const {
+    return MacroLoc;
+  }
+
+  SourceLocation GetDefinedLocation() const {
+    return DefinedLoc;
+  }
+
+  void resetImmediatelyAfterTopLevelIfndef() {
+    ImmediatelyAfterTopLevelIfndef = false;
+  }
+
+  void SetDefinedMacro(IdentifierInfo *M, SourceLocation Loc) {
+    DefinedMacro = M;
+    DefinedLoc = Loc;
   }
 
   /// Invalidate - Permanently mark this file as not being suitable for the
@@ -55,6 +87,8 @@ public:
     // If we have read tokens but have no controlling macro, the state-machine
     // below can never "accept".
     ReadAnyTokens = true;
+    ImmediatelyAfterTopLevelIfndef = false;
+    DefinedMacro = 0;
     TheMacro = 0;
   }
 
@@ -63,8 +97,17 @@ public:
   /// the "ifndef x" would count as reading tokens.
   bool getHasReadAnyTokensVal() const { return ReadAnyTokens; }
 
+  /// getImmediatelyAfterTopLevelIfndef - returns true if the last directive
+  /// was an #ifndef at the beginning of the file.
+  bool getImmediatelyAfterTopLevelIfndef() const {
+    return ImmediatelyAfterTopLevelIfndef;
+  }
+
   // If a token is read, remember that we have seen a side-effect in this file.
-  void ReadToken() { ReadAnyTokens = true; }
+  void ReadToken() {
+    ReadAnyTokens = true;
+    ImmediatelyAfterTopLevelIfndef = false;
+  }
 
   /// ExpandedMacro - When a macro is expanded with this lexer as the current
   /// buffer, this method is called to disable the MIOpt if needed.
@@ -77,7 +120,7 @@ public:
   /// ensures that this is only called if there are no tokens read before the
   /// \#ifndef.  The caller is required to do this, because reading the \#if
   /// line obviously reads in in tokens.
-  void EnterTopLevelIFNDEF(const IdentifierInfo *M) {
+  void EnterTopLevelIfndef(const IdentifierInfo *M, SourceLocation Loc) {
     // If the macro is already set, this is after the top-level #endif.
     if (TheMacro)
       return Invalidate();
@@ -91,7 +134,9 @@ public:
 
     // Remember that we're in the #if and that we have the macro.
     ReadAnyTokens = true;
+    ImmediatelyAfterTopLevelIfndef = true;
     TheMacro = M;
+    MacroLoc = Loc;
   }
 
   /// \brief Invoked when a top level conditional (except \#ifndef) is found.
@@ -111,6 +156,7 @@ public:
     // At this point, we haven't "read any tokens" but we do have a controlling
     // macro.
     ReadAnyTokens = false;
+    ImmediatelyAfterTopLevelIfndef = false;
   }
 
   /// \brief Once the entire file has been lexed, if there is a controlling
@@ -122,6 +168,12 @@ public:
       return TheMacro;
     return 0;
   }
+
+  /// \brief If the ControllingMacro is followed by a macro definition, return
+  /// the macro that was defined.
+  const IdentifierInfo *GetDefinedMacro() const {
+    return DefinedMacro;
+  }
 };
 
 }  // end namespace clang

clang/include/clang/Lex/Preprocessor.h

@@ -1444,7 +1444,7 @@ private:
   void HandleMicrosoftImportDirective(Token &Tok);
 
   // Macro handling.
-  void HandleDefineDirective(Token &Tok);
+  void HandleDefineDirective(Token &Tok, bool ImmediatelyAfterTopLevelIfndef);
   void HandleUndefDirective(Token &Tok);
 
   // Conditional Inclusion.

clang/lib/Lex/PPDirectives.cpp

@@ -626,6 +626,10 @@ void Preprocessor::HandleDirective(Token &Result) {
   CurPPLexer->ParsingPreprocessorDirective = true;
   if (CurLexer) CurLexer->SetKeepWhitespaceMode(false);
 
+  bool ImmediatelyAfterTopLevelIfndef =
+      CurPPLexer->MIOpt.getImmediatelyAfterTopLevelIfndef();
+  CurPPLexer->MIOpt.resetImmediatelyAfterTopLevelIfndef();
+
   ++NumDirectives;
 
   // We are about to read a token.  For the multiple-include optimization FA to
@@ -713,7 +717,7 @@ void Preprocessor::HandleDirective(Token &Result) {
 
     // C99 6.10.3 - Macro Replacement.
     case tok::pp_define:
-      return HandleDefineDirective(Result);
+      return HandleDefineDirective(Result, ImmediatelyAfterTopLevelIfndef);
     case tok::pp_undef:
       return HandleUndefDirective(Result);
 
@@ -1760,7 +1764,8 @@ bool Preprocessor::ReadMacroDefinitionArgList(MacroInfo *MI, Token &Tok) {
 
 /// HandleDefineDirective - Implements \#define.  This consumes the entire macro
 /// line then lets the caller lex the next real token.
-void Preprocessor::HandleDefineDirective(Token &DefineTok) {
+void Preprocessor::HandleDefineDirective(Token &DefineTok,
+                                         bool ImmediatelyAfterHeaderGuard) {
   ++NumDefined;
 
   Token MacroNameTok;
@@ -1786,6 +1791,11 @@ void Preprocessor::HandleDefineDirective(Token &DefineTok) {
   // marking each of the identifiers as being used as macro arguments.  Also,
   // check other constraints on the first token of the macro body.
   if (Tok.is(tok::eod)) {
+    if (ImmediatelyAfterHeaderGuard) {
+      // Save this macro information since it may part of a header guard.
+      CurPPLexer->MIOpt.SetDefinedMacro(MacroNameTok.getIdentifierInfo(),
+                                        MacroNameTok.getLocation());
+    }
     // If there is no body to this macro, we have no special handling here.
   } else if (Tok.hasLeadingSpace()) {
     // This is a normal token with leading space.  Clear the leading space
@@ -2076,7 +2086,7 @@ void Preprocessor::HandleIfdefDirective(Token &Result, bool isIfndef,
     // handle.
     if (!ReadAnyTokensBeforeDirective && MI == 0) {
       assert(isIfndef && "#ifdef shouldn't reach here");
-      CurPPLexer->MIOpt.EnterTopLevelIFNDEF(MII);
+      CurPPLexer->MIOpt.EnterTopLevelIfndef(MII, MacroNameTok.getLocation());
     } else
       CurPPLexer->MIOpt.EnterTopLevelConditional();
   }
@@ -2122,7 +2132,7 @@ void Preprocessor::HandleIfDirective(Token &IfToken,
   // directive seen, handle it for the multiple-include optimization.
   if (CurPPLexer->getConditionalStackDepth() == 0) {
     if (!ReadAnyTokensBeforeDirective && IfNDefMacro && ConditionalTrue)
-      CurPPLexer->MIOpt.EnterTopLevelIFNDEF(IfNDefMacro);
+      CurPPLexer->MIOpt.EnterTopLevelIfndef(IfNDefMacro, ConditionalBegin);
     else
       CurPPLexer->MIOpt.EnterTopLevelConditional();
   }

clang/lib/Lex/PPLexerChange.cpp

@@ -244,8 +244,29 @@ bool Preprocessor::HandleEndOfFile(Token &Result, bool isEndOfMacro) {
           CurPPLexer->MIOpt.GetControllingMacroAtEndOfFile()) {
       // Okay, this has a controlling macro, remember in HeaderFileInfo.
       if (const FileEntry *FE =
-            SourceMgr.getFileEntryForID(CurPPLexer->getFileID()))
+            SourceMgr.getFileEntryForID(CurPPLexer->getFileID())) {
         HeaderInfo.SetFileControllingMacro(FE, ControllingMacro);
+        if (const IdentifierInfo *DefinedMacro =
+              CurPPLexer->MIOpt.GetDefinedMacro()) {
+          if (!ControllingMacro->hasMacroDefinition() &&
+              DefinedMacro != ControllingMacro &&
+              HeaderInfo.FirstTimeLexingFile(FE)) {
+            // Emit a warning for a bad header guard.
+            Diag(CurPPLexer->MIOpt.GetMacroLocation(),
+                 diag::warn_header_guard)
+                << CurPPLexer->MIOpt.GetMacroLocation()
+                << ControllingMacro;
+            Diag(CurPPLexer->MIOpt.GetDefinedLocation(),
+                 diag::note_header_guard)
+                << CurPPLexer->MIOpt.GetDefinedLocation()
+                << DefinedMacro
+                << ControllingMacro
+                << FixItHint::CreateReplacement(
+                       CurPPLexer->MIOpt.GetDefinedLocation(),
+                       ControllingMacro->getName());
+          }
+        }
+      }
     }
   }
 

clang/test/Lexer/Inputs/bad-header-guard.h

@@ -0,0 +1,4 @@
+#ifndef bad_header_guard
+#define bad_guard
+
+#endif

clang/test/Lexer/Inputs/different-define.h

@@ -0,0 +1,4 @@
+#ifndef different_define
+#define FOO 5
+
+#endif

clang/test/Lexer/Inputs/good-header-guard.h

@@ -0,0 +1,4 @@
+#ifndef good_header_guard
+#define good_header_guard
+
+#endif

clang/test/Lexer/Inputs/multiple.h

@@ -0,0 +1,4 @@
+#ifndef multiple
+#define multi
+
+#endif

clang/test/Lexer/Inputs/no-define.h

@@ -0,0 +1,3 @@
+#ifndef no_define
+
+#endif

clang/test/Lexer/Inputs/out-of-order-define.h

@@ -0,0 +1,7 @@
+#ifndef out_of_order
+
+#define something_else
+
+#define out_of_order
+
+#endif

clang/test/Lexer/Inputs/tokens-between-ifndef-and-define.h

@@ -0,0 +1,7 @@
+#ifndef tokens_between
+
+const int pi = 3;
+
+#define pi_is_bad
+
+#endif

clang/test/Lexer/header.cpp

@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fsyntax-only -Wno-header-guard %s
+// RUN: %clang_cc1 -fsyntax-only -Wheader-guard %s 2>&1 | FileCheck %s
+
+#include "Inputs/good-header-guard.h"
+#include "Inputs/no-define.h"
+#include "Inputs/different-define.h"
+#include "Inputs/out-of-order-define.h"
+#include "Inputs/tokens-between-ifndef-and-define.h"
+
+#include "Inputs/bad-header-guard.h"
+// CHECK: In file included from {{.*}}header.cpp:{{[0-9]*}}:
+// CHECK: {{.*}}bad-header-guard.h:1:9: warning: 'bad_header_guard' is used as a header guard here, followed by #define of a different macro
+// CHECK: {{^}}#ifndef bad_header_guard
+// CHECK: {{^}}        ^~~~~~~~~~~~~~~~
+// CHECK: {{.*}}bad-header-guard.h:2:9: note: 'bad_guard' is defined here; did you mean 'bad_header_guard'?
+// CHECK: {{^}}#define bad_guard
+// CHECK: {{^}}        ^~~~~~~~~
+// CHECK: {{^}}        bad_header_guard
+
+#include "Inputs/multiple.h"
+#include "Inputs/multiple.h"
+#include "Inputs/multiple.h"
+#include "Inputs/multiple.h"
+// CHECK: In file included from {{.*}}header.cpp:{{[0-9]*}}:
+// CHECK: {{.*}}multiple.h:1:9: warning: 'multiple' is used as a header guard here, followed by #define of a different macro
+// CHECK: {{^}}#ifndef multiple
+// CHECK: {{^}}        ^~~~~~~~
+// CHECK: {{.*}}multiple.h:2:9: note: 'multi' is defined here; did you mean 'multiple'?
+// CHECK: {{^}}#define multi
+// CHECK: {{^}}        ^~~~~
+// CHECK: {{^}}        multiple
+
+// CHECK: 2 warnings generated.