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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to