This is an archive of the discontinued LLVM Phabricator instance.

[Lex] Warn when defining or undefining any builtin macro
ClosedPublic

Authored by john.brawn on Feb 23 2023, 9:24 AM.

Details

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.

Diff Detail

Event Timeline

john.brawn created this revision.Feb 23 2023, 9:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 9:24 AM
john.brawn requested review of this revision.Feb 23 2023, 9:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 9:24 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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?

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 updated this revision to Diff 503305.Mar 8 2023, 3:46 AM

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

D145691 should fix the libc++ CI failures.

aaron.ballman added inline comments.Mar 9 2023, 8:12 AM
clang/lib/Lex/PPDirectives.cpp
3194–3197

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 added inline comments.Mar 13 2023, 10:47 AM
clang/lib/Lex/PPDirectives.cpp
3194–3197

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.

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
3194–3197

Ah, good point!

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

john.brawn planned changes to this revision.May 12 2023, 8:39 AM

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 edited the summary of this revision. (Show Details)

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 added inline comments.May 15 2023, 10:55 AM
clang/test/Preprocessor/macro-reserved.cpp
15

Why do we diagnose the undef but not the define?

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 accepted this revision.May 16 2023, 10:20 AM

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!

This revision is now accepted and ready to land.May 16 2023, 10:20 AM
This revision was landed with ongoing or failed builds.May 17 2023, 3:50 AM
This revision was automatically updated to reflect the committed changes.

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

thakis added a subscriber: thakis.May 17 2023, 7:59 AM

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

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

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.

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.

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/20230526104837.20594-1-martin@martin.st/ tries to avoid warnings due to -U__STRICT_ANSI__ in an external project.

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/6475a837.170a0220.77d4a.116a@mx.google.com/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 added a subscriber: cseo.EditedJun 1 2023, 5:54 AM

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/6475a837.170a0220.77d4a.116a@mx.google.com/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__