[Lex] Warn when defining or undefining any builtin macro

[john.brawn]

Summary

Currently we warn when MI->isBuiltinMacro, but this is only true for builtin macros that require processing when expanding. Checking SourceMgr.isWrittenInBuiltinFile in addition to this will mean that we catch all builtin macros, though we shouldn't warn on feature test macros.

As part of doing this I've also moved the handling of undefining from CheckMacroName to HandleUndefDirective, as it doesn't really make sense to handle undefining in CheckMacroName but defining in HandleDefineDirective. It would be nice to instead handle both in CheckMacroName, but that isn't possible as the handling of defines requires looking at what the name is being defined to.

[aaron.ballman]

This seems to cause precommit CI failures that should be addressed, and it should also have a release note about the fix.

Also, should this cover things like clang -U__cplusplus -D__STDC__=1 foo.cpp as well?

[john.brawn]

In D144654#4156430, @aaron.ballman wrote:

This seems to cause precommit CI failures that should be addressed, and it should also have a release note about the fix.

Failures in CI should be fixed by D145397 and D144651. I'll also add a release note.

Also, should this cover things like clang -U__cplusplus -D__STDC__=1 foo.cpp as well?

Yes, and I'll add tests for this.

[john.brawn]

Add command line test and release note, and run clang-format.

[john.brawn]

D145691 should fix the libc++ CI failures.

[aaron.ballman]

clang/lib/Lex/PPDirectives.cpp

3187–3190

Should this diagnostic be suppressed in a system header on the assumption that system headers are part of the implementation and thus free to undefine/redefine macros at will?

[john.brawn]

clang/lib/Lex/PPDirectives.cpp

3187–3190

It's already suppressed. ShowInSystemHeader in the Diagnostic tablegen class determines if a diagnostic is shown when it happens in a system header, and it's false by default for warnings.

[aaron.ballman]

It looks like there's still relevant precommit CI failures: clang-tidy/checkers/readability/identifier-naming-case-violation.cpp looked related to this patch.

clang/lib/Lex/PPDirectives.cpp

3187–3190

Ah, good point!

[john.brawn]

Rebasing now that the patches this depends on have been committed.

[john.brawn]

The clang-tidy failures in pre-merge are because

• clang-tidy tests use the compile database from building llvm (though you have to manually copy it otherwise the tests silently pass without actually running the test, which is why I wasn't seeing this locally)
• this has -D_GNU_SOURCE because llvm/cmake/config-ix.cmake adds it to the defines
• clang predefines _GNU_SOURCE when compiling C++ (for certain targets)
• therefore we get the warning for a redefined builtin

I'm not sure yet what the best fix is here. gcc has the same behaviour of defining _GNU_SOURCE for C++, but does it by adding -D_GNU_SOURCE to the cc1 command line so it doesn't give a warning on redefine, but it doesn't look like copying this behaviour in clang would be easy. I'll think about this some more.

[john.brawn]

I've gone with handling the _GNU_SOURCE problem in the clang-tidy tests by re-using the list of feature test macros in shouldWarnOnMacroDef, and not warning if there's a match.

[aaron.ballman]

clang/test/Preprocessor/macro-reserved.cpp

15

Why do we diagnose the undef but not the define?

[john.brawn]

Adjusted in order to make clang-format happy.

clang/test/Preprocessor/macro-reserved.cpp

15

After the undef the builtin macro definition no longer exists, so when Preprocessor::HandleDefineDirective checks for an existing definition to see if it's a builtin it doesn't find one.

[aaron.ballman]

LGTM, though precommit CI wasn't able to run on this, so please pay attention to the bots when you land in case there's fallout.

clang/test/Preprocessor/macro-reserved.cpp

15

Ah, that makes sense. Thank you!

[john.brawn]

This patch caused a failure in AArch64 buildbots due to AArch64TargetInfo::getTargetDefines redefining _LP64 and LP64. Fixed in https://reviews.llvm.org/rGe55d52cd34fb7a6a6617639d147b9d0abaceeeab.

[thakis]

In D144654#4349908, @john.brawn wrote:

This patch caused a failure in AArch64 buildbots due to AArch64TargetInfo::getTargetDefines redefining _LP64 and LP64. Fixed in https://reviews.llvm.org/rGe55d52cd34fb7a6a6617639d147b9d0abaceeeab.

Tests are still failing even with this: http://45.33.8.238/macm1/60903/step_7.txt

[thakis]

Reverted in cf8c358dc9414e3de9f65eebdfdcb66dd5817857 for now.

[john.brawn]

D150966 should fix the failure that was seen in buildbots, so I've now re-committed this.

[nathanchance]

For what it's worth, this change is quite noisy for the Linux kernel, as there are certain macros undefined in header files that are included in many other headers. Additionally, at least one new instance is in a UAPI header, so it cannot be changed to my knowledge (honestly, all of these appear to be intentional, so I am not sure any of them should be changed). Is there any way for us to opt out of this? It would be a shame if we had to outright disable -Wbuiltin-macro-redefined, it has caught issues before (the kernel is generally against opting out on a per-location basis but does have the infrastructure to do so, not sure if it will be available for all these locations though...).

arch/arm/include/uapi/asm/types.h: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/include/uapi/asm/types.h?h=v6.4-rc3

In file included from scripts/mod/devicetable-offsets.c:3:
In file included from include/linux/mod_devicetable.h:12:
In file included from include/uapi/linux/mei.h:10:
In file included from include/uapi/linux/mei_uuid.h:12:
In file included from include/linux/types.h:6:
In file included from include/uapi/linux/types.h:5:
arch/arm/include/uapi/asm/types.h:27:8: error: undefining builtin macro [-Werror,-Wbuiltin-macro-redefined]
#undef __INT32_TYPE__
       ^
arch/arm/include/uapi/asm/types.h:32:8: error: undefining builtin macro [-Werror,-Wbuiltin-macro-redefined]
#undef __UINT32_TYPE__
       ^
arch/arm/include/uapi/asm/types.h:37:8: error: undefining builtin macro [-Werror,-Wbuiltin-macro-redefined]
#undef __UINTPTR_TYPE__
       ^
3 errors generated.

arch/arm64/include/asm/neon-intrinsics.h: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/neon-intrinsics.h?h=v6.4-rc3

In file included from arch/arm64/lib/xor-neon.c:11:
arch/arm64/include/asm/neon-intrinsics.h:18:8: error: undefining builtin macro [-Werror,-Wbuiltin-macro-redefined]
#undef __INT64_TYPE__
       ^
arch/arm64/include/asm/neon-intrinsics.h:23:8: error: undefining builtin macro [-Werror,-Wbuiltin-macro-redefined]
#undef __UINT64_TYPE__
       ^
2 errors generated.

include/linux/drbd_genl_api.h (this may be unnecessary now?): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/drbd_genl_api.h?h=v6.4-rc3

In file included from drivers/block/drbd/drbd_debugfs.c:11:
In file included from drivers/block/drbd/drbd_int.h:35:
include/linux/drbd_genl_api.h:47:8: error: undefining builtin macro [-Werror,-Wbuiltin-macro-redefined]
#undef linux
       ^
1 error generated.

arch/mips/kernel/vmlinux.lds.S (there is little context available as to why this exists): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/vmlinux.lds.S?h=v6.4-rc3

In file included from <built-in>:357:
<command line>:6:8: error: undefining builtin macro [-Werror,-Wbuiltin-macro-redefined]
#undef mips
       ^
1 error generated.

[john.brawn]

gcc has the same warning so I wasn't expecting this cause to change problems, but looking more closely at gcc's behaviour it looks like it only warns for some builtin macros and not others (though glancing over the gcc source code it's not clear which macros and for what reason).

I'll look at this some more and see if I can improve the behaviour.

[mstorsjo]

FWIW, I've also run into noisy warnings caused by this, in a few places. D151662 fixes a bunch of clang's tests when run in MinGW configurations, and https://patchwork.ffmpeg.org/project/ffmpeg/patch/[e-mail]/ tries to avoid warnings due to -U__STRICT_ANSI__ in an external project.

[nickdesaulniers]

In D144654#4380488, @john.brawn wrote:

gcc has the same warning so I wasn't expecting this cause to change problems, but looking more closely at gcc's behaviour it looks like it only warns for some builtin macros and not others (though glancing over the gcc source code it's not clear which macros and for what reason).

I'll look at this some more and see if I can improve the behaviour.

Based on
https://lore.kernel.org/llvm/[e-mail]/T/#u
I think the following macros aren't warned on: https://godbolt.org/z/dfqnG7bae

#undef __INT32_TYPE__
#undef __UINT32_TYPE__
#undef __UINTPTR_TYPE__
#undef __i386__
#undef __UINT64_TYPE__
#undef __INT64_TYPE__

[cseo]

In D144654#4381893, @nickdesaulniers wrote:

In D144654#4380488, @john.brawn wrote:

gcc has the same warning so I wasn't expecting this cause to change problems, but looking more closely at gcc's behaviour it looks like it only warns for some builtin macros and not others (though glancing over the gcc source code it's not clear which macros and for what reason).

I'll look at this some more and see if I can improve the behaviour.

Based on
https://lore.kernel.org/llvm/[e-mail]/T/#u
I think the following macros aren't warned on: https://godbolt.org/z/dfqnG7bae

#undef __INT32_TYPE__
#undef __UINT32_TYPE__
#undef __UINTPTR_TYPE__
#undef __i386__
#undef __UINT64_TYPE__
#undef __INT64_TYPE__

glibc build also fails because of

#undef  __OPTIMIZE__

clang/lib/Lex/PPDirectives.cpp

if (II->isCPlusPlusOperatorKeyword()) {
     // recovery when legacy C headers are included in C++.
   }
 
   if ((isDefineUndef != MU_Other) && II->getPPKeywordID() == tok::pp_defined) {
     // Error if defining "defined": C99 6.10.8/4, C++ [cpp.predefined]p4.
     return Diag(MacroNameTok, diag::err_defined_macro_name);
   }
 
-  if (isDefineUndef == MU_Undef) {
-    auto *MI = getMacroInfo(II);
-    if (MI && MI->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);
-    }
-  }
-
   // If defining/undefining reserved identifier or a keyword, we need to issue
   // a warning.
   SourceLocation MacroNameLoc = MacroNameTok.getLocation();
   if (ShadowFlag)
     *ShadowFlag = false;
   if (!SourceMgr.isInSystemHeader(MacroNameLoc) &&
       (SourceMgr.getBufferName(MacroNameLoc) != "<built-in>")) {
     MacroDiag D = MD_NoWarn;
if (VAOCtx.isInVAOpt()) {
       return nullptr;
     }
   }
   MI->setDefinitionEndLoc(LastTok.getLocation());
 
   MI->setTokens(Tokens, BP);
   return MI;
 }
+
+static bool isObjCProtectedMacro(const IdentifierInfo *II) {
+  return II->isStr("__strong") ||
+         II->isStr("__weak") ||
+         II->isStr("__unsafe_unretained") ||
+         II->isStr("__autoreleasing");
+}
+
 /// HandleDefineDirective - Implements \#define.  This consumes the entire macro
 /// line then lets the caller lex the next real token.
 void Preprocessor::HandleDefineDirective(
     Token &DefineTok, const bool ImmediatelyAfterHeaderGuard) {
   ++NumDefined;
 
   Token MacroNameTok;
   bool MacroShadowsKeyword;
if (const MacroInfo *OtherMI=getMacroInfo(MacroNameTok.getIdentifierInfo())) {
     // identical. Even if they are in system headers. Even if they are things we
     // would silently allow in the past.
     if (MacroNameTok.getIdentifierInfo()->isFinal())
       emitFinalMacroWarning(MacroNameTok, /*IsUndef=*/false);
 
     // In Objective-C, ignore attempts to directly redefine the builtin
     // definitions of the ownership qualifiers.  It's still possible to
     // #undef them.
-    auto isObjCProtectedMacro = [](const IdentifierInfo *II) -> bool {
-      return II->isStr("__strong") ||
-             II->isStr("__weak") ||
-             II->isStr("__unsafe_unretained") ||
-             II->isStr("__autoreleasing");
-    };
     if (getLangOpts().ObjC &&
         SourceMgr.getFileID(OtherMI->getDefinitionLoc())
           == getPredefinesFileID() &&
         isObjCProtectedMacro(MacroNameTok.getIdentifierInfo())) {
       // Warn if it changes the tokens.
       if ((!getDiagnostics().getSuppressSystemWarnings() ||
            !SourceMgr.isInSystemHeader(DefineTok.getLocation())) &&
           !MI->isIdenticalTo(*OtherMI, *this,
                              /*Syntactic=*/LangOpts.MicrosoftExt)) {
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())
+      if (OtherMI->isBuiltinMacro() ||
+          SourceMgr.isWrittenInBuiltinFile(OtherMI->getDefinitionLoc()))
         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);
void Preprocessor::HandleUndefDirective() {
   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)))
+      Diag(MacroNameTok, diag::ext_pp_undef_builtin_macro);
+
     if (MI->isWarnIfUnused())
       WarnUnusedMacroLocs.erase(MI->getDefinitionLoc());
 
     Undef = AllocateUndefMacroDirective(MacroNameTok.getLocation());
   }
 
   // If the callbacks want to know, tell them about the macro #undef.
   // Note: no matter if the macro was defined or not.

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}}
 
 #undef for
 #undef final
 #undef __HAVE_X
 #undef __cplusplus
 #undef _HAVE_X
 #undef X__Y
+#undef __STDC_HOSTED__ // expected-warning {{undefining builtin macro}}
 
 // allowlisted definitions
 #define while while
 #define const
 #define static
 #define extern
 #define inline
 

clang/test/Preprocessor/macro-reserved.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -pedantic -std=c++98 %s
 
 #define for 0    // expected-warning {{keyword is hidden by macro definition}}
 #define final 1
 #define __HAVE_X 0
 #define _HAVE_X 0
 #define X__Y
 
 #undef for
 #undef final
 #undef __HAVE_X
 #undef _HAVE_X
 #undef X__Y
 
-#undef __cplusplus
+#undef __cplusplus // expected-warning {{undefining builtin macro}}
 #define __cplusplus
 
 // allowlisted definitions
 #define while while
 #define const
 #define static
 #define extern
 #define inline