[PATCH] D17893: Sema: Add semantic analysis for the C++ ABI stability attributes and whitelist.

[Peter Collingbourne via cfe-commits]

pcc added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8380
@@ +8379,3 @@
+def warn_unused_abi_stability_attr : Warning<
+  "unused C++ ABI stability attribute on non-dynamic class">,
+  InGroup<DiagGroup<"unused-stability-attr">>;
----------------
rsmith wrote:
> How valuable is it to warn on this? It seems like we may want `unstable_abi` to affect other things than virtual functions in future (for instance, we may want to apply the ARM ABI "return this from constructors" change to classes with unstable ABI, fix some subtle problems in the class layout algorithm, pass certain trivially-copyable structs in registers even though they're not POD for the purpose of layout, ...). If the design intent is that this only affects the virtual call portion of the ABI, then I think it has the wrong name.
> 
> (If someone asks for the unstable ABI for the class, and it happens that the unstable ABI is the same as the stable one, I don't think that warrants a warning.)
I agree that the attribute should permit other struct ABI changes. I also agree that we shouldn't warn by default. We've traditionally warned when an attribute has no effect, but that's probably the wrong decision in this case. I've made this a non-default warning.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:4891
@@ +4890,3 @@
+void Sema::checkClassABI(CXXRecordDecl *Record) {
+  // This can only be done accurately for non-dependent types, as the
+  // determination uses the class's bases, which may be dependent.
----------------
rsmith wrote:
> Can we exit early if no unstable class ABI flags were passed?
We may need to inherit an ABI from a base, even if no flags were passed.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:4935-4943
@@ +4934,11 @@
+
+  bool HasStableAttr = Record->hasAttr<StableABIAttr>();
+  bool HasUnstableAttr = Record->hasAttr<UnstableABIAttr>();
+  if (HasStableAttr && HasUnstableAttr) {
+    Diag(Record->getLocation(), diag::err_abi_mismatch) << Record;
+    Diag(Record->getAttr<StableABIAttr>()->getLocation(),
+         diag::note_abi_stability_attr) << /*Unstable=*/false;
+    Diag(Record->getAttr<UnstableABIAttr>()->getLocation(),
+         diag::note_abi_stability_attr) << /*Unstable=*/true;
+  }
+
----------------
rsmith wrote:
> Should you also diagnose conflicts with other struct ABI attributes like `ms_struct`?
I'd expect those attributes to be orthogonal, e.g. `ms_struct` + `unstable_abi` would give you whatever just `unstable_abi` would give you when targeting Windows.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:5018-5019
@@ +5017,4 @@
+  if (!SourceMgr.isInSystemHeader(Record->getLocation())) {
+    Diag(Record->getLocation(), diag::warn_cxx_stable_abi)
+        << Record;
+    if (!getLangOpts().UnstableABIContextNamesPath.empty()) {
----------------
rsmith wrote:
> `-Weverything` users will not like this.
That's unfortunate, but I think the alternative of creating a category of warnings that `-Weverything` does not warn on would be worse. My understanding is that as a user-facing flag `-Weverything` is mostly intended for manual discovery of warnings that Clang produces; as part of that manual process, users can easily add the `-Wno-c++-stable-abi` flag.


http://reviews.llvm.org/D17893