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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Basically LG though I had a question about the test case.



================
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:
> > 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.
> Actually I went and double-checked and I'm wrong, `-march=armv8.N+nowhatever` doesn't cause __ARM_FEATURE_WHATEVER to be undefined. Which seems wrong, but it's beyond the scope of this patch.
Yeah, that does seem wrong. Agreed it's not in scope for this patch, but if you would file an issue in GitHub so we don't lose track of it, that'd be appreciated.


================
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:
> > 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)?
> Most of these are TargetInfos that combine OS plus target, e.g. CygwinARMTargetInfo is both Cygwin OS and ARM Target. The only exception is Le64, and I have no idea if it's correct or not for it to be doing that.
Ahhh, that's a good point about being combined OS + target. Thanks for pointing that out!


================
Comment at: clang/test/Preprocessor/predefined-macros-no-warnings.c:5
+// warnings suppressed by default.
+// RUN: %clang_cc1 %s -E -o /dev/null -Wsystem-headers -Werror -triple arc
+// RUN: %clang_cc1 %s -E -o /dev/null -Wsystem-headers -Werror -triple xcore
----------------
So the expectation is that any warnings that are emitted would be upgraded to an error and the test would be flagged as a failure because %clang_cc1 would return nonzero in that case?

(I was thrown for a loop by not using `-verify` and `// expected-no-diagnostics`)

Pretty sure `-Eonly` is equivalent (it runs the preprocessor without emitting output, so no extra overhead from piping to /dev/null).


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

https://reviews.llvm.org/D150966