[Lex] Only warn on defining or undefining language-defined builtins

[john.brawn]

Summary

D144654 made it so that we warn on any defining or undefining of builtin macros. However the C and C++ standards only forbid the defining or undefining of macros defined in the language standard itself, but clang defines more macros than those and warning on those may not be helpful.

Resolve this by only warning if the builtin macro name is the name of a macro defined by the language. This is done in a way that removes some of the existing checks, as those were made redundant by restricting the warning in this way.

[nickdesaulniers]

clang/test/Preprocessor/macro-reserved.c

19

Please add tests for the rest of https://reviews.llvm.org/D144654#4381893. AFAICT, i386 requires -m32 to be defined in the first place.

[nickdesaulniers]

clang/lib/Lex/PPDirectives.cpp

163–172

Should these depend on LangOpt?

[nathanchance]

This patch cleared up all the warnings I reported, thank you for the fix!

[mstorsjo]

Thanks! +1 from me, this does clear up all the issues I've had - it supersedes D151662 and makes the external patch to avoid doing -U__STRICT_ANSI__ unnecessary.

[john.brawn]

Add more testing.

[john.brawn]

clang/lib/Lex/PPDirectives.cpp

163–172

I don't think so. If we're e.g. not compiling C++ then __cplusplus won't be defined so it doesn't matter if we check for it.

[nickdesaulniers]

clang/test/Preprocessor/macro-reserved.c

22

This line wont have coverage unless the RUN line sets -m32 (even then, an explicit triple would be better for non-x86 hosts running the test).

Maybe add this undef to either init-x86.c or predefined-arch-macros.c?

[john.brawn]

Move some of the testing to init-x86.c

[nickdesaulniers]

clang/test/Preprocessor/init-x86.c

1728–1741

Hmm...I guess if this is going to require adding run lines, then let's move these to a new test file in the same dir. Maybe undef-x86.c or whatever? Sorry for back and forth.

[john.brawn]

Put x86 tests in undef-x86.c.

[nickdesaulniers]

Thanks for the patch!

clang/test/Preprocessor/undef-x86.c

2 ↗

(On Diff #527432)

I don't think i686 adds anything beyond i386, at least for this test they are both -m32; consider removing this run line.

clang/lib/Lex/PPDirectives.cpp

static constexpr StringRef ReservedMacro[] = {
       "_XOPEN_SOURCE_EXTENDED",
       "__STDCPP_WANT_MATH_SPEC_FUNCS__",
       "__STDC_FORMAT_MACROS",
   };
   return std::binary_search(std::begin(ReservedMacro), std::end(ReservedMacro),
                             MacroName);
 }
 
+static bool isLanguageDefinedBuiltin(const SourceManager &SourceMgr,
+                                     const MacroInfo *MI,
+                                     const StringRef MacroName) {
+  // If this is a macro with special handling (like __LINE__) then it's language
+  // defined.
+  if (MI->isBuiltinMacro())
+    return true;
+  // Builtin macros are defined in the builtin file
+  if (!SourceMgr.isWrittenInBuiltinFile(MI->getDefinitionLoc()))
+    return false;
+  // C defines macros starting with __STDC, and C++ defines macros starting with
+  // __STDCPP
+  if (MacroName.startswith("__STDC"))
+    return true;
+  // C++ defines the __cplusplus macro
+  if (MacroName == "__cplusplus")
+    return true;
+  // C++ defines various feature-test macros starting with __cpp
+  if (MacroName.startswith("__cpp"))
+    return true;
+  // Anything else isn't language-defined
+  return false;
+}
+
 static MacroDiag shouldWarnOnMacroDef(Preprocessor &PP, IdentifierInfo *II) {
   const LangOptions &Lang = PP.getLangOpts();
   StringRef Text = II->getName();
   if (isReservedInAllContexts(II->isReserved(Lang)))
     return isFeatureTestMacro(Text) ? MD_NoWarn : MD_ReservedMacro;
   if (II->isKeyword(Lang))
     return MD_KeywordDef;
   if (Lang.CPlusPlus11 && (Text.equals("override") || Text.equals("final")))
if (const MacroInfo *OtherMI=getMacroInfo(MacroNameTok.getIdentifierInfo())) {
     if (!getDiagnostics().getSuppressSystemWarnings() ||
         !SourceMgr.isInSystemHeader(DefineTok.getLocation())) {
 
       if (!OtherMI->isUsed() && OtherMI->isWarnIfUnused())
         Diag(OtherMI->getDefinitionLoc(), diag::pp_macro_not_used);
 
       // Warn if defining "__LINE__" and other builtins, per C99 6.10.8/4 and
       // C++ [cpp.predefined]p4, but allow it as an extension.
-      if (OtherMI->isBuiltinMacro() ||
-          (SourceMgr.isWrittenInBuiltinFile(OtherMI->getDefinitionLoc()) &&
-           !isFeatureTestMacro(MacroNameTok.getIdentifierInfo()->getName())))
+      if (isLanguageDefinedBuiltin(SourceMgr, OtherMI, II->getName()))
         Diag(MacroNameTok, diag::ext_pp_redef_builtin_macro);
       // Macros must be identical.  This means all tokens and whitespace
       // separation must be the same.  C99 6.10.3p2.
       else if (!OtherMI->isAllowRedefinitionsWithoutWarning() &&
                !MI->isIdenticalTo(*OtherMI, *this, /*Syntactic=*/LangOpts.MicrosoftExt)) {
         Diag(MI->getDefinitionLoc(), diag::ext_pp_macro_redef)
           << MacroNameTok.getIdentifierInfo();
         Diag(OtherMI->getDefinitionLoc(), diag::note_previous_definition);
if (II->isFinal())
     emitFinalMacroWarning(MacroNameTok, /*IsUndef=*/true);
 
   // If the macro is not defined, this is a noop undef.
   if (const MacroInfo *MI = MD.getMacroInfo()) {
     if (!MI->isUsed() && MI->isWarnIfUnused())
       Diag(MI->getDefinitionLoc(), diag::pp_macro_not_used);
 
     // Warn if undefining "__LINE__" and other builtins, per C99 6.10.8/4 and
-    // C++ [cpp.predefined]p4, but allow it as an extension. Don't warn if this
-    // is an Objective-C builtin macro though.
-    if ((MI->isBuiltinMacro() ||
-         SourceMgr.isWrittenInBuiltinFile(MI->getDefinitionLoc())) &&
-        !(getLangOpts().ObjC && isObjCProtectedMacro(II)))
+    // C++ [cpp.predefined]p4, but allow it as an extension.
+    if (isLanguageDefinedBuiltin(SourceMgr, MI, II->getName()))
       Diag(MacroNameTok, diag::ext_pp_undef_builtin_macro);
 
     if (MI->isWarnIfUnused())
       WarnUnusedMacroLocs.erase(MI->getDefinitionLoc());
 
     Undef = AllocateUndefMacroDirective(MacroNameTok.getLocation());
   }
 

clang/test/Preprocessor/init-x86.c

 // X86_64-NETBSD:#define __WINT_WIDTH__ 32
 // X86_64-NETBSD:#define __amd64 1
 // X86_64-NETBSD:#define __amd64__ 1
 // X86_64-NETBSD:#define __x86_64 1
 // X86_64-NETBSD:#define __x86_64__ 1
 
 // RUN: %clang_cc1 -E -dM -triple=i386-unknown-openbsd -x c++ < /dev/null | FileCheck -match-full-lines -check-prefix I386-OPENBSD-CXX %s
 // I386-OPENBSD-CXX: #define __STDCPP_DEFAULT_NEW_ALIGNMENT__ 16UL
+
+// RUN: %clang_cc1 -triple=i386-none-none -fsyntax-only -verify -DUNDEF %s
+// RUN: %clang_cc1 -triple=i686-none-none -fsyntax-only -verify -DUNDEF %s
+// RUN: %clang_cc1 -triple=x86_64-none-none -fsyntax-only -verify -DUNDEF %s
+#ifdef UNDEF
+// Check that we can undefine triple-specific defines without warning
+// expected-no-diagnostics
+#undef __i386
+#undef __i386__
+#undef i386
+#undef __amd64
+#undef __amd64__
+#undef __x86_64
+#undef __x86_64__
+#endif

clang/test/Preprocessor/macro-reserved.c

 // RUN: %clang_cc1 -fsyntax-only -pedantic -verify %s
 
 #define for 0    // expected-warning {{keyword is hidden by macro definition}}
 #define final 1
 #define __HAVE_X 0
 #define __cplusplus
 #define _HAVE_X 0
 #define X__Y
 #define __STDC__ 1 // expected-warning {{redefining builtin macro}}
+#define __clang__ 1
 
 #undef for
 #undef final
 #undef __HAVE_X
 #undef __cplusplus
 #undef _HAVE_X
 #undef X__Y
 #undef __STDC_HOSTED__ // expected-warning {{undefining builtin macro}}
+#undef __INT32_TYPE__
+#undef __UINT32_TYPE__
+#undef __UINTPTR_TYPE__
+#undef __UINT64_TYPE__
+#undef __INT64_TYPE__
 
 // allowlisted definitions
 #define while while
 #define const
 #define static
 #define extern
 #define inline