Emit warning if define or undef reserved identifier or keyword.

[sepavloff]

Summary

This change implements warnings if macro name is identical to a keyword or
reserved identifier. The warnings are different depending on the "danger"
of the operation. Defining macro that replaces a keyword is on by default.
Other cases produce warning that is off by default but can be turned on
using option -Wreserved-id-macro.

This change fixes PR11488.

[rnk]

include/clang/Basic/DiagnosticLexKinds.td

299 ↗

(On Diff #15994)

Personally, I think it's confusing to have a warning group where some instances are on by default and others are off. We end up with a tri-state of default, on, off. Many users do things like clang -w -Werror -Wthing1 -Wthing2 -Wthing3 ... to explicitly use a curated set of warnings. It would be too bad if they couldn't use the on-by-default part of this warning due to lumping them into the same group.

Maybe we can split the group into -Wkeyword-macro and -Wreserved-id-macro? Does gcc have a flag name we should be using for compatibility?

lib/Basic/IdentifierTable.cpp

216 ↗

(On Diff #15994)

indentation is off

lib/Lex/PPDirectives.cpp

111 ↗

(On Diff #15994)

The LLVM naming conventions call for leading lower camel case function names, so this should be shouldWarnOnMacroDef.

117 ↗

(On Diff #15994)

indentation

117 ↗

(On Diff #15994)

We have a table lookup based isUppercase() for this. Factoring this check out as a helper gives us a good place to put this quote from the C++ standard.

133 ↗

(On Diff #15994)

s/Should/should/

143 ↗

(On Diff #15994)

Reuse this code

197 ↗

(On Diff #15994)

There's got to be a better way to identify builtin macros...

[sepavloff]

Addressed reviewer's notes.

[sepavloff]

Thank you for review!

include/clang/Basic/DiagnosticLexKinds.td

299 ↗

(On Diff #15994)

Implemented two groups, -Wkeyword-macro and -Wreserved-id-macro, the first is on by default.
Bug tracker of gcc has issue https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51437, which is a clone of clang PR11488. It is still in unconfirmed state. Experiments show that gcc does not emit warnings for described use.

lib/Basic/IdentifierTable.cpp

216 ↗

(On Diff #15994)

Fixed.

lib/Lex/PPDirectives.cpp

111 ↗

(On Diff #15994)

Ah, old naming. Fixed.

117 ↗

(On Diff #15994)

Introduced function isReservedId, which makes this check.

133 ↗

(On Diff #15994)

Fixed.

143 ↗

(On Diff #15994)

Done: calls isReservedId.

197 ↗

(On Diff #15994)

It seems they are identified by similar way throughout clang sources. SourceManager considers all sources as files and the only way to check if SourceLocation is in some predefined source is to check "file name".

[rnk]

According to phabricator, lib/Lex/PPDirectives.cpp got chmoded to executable. Looks good if you can revert that change.

[sepavloff]

Closed by commit rL223114 (authored by @sepavloff).

cfe/trunk/include/clang/Basic/DiagnosticGroups.td

 def Varargs : DiagGroup<"varargs">;
 
 def Unsequenced : DiagGroup<"unsequenced">;
 // GCC name for -Wunsequenced
 def : DiagGroup<"sequence-point", [Unsequenced]>;
 
 // Preprocessor warnings.
 def AmbiguousMacro : DiagGroup<"ambiguous-macro">;
+def KeywordAsMacro : DiagGroup<"keyword-macro">;
+def ReservedIdAsMacro : DiagGroup<"reserved-id-macro">;
 
 // Just silence warnings about -Wstrict-aliasing for now.
 def : DiagGroup<"strict-aliasing=0">;
 def : DiagGroup<"strict-aliasing=1">;
 def : DiagGroup<"strict-aliasing=2">;
 def : DiagGroup<"strict-aliasing">;
 
 // Just silence warnings about -Wstrict-overflow for now.

cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td

def warn_pp_undef_identifier : Warning<
   "%0 is not defined, evaluates to 0">,
   InGroup<DiagGroup<"undef">>, DefaultIgnore;
 def warn_pp_ambiguous_macro : Warning<
   "ambiguous expansion of macro %0">, InGroup<AmbiguousMacro>;
 def note_pp_ambiguous_macro_chosen : Note<
   "expanding this definition of %0">;
 def note_pp_ambiguous_macro_other : Note<
   "other definition of %0">;
+def warn_pp_macro_hides_keyword : Warning<
+  "keyword or reserved identifier is hidden by macro definition">,
+  InGroup<KeywordAsMacro>;
+def warn_pp_macro_is_reserved_id : Warning<
+  "macro name is a keyword or reserved identifier">, InGroup<KeywordAsMacro>;
+def warn_pp_defundef_reserved_ident : Warning<
+  "reserved identifier is used as macro name">, DefaultIgnore,
+  InGroup<ReservedIdAsMacro>;
 
 def pp_invalid_string_literal : Warning<
   "invalid string literal, ignoring final '\\'">;
 def warn_pp_expr_overflow : Warning<
   "integer overflow in preprocessor expression">;
 def warn_pp_convert_lhs_to_positive : Warning<
   "left side of operator converted from negative value to unsigned: %0">;
 def warn_pp_convert_rhs_to_positive : Warning<

cfe/trunk/include/clang/Basic/IdentifierTable.h

void setIsCPlusPlusOperatorKeyword(bool Val = true) {
     IsCPPOperatorKeyword = Val;
     if (Val)
       NeedsHandleIdentifier = 1;
     else
       RecomputeNeedsHandleIdentifier();
   }
   bool isCPlusPlusOperatorKeyword() const { return IsCPPOperatorKeyword; }
 
+  /// \brief Return true if this token is a keyword in the specified language.
+  bool isKeyword(const LangOptions &LangOpts);
+
   /// getFETokenInfo/setFETokenInfo - The language front-end is allowed to
   /// associate arbitrary metadata with this token.
   template<typename T>
   T *getFETokenInfo() const { return static_cast<T*>(FETokenInfo); }
   void setFETokenInfo(void *T) { FETokenInfo = T; }
 
   /// \brief Return true if the Preprocessor::HandleIdentifier must be called
   /// on a token of this identifier.

cfe/trunk/lib/Basic/IdentifierTable.cpp

enum KeywordStatus {
     KS_Extension,   // Is an extension
     KS_Enabled,     // Enabled
     KS_Future       // Is a keyword in future standard
   };
 }
 
 /// \brief Translates flags as specified in TokenKinds.def into keyword status
 /// in the given language standard.
-static KeywordStatus GetKeywordStatus(const LangOptions &LangOpts,
+static KeywordStatus getKeywordStatus(const LangOptions &LangOpts,
                                       unsigned Flags) {
   if (Flags == KEYALL) return KS_Enabled;
   if (LangOpts.CPlusPlus && (Flags & KEYCXX)) return KS_Enabled;
   if (LangOpts.CPlusPlus11 && (Flags & KEYCXX11)) return KS_Enabled;
   if (LangOpts.C99 && (Flags & KEYC99)) return KS_Enabled;
   if (LangOpts.GNUKeywords && (Flags & KEYGNU)) return KS_Extension;
   if (LangOpts.MicrosoftExt && (Flags & KEYMS)) return KS_Extension;
   if (LangOpts.Borland && (Flags & KEYBORLAND)) return KS_Extension;

 }
 
 /// AddKeyword - This method is used to associate a token ID with specific
 /// identifiers because they are language keywords.  This causes the lexer to
 /// automatically map matching identifiers to specialized token codes.
 static void AddKeyword(StringRef Keyword,
                        tok::TokenKind TokenCode, unsigned Flags,
                        const LangOptions &LangOpts, IdentifierTable &Table) {
-  KeywordStatus AddResult = GetKeywordStatus(LangOpts, Flags);
+  KeywordStatus AddResult = getKeywordStatus(LangOpts, Flags);
 
   // Don't add this keyword under MSVCCompat.
   if (LangOpts.MSVCCompat && (Flags & KEYNOMS))
      return;
   // Don't add this keyword if disabled in this language.
   if (AddResult == KS_Disabled) return;
 
   IdentifierInfo &Info =

 #define TESTING_KEYWORD(NAME, FLAGS)
 #include "clang/Basic/TokenKinds.def"
 
   if (LangOpts.ParseUnknownAnytype)
     AddKeyword("__unknown_anytype", tok::kw___unknown_anytype, KEYALL,
                LangOpts, *this);
 }
 
+/// \brief Checks if the specified token kind represents a keyword in the
+/// specified language.
+/// \returns Status of the keyword in the language.
+static KeywordStatus getTokenKwStatus(const LangOptions &LangOpts,
+                                      tok::TokenKind K) {
+  switch (K) {
+#define KEYWORD(NAME, FLAGS) \
+  case tok::kw_##NAME: return getKeywordStatus(LangOpts, FLAGS);
+#include "clang/Basic/TokenKinds.def"
+  default: return KS_Disabled;
+  }
+}
+
+/// \brief Returns true if the identifier represents a keyword in the
+/// specified language.
+bool IdentifierInfo::isKeyword(const LangOptions &LangOpts) {
+  switch (getTokenKwStatus(LangOpts, getTokenID())) {
+  case KS_Enabled:
+  case KS_Extension:
+    return true;
+  default:
+    return false;
+  }
+}
+
 tok::PPKeywordKind IdentifierInfo::getPPKeywordID() const {
   // We use a perfect hash function here involving the length of the keyword,
   // the first and third character.  For preprocessor ID's there are no
   // collisions (if there were, the switch below would complain about duplicate
   // case values).  Note that this depends on 'if' being null terminated.
 
 #define HASH(LEN, FIRST, THIRD) \
   (LEN << 5) + (((FIRST-'a') + (THIRD-'a')) & 31)

cfe/trunk/lib/Lex/PPDirectives.cpp

 void Preprocessor::DiscardUntilEndOfDirective() {
   Token Tmp;
   do {
     LexUnexpandedToken(Tmp);
     assert(Tmp.isNot(tok::eof) && "EOF seen while discarding directive tokens");
   } while (Tmp.isNot(tok::eod));
 }
 
+/// \brief Enumerates possible cases of #define/#undef a reserved identifier.
+enum MacroDiag {
+  MD_NoWarn,        //> Not a reserved identifier
+  MD_KeywordDef,    //> Macro hides keyword, enabled by default
+  MD_KeywordUndef,  //> #undef keyword, enabled by default
+  MD_WarnStrict     //> Other reserved id, disabled by default
+};
+
+/// \brief Checks if the specified identifier is reserved in the specified
+/// language.
+/// This function does not check if the identifier is a keyword.
+static bool isReservedId(StringRef Text, const LangOptions &Lang) {
+  // C++ [macro.names], C11 7.1.3:
+  // All identifiers that begin with an underscore and either an uppercase
+  // letter or another underscore are always reserved for any use.
+  if (Text.size() >= 2 && Text[0] == '_' &&
+      (isUppercase(Text[1]) || Text[1] == '_'))
+    return true;
+  // C++ [global.names]
+  // Each name that contains a double underscore ... is reserved to the
+  // implementation for any use.
+  if (Lang.CPlusPlus) {
+    if (Text.find("__") != StringRef::npos)
+      return true;
+  }
+  return false;
+}
+
+static MacroDiag shouldWarnOnMacroDef(Preprocessor &PP, IdentifierInfo *II) {
+  const LangOptions &Lang = PP.getLangOpts();
+  StringRef Text = II->getName();
+  if (isReservedId(Text, Lang))
+    return MD_WarnStrict;
+  if (II->isKeyword(Lang))
+    return MD_KeywordDef;
+  if (Lang.CPlusPlus && (Text.equals("override") || Text.equals("final")))
+    return MD_KeywordDef;
+  return MD_NoWarn;
+}
+
+static MacroDiag shouldWarnOnMacroUndef(Preprocessor &PP, IdentifierInfo *II) {
+  const LangOptions &Lang = PP.getLangOpts();
+  if (II->isKeyword(Lang))
+    return MD_KeywordUndef;
+  StringRef Text = II->getName();
+  if (Lang.CPlusPlus && (Text.equals("override") || Text.equals("final")))
+    return MD_KeywordUndef;
+  if (isReservedId(Text, Lang))
+    return MD_WarnStrict;
+  return MD_NoWarn;
+}
+
 bool Preprocessor::CheckMacroName(Token &MacroNameTok, MacroUse isDefineUndef) {
   // Missing macro name?
   if (MacroNameTok.is(tok::eod))
     return Diag(MacroNameTok, diag::err_pp_missing_macro_name);
 
   IdentifierInfo *II = MacroNameTok.getIdentifierInfo();
   if (!II) {
     bool Invalid = false;
bool Preprocessor::CheckMacroName(Token &MacroNameTok, MacroUse isDefineUndef) {
 
   if (isDefineUndef == MU_Undef && II->hasMacroDefinition() &&
       getMacroInfo(II)->isBuiltinMacro()) {
     // Warn if undefining "__LINE__" and other builtins, per C99 6.10.8/4
     // and C++ [cpp.predefined]p4], but allow it as an extension.
     Diag(MacroNameTok, diag::ext_pp_undef_builtin_macro);
   }
 
+  // Warn if defining/undefining reserved identifier including keywords.
+  SourceLocation MacroNameLoc = MacroNameTok.getLocation();
+  if (!SourceMgr.isInSystemHeader(MacroNameLoc) &&
+      (strcmp(SourceMgr.getBufferName(MacroNameLoc), "<built-in>") != 0)) {
+    MacroDiag D = MD_NoWarn;
+    if (isDefineUndef == MU_Define)
+      D = shouldWarnOnMacroDef(*this, II);
+    else if (isDefineUndef == MU_Undef)
+      D = shouldWarnOnMacroUndef(*this, II);
+    if (D == MD_KeywordDef)
+      Diag(MacroNameTok, diag::warn_pp_macro_hides_keyword);
+    if (D == MD_KeywordUndef)
+      Diag(MacroNameTok, diag::warn_pp_macro_is_reserved_id);
+    else if (D == MD_WarnStrict)
+      Diag(MacroNameTok, diag::warn_pp_defundef_reserved_ident);
+  }
+
   // Okay, we got a good identifier.
   return false;
 }
 
 /// \brief Lex and validate a macro name, which occurs after a
 /// \#define or \#undef.
 ///
 /// This sets the token kind to eod and discards the rest of the macro line if

cfe/trunk/test/PCH/single-token-macro.c

 // rdar://10588825
 
 // Test this without pch.
 // RUN: %clang_cc1 %s -include %s -verify -fsyntax-only
 
 // Test with pch.
 // RUN: %clang_cc1 %s -emit-pch -o %t
 // RUN: %clang_cc1 %s -include-pch %t -verify -fsyntax-only
 
 // expected-no-diagnostics
 
 #ifndef HEADER
 #define HEADER
 
+#pragma clang diagnostic ignored "-Wreserved-id-macro"
+
 #ifdef __stdcall
 // __stdcall is defined as __attribute__((__stdcall__)) for targeting mingw32.
 #undef __stdcall
 #endif
 
 #define __stdcall
 #define STDCALL __stdcall
 

cfe/trunk/test/Preprocessor/cxx_oper_keyword_ms_compat.cpp

 // RUN: %clang_cc1 %s -E -verify -fms-extensions
 // expected-no-diagnostics
 
+#pragma clang diagnostic ignored "-Wkeyword-macro"
+
 bool f() {
   // Check that operators still work before redefining them.
 #if compl 0 bitand 1
   return true and false;
 #endif
 }
 
 #ifdef and

cfe/trunk/test/Sema/thread-specifier.c

 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic %s -DGNU
 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DGNU
 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic %s -DC11 -D__thread=_Thread_local
 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DC11 -D__thread=_Thread_local
 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DCXX11 -D__thread=thread_local -std=c++11 -Wno-deprecated
 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DC11 -D__thread=_Thread_local -std=c++11 -Wno-deprecated
 
+#pragma clang diagnostic ignored "-Wkeyword-macro"
+
 #ifdef __cplusplus
 // In C++, we define __private_extern__ to extern.
 #undef __private_extern__
 #endif
 
 __thread int t1;
 __thread extern int t2;
 __thread static int t3;