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

John Brawn via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed May 24 07:06:10 PDT 2023


john.brawn 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");
 }
----------------
aaron.ballman wrote:
> 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.
https://github.com/llvm/llvm-project/issues/62919


================
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
----------------
aaron.ballman wrote:
> 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).
Yes the intent is for the test to fail on any warning. Using `-Werror` and not `-verify` means that on failure you get which macro caused the error:
```
Exit Code: 1

Command Output (stderr):
--
<built-in>:351:9: error: redefining builtin macro [-Werror,-Wbuiltin-macro-redefined]
#define __LITTLE_ENDIAN__ 1
        ^
1 error generated.

--
```
whereas `-verify` just outputs
```
Exit Code: 1

Command Output (stderr):
--
error: 'error' diagnostics seen but not expected:
  Line 351: redefining builtin macro
1 error generated.

--
```

Using `-Eonly` makes sense, I'll do that.


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

https://reviews.llvm.org/D150966



More information about the cfe-commits mailing list