From 671538e8a51eab5bd65a1f9f327ba7f44f84e486 Mon Sep 17 00:00:00 2001 From: Richard Trieu Date: Wed, 12 Jun 2013 21:20:57 +0000 Subject: [PATCH] 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. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@183867 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticLexKinds.td | 7 ++- include/clang/Lex/HeaderSearch.h | 5 ++ include/clang/Lex/MultipleIncludeOpt.h | 56 ++++++++++++++++++- include/clang/Lex/Preprocessor.h | 2 +- lib/Lex/PPDirectives.cpp | 18 ++++-- lib/Lex/PPLexerChange.cpp | 23 +++++++- test/Lexer/Inputs/bad-header-guard.h | 4 ++ test/Lexer/Inputs/different-define.h | 4 ++ test/Lexer/Inputs/good-header-guard.h | 4 ++ test/Lexer/Inputs/multiple.h | 4 ++ test/Lexer/Inputs/no-define.h | 3 + test/Lexer/Inputs/out-of-order-define.h | 7 +++ .../Inputs/tokens-between-ifndef-and-define.h | 7 +++ test/Lexer/header.cpp | 33 +++++++++++ 14 files changed, 168 insertions(+), 9 deletions(-) create mode 100644 test/Lexer/Inputs/bad-header-guard.h create mode 100644 test/Lexer/Inputs/different-define.h create mode 100644 test/Lexer/Inputs/good-header-guard.h create mode 100644 test/Lexer/Inputs/multiple.h create mode 100644 test/Lexer/Inputs/no-define.h create mode 100644 test/Lexer/Inputs/out-of-order-define.h create mode 100644 test/Lexer/Inputs/tokens-between-ifndef-and-define.h create mode 100644 test/Lexer/header.cpp diff --git a/include/clang/Basic/DiagnosticLexKinds.td b/include/clang/Basic/DiagnosticLexKinds.td index 6d4008c5e7..49acbd5a31 100644 --- a/include/clang/Basic/DiagnosticLexKinds.td +++ b/include/clang/Basic/DiagnosticLexKinds.td @@ -585,5 +585,10 @@ def warn_forgotten_module_header : Warning< InGroup, 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>; +def note_header_guard : Note< + "%0 is defined here; did you mean %1?">; } diff --git a/include/clang/Lex/HeaderSearch.h b/include/clang/Lex/HeaderSearch.h index 498de09496..838df6831a 100644 --- a/include/clang/Lex/HeaderSearch.h +++ b/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. diff --git a/include/clang/Lex/MultipleIncludeOpt.h b/include/clang/Lex/MultipleIncludeOpt.h index a2a5a77c73..b532bf82f0 100644 --- a/include/clang/Lex/MultipleIncludeOpt.h +++ b/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 diff --git a/include/clang/Lex/Preprocessor.h b/include/clang/Lex/Preprocessor.h index 082f4e4cde..e993ce762f 100644 --- a/include/clang/Lex/Preprocessor.h +++ b/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. diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp index 947011d707..0cfdac92d2 100644 --- a/lib/Lex/PPDirectives.cpp +++ b/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(); } diff --git a/lib/Lex/PPLexerChange.cpp b/lib/Lex/PPLexerChange.cpp index 6437336246..e521032bca 100644 --- a/lib/Lex/PPLexerChange.cpp +++ b/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()); + } + } + } } } diff --git a/test/Lexer/Inputs/bad-header-guard.h b/test/Lexer/Inputs/bad-header-guard.h new file mode 100644 index 0000000000..601da09327 --- /dev/null +++ b/test/Lexer/Inputs/bad-header-guard.h @@ -0,0 +1,4 @@ +#ifndef bad_header_guard +#define bad_guard + +#endif diff --git a/test/Lexer/Inputs/different-define.h b/test/Lexer/Inputs/different-define.h new file mode 100644 index 0000000000..f23454b7c8 --- /dev/null +++ b/test/Lexer/Inputs/different-define.h @@ -0,0 +1,4 @@ +#ifndef different_define +#define FOO 5 + +#endif diff --git a/test/Lexer/Inputs/good-header-guard.h b/test/Lexer/Inputs/good-header-guard.h new file mode 100644 index 0000000000..664a479d39 --- /dev/null +++ b/test/Lexer/Inputs/good-header-guard.h @@ -0,0 +1,4 @@ +#ifndef good_header_guard +#define good_header_guard + +#endif diff --git a/test/Lexer/Inputs/multiple.h b/test/Lexer/Inputs/multiple.h new file mode 100644 index 0000000000..5dfb327a0b --- /dev/null +++ b/test/Lexer/Inputs/multiple.h @@ -0,0 +1,4 @@ +#ifndef multiple +#define multi + +#endif diff --git a/test/Lexer/Inputs/no-define.h b/test/Lexer/Inputs/no-define.h new file mode 100644 index 0000000000..591a66b30c --- /dev/null +++ b/test/Lexer/Inputs/no-define.h @@ -0,0 +1,3 @@ +#ifndef no_define + +#endif diff --git a/test/Lexer/Inputs/out-of-order-define.h b/test/Lexer/Inputs/out-of-order-define.h new file mode 100644 index 0000000000..d38e93f29b --- /dev/null +++ b/test/Lexer/Inputs/out-of-order-define.h @@ -0,0 +1,7 @@ +#ifndef out_of_order + +#define something_else + +#define out_of_order + +#endif diff --git a/test/Lexer/Inputs/tokens-between-ifndef-and-define.h b/test/Lexer/Inputs/tokens-between-ifndef-and-define.h new file mode 100644 index 0000000000..ce8d770506 --- /dev/null +++ b/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 diff --git a/test/Lexer/header.cpp b/test/Lexer/header.cpp new file mode 100644 index 0000000000..278ff2e432 --- /dev/null +++ b/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. -- 2.40.0