[PATCH] D150966: [clang] Don't define predefined macros multiple times

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:241-242
   Builder.defineMacro("__ARM_FEATURE_QRDMX", "1");
-  Builder.defineMacro("__ARM_FEATURE_ATOMICS", "1");
-  Builder.defineMacro("__ARM_FEATURE_CRC32", "1");
 }
----------------
john.brawn wrote:
> aaron.ballman wrote:
> > Hmm, is this correct?
> > 
> > `__ARM_FEATURE_ATOMICS` is defined in one other place, but it's conditionally defined: https://github.com/llvm/llvm-project/blob/11926e6149d2a68ecb0652b248efe6890c163846/clang/lib/Basic/Targets/AArch64.cpp#L475
> > 
> > and `__ARM_FEATURE_CRC32` is defined in two places, both conditional: https://github.com/llvm/llvm-project/blob/11926e6149d2a68ecb0652b248efe6890c163846/clang/lib/Basic/Targets/AArch64.cpp#L422 and https://github.com/llvm/llvm-project/blob/11926e6149d2a68ecb0652b248efe6890c163846/clang/lib/Basic/Targets/ARM.cpp#L747
> > 
> > 
> AArch64TargetInfo::setArchFeatures sets HasCRC and HasLSE to true for >= 8.1. This does mean that if you do `-march=armv8.1-a+nocrc` then the current behaviour is that __ARM_FEATURE_CRC32 is defined but the behaviour with this patch is that it's not defined, but the new behaviour is correct as we shouldn't be defining it in that case.
Ah, okay! I think it's worth adding a test case for that scenario to show we've made a bugfix here, not just an NFC change.


================
Comment at: clang/lib/Basic/Targets/VE.cpp:30-32
-  Builder.defineMacro("unix", "1");
-  Builder.defineMacro("__unix__", "1");
-  Builder.defineMacro("__linux__", "1");
----------------
john.brawn wrote:
> aaron.ballman wrote:
> > Shouldn't this be calling `DefineStd(Builder, "unix", Opts);` like all the others?
> Only the OS target should be defining OS-related macros, and in AllocateTarget in Targets.cpp VETargetInfo is hardcoded to always use LinuxTargetInfo, which calls `DefineStd(Builder, "unix", Opts);`.
Hmmm, we still have calls to this outside of `getOSDefines()` though?

https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/ARM.cpp#L1400
https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/Le64.cpp#L28
https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/X86.h#L634
https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/X86.h#L907

should those also be changing (perhaps in a follow-up)?


================
Comment at: clang/lib/Basic/Targets/VE.cpp:35
   Builder.defineMacro("__ve__", "1");
-  Builder.defineMacro("__STDC_HOSTED__", "1");
-  Builder.defineMacro("__STDC__", "1");
----------------
aaron.ballman wrote:
> This is technically a breaking change but I think it's a good breaking change. The VE target seems to imply that there's never a freestanding mode unlike the general utility: https://github.com/llvm/llvm-project/blob/11926e6149d2a68ecb0652b248efe6890c163846/clang/lib/Frontend/InitPreprocessor.cpp#L431
This might also be worth an explicit test case so it's clear that the behavioral change is intentional.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150966/new/

https://reviews.llvm.org/D150966