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