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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I think this is okay.  We can review further if we see other problems.


https://reviews.llvm.org/D17893



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

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

[John McCall via cfe-commits]

rjmccall added inline comments.


Comment at: lib/Sema/SemaDeclCXX.cpp:4935-4943
@@ +4934,11 @@
+
+  bool HasStableAttr = Record->hasAttr();
+  bool HasUnstableAttr = Record->hasAttr();
+  if (HasStableAttr && HasUnstableAttr) {
+Diag(Record->getLocation(), diag::err_abi_mismatch) << Record;
+Diag(Record->getAttr()->getLocation(),
+ diag::note_abi_stability_attr) << /*Unstable=*/false;
+Diag(Record->getAttr()->getLocation(),
+ diag::note_abi_stability_attr) << /*Unstable=*/true;
+  }
+

pcc wrote:
> rjmccall wrote:
> > No, ms_struct is a request for a specific ABI; this would be a conflict.
> Fair point. I think we can handle this by making `ms_struct` imply 
> `stable_abi` in the same way that `uuid` would imply `stable_abi`. Then there 
> would be a conflict between `ms_struct` and `unstable_abi`, and we would 
> infer the correct ABI in implied namespaces.
> 
> Are there any other attributes like that? I could only find `vecreturn` and 
> `novtable`.
I can't think of anything else that's a conflict.  vecreturn affects a 
different aspect of the ABI.  novtable is advisory and orthogonal.  
Alignment/packed attributes should still be honored under an unstable ABI.


http://reviews.llvm.org/D17893



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

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

[Mehdi AMINI via cfe-commits]

joker.eph added inline comments.


Comment at: lib/Sema/SemaDeclCXX.cpp:4999
@@ +4998,3 @@
+return;
+  }
+

pcc wrote:
> joker.eph wrote:
> > joker.eph wrote:
> > > Isn't this correct by the loop which starts with `DC = 
> > > InnermostExternalDC`?
> > `s/correct/already covered/`
> That loop checks for glob contexts (contexts ending in `**` in the ABI list), 
> which have different semantics to regular contexts (ending in `*`).
Oh, I misread `UnstableABIGlobContexts` for `UnstableABIContexts`...


http://reviews.llvm.org/D17893



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

Re: [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: lib/Sema/SemaDeclCXX.cpp:4999
@@ +4998,3 @@
+return;
+  }
+

joker.eph wrote:
> joker.eph wrote:
> > Isn't this correct by the loop which starts with `DC = InnermostExternalDC`?
> `s/correct/already covered/`
That loop checks for glob contexts (contexts ending in `**` in the ABI list), 
which have different semantics to regular contexts (ending in `*`).


http://reviews.llvm.org/D17893



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

Re: [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: lib/Sema/SemaDeclCXX.cpp:4935-4943
@@ +4934,11 @@
+
+  bool HasStableAttr = Record->hasAttr();
+  bool HasUnstableAttr = Record->hasAttr();
+  if (HasStableAttr && HasUnstableAttr) {
+Diag(Record->getLocation(), diag::err_abi_mismatch) << Record;
+Diag(Record->getAttr()->getLocation(),
+ diag::note_abi_stability_attr) << /*Unstable=*/false;
+Diag(Record->getAttr()->getLocation(),
+ diag::note_abi_stability_attr) << /*Unstable=*/true;
+  }
+

rjmccall wrote:
> pcc wrote:
> > rjmccall wrote:
> > > No, ms_struct is a request for a specific ABI; this would be a conflict.
> > Fair point. I think we can handle this by making `ms_struct` imply 
> > `stable_abi` in the same way that `uuid` would imply `stable_abi`. Then 
> > there would be a conflict between `ms_struct` and `unstable_abi`, and we 
> > would infer the correct ABI in implied namespaces.
> > 
> > Are there any other attributes like that? I could only find `vecreturn` and 
> > `novtable`.
> I can't think of anything else that's a conflict.  vecreturn affects a 
> different aspect of the ABI.  novtable is advisory and orthogonal.  
> Alignment/packed attributes should still be honored under an unstable ABI.
Thanks, I've added conflicts for `ms_struct` and `uuid`.


http://reviews.llvm.org/D17893



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

Re: [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>;

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();
+  bool HasUnstableAttr = Record->hasAttr();
+  if (HasStableAttr && HasUnstableAttr) {
+Diag(Record->getLocation(), diag::err_abi_mismatch) << Record;
+Diag(Record->getAttr()->getLocation(),
+ diag::note_abi_stability_attr) << /*Unstable=*/false;
+Diag(Record->getAttr()->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



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

Re: [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: lib/Sema/SemaDeclCXX.cpp:4935-4943
@@ +4934,11 @@
+
+  bool HasStableAttr = Record->hasAttr();
+  bool HasUnstableAttr = Record->hasAttr();
+  if (HasStableAttr && HasUnstableAttr) {
+Diag(Record->getLocation(), diag::err_abi_mismatch) << Record;
+Diag(Record->getAttr()->getLocation(),
+ diag::note_abi_stability_attr) << /*Unstable=*/false;
+Diag(Record->getAttr()->getLocation(),
+ diag::note_abi_stability_attr) << /*Unstable=*/true;
+  }
+

rjmccall wrote:
> No, ms_struct is a request for a specific ABI; this would be a conflict.
Fair point. I think we can handle this by making `ms_struct` imply `stable_abi` 
in the same way that `uuid` would imply `stable_abi`. Then there would be a 
conflict between `ms_struct` and `unstable_abi`, and we would infer the correct 
ABI in implied namespaces.

Are there any other attributes like that? I could only find `vecreturn` and 
`novtable`.


http://reviews.llvm.org/D17893



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

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

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

Attribute side of things looks good, but I leave it to others to decide if the 
ABI functionality makes sense (it does to me, but I'm no ABI expert).


http://reviews.llvm.org/D17893



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

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

[Mehdi AMINI via cfe-commits]

joker.eph added inline comments.


Comment at: lib/Sema/SemaDeclCXX.cpp:4999
@@ +4998,3 @@
+return;
+  }
+

joker.eph wrote:
> Isn't this correct by the loop which starts with `DC = InnermostExternalDC`?
`s/correct/already covered/`


http://reviews.llvm.org/D17893



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

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

[John McCall via cfe-commits]

rjmccall added inline comments.


Comment at: lib/Sema/SemaDeclCXX.cpp:4935-4943
@@ +4934,11 @@
+
+  bool HasStableAttr = Record->hasAttr();
+  bool HasUnstableAttr = Record->hasAttr();
+  if (HasStableAttr && HasUnstableAttr) {
+Diag(Record->getLocation(), diag::err_abi_mismatch) << Record;
+Diag(Record->getAttr()->getLocation(),
+ diag::note_abi_stability_attr) << /*Unstable=*/false;
+Diag(Record->getAttr()->getLocation(),
+ diag::note_abi_stability_attr) << /*Unstable=*/true;
+  }
+

No, ms_struct is a request for a specific ABI; this would be a conflict.


http://reviews.llvm.org/D17893



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

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

[Richard Smith via cfe-commits]

rsmith 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>;

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.)


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.

Can we exit early if no unstable class ABI flags were passed?


Comment at: lib/Sema/SemaDeclCXX.cpp:4935-4943
@@ +4934,11 @@
+
+  bool HasStableAttr = Record->hasAttr();
+  bool HasUnstableAttr = Record->hasAttr();
+  if (HasStableAttr && HasUnstableAttr) {
+Diag(Record->getLocation(), diag::err_abi_mismatch) << Record;
+Diag(Record->getAttr()->getLocation(),
+ diag::note_abi_stability_attr) << /*Unstable=*/false;
+Diag(Record->getAttr()->getLocation(),
+ diag::note_abi_stability_attr) << /*Unstable=*/true;
+  }
+

Should you also diagnose conflicts with other struct ABI attributes like 
`ms_struct`?


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()) {

`-Weverything` users will not like this.


http://reviews.llvm.org/D17893



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

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

[Mehdi AMINI via cfe-commits]

joker.eph added a comment.

LGTM, but I'm not a clang expert, so if Richard or someone else could 
double-check?



Comment at: lib/Sema/SemaDeclCXX.cpp:4999
@@ +4998,3 @@
+return;
+  }
+

Isn't this correct by the loop which starts with `DC = InnermostExternalDC`?


http://reviews.llvm.org/D17893



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

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

[Peter Collingbourne via cfe-commits]

pcc updated this revision to Diff 50668.
pcc added a comment.

- Add a test for passing arguments to the attribute


http://reviews.llvm.org/D17893

Files:
  include/clang/AST/DeclCXX.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/LangOptions.h
  include/clang/Driver/Options.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/Basic/LangOptions.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  test/Driver/funstable-cxx-abi.cpp
  test/SemaCXX/Inputs/unstable-abi-list-global1.txt
  test/SemaCXX/Inputs/unstable-abi-list-global2.txt
  test/SemaCXX/Inputs/unstable-abi-list.txt
  test/SemaCXX/attr-stability.cpp
  test/SemaCXX/unstable-cxx-abi-global1.cpp
  test/SemaCXX/unstable-cxx-abi-global2.cpp
  test/SemaCXX/unstable-cxx-abi-warning.cpp
  test/SemaCXX/unstable-cxx-abi.cpp

Index: test/SemaCXX/unstable-cxx-abi.cpp
===
--- /dev/null
+++ test/SemaCXX/unstable-cxx-abi.cpp
@@ -0,0 +1,91 @@
+// RUN: %clang_cc1 %s -std=c++11 -fsyntax-only -verify -funstable-c++-abi-classes=%S/Inputs/unstable-abi-list.txt -Wstable-c++-abi
+
+struct stable { // expected-note 3{{base 'stable' uses the stable ABI}} expected-warning {{stable C++ ABI was inferred for class 'stable'}} expected-note-re {{add the global namespace to file {{.*}} to silence this warning}}
+  virtual void f();
+};
+struct [[clang::unstable_abi]] unstable { // expected-note 3{{base 'unstable' uses the unstable ABI}}
+  virtual void f();
+};
+
+struct
+[[clang::stable_abi]] // expected-note {{stable ABI specified by attribute}}
+[[clang::unstable_abi]] // expected-note {{unstable ABI specified by attribute}}
+bistable { // expected-error {{inconsistent ABI for class 'bistable'}}
+  virtual void f();
+};
+
+struct mixed_bases : stable, unstable {}; // expected-error {{inconsistent ABI for class 'mixed_bases'}}
+struct mixed_base_vbase : stable, virtual unstable {}; // expected-error {{inconsistent ABI for class 'mixed_base_vbase'}}
+
+struct
+[[clang::unstable_abi]] // expected-note {{unstable ABI specified by attribute}}
+mixed_attr_base : stable { // expected-error {{inconsistent ABI for class 'mixed_attr_base'}}
+};
+
+struct
+[[clang::stable_abi]] // expected-note {{stable ABI specified by attribute}}
+mixed_attr_base2 : unstable { // expected-error {{inconsistent ABI for class 'mixed_attr_base2'}}
+};
+
+namespace std {
+
+struct unstable {
+  virtual void f();
+};
+
+}
+
+namespace std {
+
+namespace {
+
+struct unstable2 {
+  virtual void f();
+};
+
+namespace foo {
+
+struct unstable {
+  virtual void f();
+};
+
+}
+
+}
+
+namespace bar {
+
+struct stable { // expected-warning {{stable C++ ABI was inferred for class 'stable'}} expected-note-re {{add namespace 'std::bar' to file {{.*}} to silence this warning}}
+  virtual void f();
+};
+
+}
+
+struct non_dynamic {
+  struct unstable3 {
+virtual void f();
+  };
+  void mf() {
+struct unstable4 {
+  virtual void f();
+};
+struct unstable_derived : unstable, unstable4 {};
+  }
+};
+
+void f() {
+  struct unstable5 {
+virtual void f();
+  };
+  struct unstable_derived : unstable, unstable5 {};
+}
+
+}
+
+struct stable2 : std::non_dynamic { // expected-warning {{stable C++ ABI was inferred for class 'stable2'}}  expected-note-re {{add the global namespace to file {{.*}} to silence this warning}}
+  virtual void f();
+};
+
+struct unstable_derived : unstable, std::unstable2, std::foo::unstable,
+  std::non_dynamic::unstable3 {};
+struct stable_derived : stable, std::bar::stable, stable2 {};
Index: test/SemaCXX/unstable-cxx-abi-warning.cpp
===
--- /dev/null
+++ test/SemaCXX/unstable-cxx-abi-warning.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 %s -std=c++11 -fsyntax-only -verify -Wstable-c++-abi
+// RUN: %clang_cc1 %s -std=c++11 -fsyntax-only -verify -Wstable-c++-abi -funstable-c++-abi-classes=%S/Inputs/unstable-abi-list.txt -funstable-c++-abi-classes=%S/Inputs/unstable-abi-list-global1.txt
+
+namespace ns {
+
+class non_dynamic {};
+
+class dynamic { // expected-warning {{stable C++ ABI was inferred for class 'dynamic'}} expected-note {{add attribute clang::unstable_abi or clang::stable_abi to silence this warning}}
+  virtual void f();
+};
+
+}
Index: test/SemaCXX/unstable-cxx-abi-global2.cpp
===
--- /dev/null
+++ test/SemaCXX/unstable-cxx-abi-global2.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 %s -std=c++11 -fsyntax-only -verify -funstable-c++-abi-classes
+// RUN: %clang_cc1 %s -std=c++11 -fsyntax-only -verify -funstable-c++-abi-classes=%S/Inputs/unstable-abi-list-global2.txt
+
+namespace foo {
+
+struct unstable { // expected-note {{base 'unstable' uses the unstable ABI}}
+  virtual void f();
+};
+

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

[Peter Collingbourne via cfe-commits]

pcc added a comment.

> Missing some attribute-related tests like attaching the attribute to 
> something other than a record, or passing arguments to the attribute.


Added.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8345
@@ +8344,3 @@
+def note_add_unstable_abi_attr : Note<
+  "add attribute clang::unstable_abi or clang::stable_abi to silence this 
warning">;
+

aaron.ballman wrote:
> Should this also be a %select, or can you really add either attribute to 
> silence the warning?
The latter. I gave this a better name to make things clear.


Comment at: lib/Sema/SemaDeclCXX.cpp:4896
@@ +4895,3 @@
+
+  // No need to do this for non-dynamic classes.
+  if (!Record->isDynamicClass())

aaron.ballman wrote:
> Since the attributes do nothing for a non-dynamic class, should the user get 
> a warning if the place the attribute on one (so they know it does nothing)?
I suppose so; done.


http://reviews.llvm.org/D17893



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

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

[Peter Collingbourne via cfe-commits]

pcc updated this revision to Diff 50667.
pcc marked 3 inline comments as done.
pcc added a comment.

- Address review comments


http://reviews.llvm.org/D17893

Files:
  include/clang/AST/DeclCXX.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/LangOptions.h
  include/clang/Driver/Options.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/Basic/LangOptions.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  test/Driver/funstable-cxx-abi.cpp
  test/SemaCXX/Inputs/unstable-abi-list-global1.txt
  test/SemaCXX/Inputs/unstable-abi-list-global2.txt
  test/SemaCXX/Inputs/unstable-abi-list.txt
  test/SemaCXX/attr-stability.cpp
  test/SemaCXX/unstable-cxx-abi-global1.cpp
  test/SemaCXX/unstable-cxx-abi-global2.cpp
  test/SemaCXX/unstable-cxx-abi-warning.cpp
  test/SemaCXX/unstable-cxx-abi.cpp

Index: test/SemaCXX/unstable-cxx-abi.cpp
===
--- /dev/null
+++ test/SemaCXX/unstable-cxx-abi.cpp
@@ -0,0 +1,91 @@
+// RUN: %clang_cc1 %s -std=c++11 -fsyntax-only -verify -funstable-c++-abi-classes=%S/Inputs/unstable-abi-list.txt -Wstable-c++-abi
+
+struct stable { // expected-note 3{{base 'stable' uses the stable ABI}} expected-warning {{stable C++ ABI was inferred for class 'stable'}} expected-note-re {{add the global namespace to file {{.*}} to silence this warning}}
+  virtual void f();
+};
+struct [[clang::unstable_abi]] unstable { // expected-note 3{{base 'unstable' uses the unstable ABI}}
+  virtual void f();
+};
+
+struct
+[[clang::stable_abi]] // expected-note {{stable ABI specified by attribute}}
+[[clang::unstable_abi]] // expected-note {{unstable ABI specified by attribute}}
+bistable { // expected-error {{inconsistent ABI for class 'bistable'}}
+  virtual void f();
+};
+
+struct mixed_bases : stable, unstable {}; // expected-error {{inconsistent ABI for class 'mixed_bases'}}
+struct mixed_base_vbase : stable, virtual unstable {}; // expected-error {{inconsistent ABI for class 'mixed_base_vbase'}}
+
+struct
+[[clang::unstable_abi]] // expected-note {{unstable ABI specified by attribute}}
+mixed_attr_base : stable { // expected-error {{inconsistent ABI for class 'mixed_attr_base'}}
+};
+
+struct
+[[clang::stable_abi]] // expected-note {{stable ABI specified by attribute}}
+mixed_attr_base2 : unstable { // expected-error {{inconsistent ABI for class 'mixed_attr_base2'}}
+};
+
+namespace std {
+
+struct unstable {
+  virtual void f();
+};
+
+}
+
+namespace std {
+
+namespace {
+
+struct unstable2 {
+  virtual void f();
+};
+
+namespace foo {
+
+struct unstable {
+  virtual void f();
+};
+
+}
+
+}
+
+namespace bar {
+
+struct stable { // expected-warning {{stable C++ ABI was inferred for class 'stable'}} expected-note-re {{add namespace 'std::bar' to file {{.*}} to silence this warning}}
+  virtual void f();
+};
+
+}
+
+struct non_dynamic {
+  struct unstable3 {
+virtual void f();
+  };
+  void mf() {
+struct unstable4 {
+  virtual void f();
+};
+struct unstable_derived : unstable, unstable4 {};
+  }
+};
+
+void f() {
+  struct unstable5 {
+virtual void f();
+  };
+  struct unstable_derived : unstable, unstable5 {};
+}
+
+}
+
+struct stable2 : std::non_dynamic { // expected-warning {{stable C++ ABI was inferred for class 'stable2'}}  expected-note-re {{add the global namespace to file {{.*}} to silence this warning}}
+  virtual void f();
+};
+
+struct unstable_derived : unstable, std::unstable2, std::foo::unstable,
+  std::non_dynamic::unstable3 {};
+struct stable_derived : stable, std::bar::stable, stable2 {};
Index: test/SemaCXX/unstable-cxx-abi-warning.cpp
===
--- /dev/null
+++ test/SemaCXX/unstable-cxx-abi-warning.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 %s -std=c++11 -fsyntax-only -verify -Wstable-c++-abi
+// RUN: %clang_cc1 %s -std=c++11 -fsyntax-only -verify -Wstable-c++-abi -funstable-c++-abi-classes=%S/Inputs/unstable-abi-list.txt -funstable-c++-abi-classes=%S/Inputs/unstable-abi-list-global1.txt
+
+namespace ns {
+
+class non_dynamic {};
+
+class dynamic { // expected-warning {{stable C++ ABI was inferred for class 'dynamic'}} expected-note {{add attribute clang::unstable_abi or clang::stable_abi to silence this warning}}
+  virtual void f();
+};
+
+}
Index: test/SemaCXX/unstable-cxx-abi-global2.cpp
===
--- /dev/null
+++ test/SemaCXX/unstable-cxx-abi-global2.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 %s -std=c++11 -fsyntax-only -verify -funstable-c++-abi-classes
+// RUN: %clang_cc1 %s -std=c++11 -fsyntax-only -verify -funstable-c++-abi-classes=%S/Inputs/unstable-abi-list-global2.txt
+
+namespace foo {
+
+struct unstable { // expected-note {{base 'unstable' uses the unstable ABI}}
+  virtual void 

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

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

Missing some attribute-related tests like attaching the attribute to something 
other than a record, or passing arguments to the attribute.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8332
@@ +8331,3 @@
+  "base %0 uses the stable ABI">;
+def note_unstable_abi_base : Note<
+  "base %0 uses the unstable ABI">;

It would be good to combine these into one Note using %select.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8336
@@ +8335,3 @@
+  "stable ABI specified by attribute">;
+def note_unstable_abi_attr : Note<
+  "unstable ABI specified by attribute">;

Same for these.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8345
@@ +8344,3 @@
+def note_add_unstable_abi_attr : Note<
+  "add attribute clang::unstable_abi or clang::stable_abi to silence this 
warning">;
+

Should this also be a %select, or can you really add either attribute to 
silence the warning?


Comment at: lib/Sema/SemaDeclCXX.cpp:4896
@@ +4895,3 @@
+
+  // No need to do this for non-dynamic classes.
+  if (!Record->isDynamicClass())

Since the attributes do nothing for a non-dynamic class, should the user get a 
warning if the place the attribute on one (so they know it does nothing)?


http://reviews.llvm.org/D17893



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

Re: [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/Attr.td:1543
@@ +1542,3 @@
+  let Subjects = SubjectList<[Record]>;
+  let Documentation = [Undocumented];
+}

aaron.ballman wrote:
> No new undocumented attributes, please. Same below.
Yes, sorry, forgot to go back and add documentation for these attributes. Will 
do. (Reckon it's not worth documenting the feature itself until it actually 
does something.)


http://reviews.llvm.org/D17893



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

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

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.

I may have more comments when I catch up on email next week. Just a drive-by as 
I added myself as reviewer.



Comment at: include/clang/Basic/Attr.td:1543
@@ +1542,3 @@
+  let Subjects = SubjectList<[Record]>;
+  let Documentation = [Undocumented];
+}

No new undocumented attributes, please. Same below.


http://reviews.llvm.org/D17893



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