[PATCH] D60872: Add new warning knob for unknown attribute namespaces

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 2 inline comments as done.
aaron.ballman added a comment.

In D60872#1473205 , @rsmith wrote:

> Hmm. So there are a few different situations where we might meet an unknown 
> attribute (I'm sure I missed some):
>
> 1. The attribute had a typo in it (eg: [[clagn::fallthrough]]).
> 2. The attribute has semantic effects (ignoring it is incorrect and will -- 
> at best -- not compile or -- at worst -- generate a subtly broken program, 
> eg: [[gnu::aligned]] or [[gnu::target]]).
> 3. The attribute has important effects (ignoring it is probably correct but 
> suboptimal in some way, eg: [[no_unique_address]], [[gnu::packed]], ...).
> 4. The attribute has unimportant effects (is entirely safely ignorable) or is 
> only intended to affect the behavior of a different tool (eg: gsl, static 
> analyzer, clang-tidy, etc).
>
>   I think the ideal would be to warn on unknown attributes in cases 1 and 2, 
> optionally warn on unknown attributes in case 3, and to not warn in case 4. 
> Without user hints, we can't tell which is which in general.
>
>   "Do not warn on unknown namespaces" gives us some part of not warning on 
> case 4. However, it also turns off warnings in cases 1-3 (eg, we won't warn 
> on `[[gcc::noinline]]` as a typo for `[[gnu::noinline]]`),


Disabling warnings sometimes means you lose out on good information. It's a 
good point to keep in mind, but at the same time, it's also the point to the 
diagnostic to not warn on attributes in unknown namespaces, so the behavior 
doesn't seem all that surprising to me.

> and doesn't turn off warnings for case-4 attributes in, say, namespace `gnu`, 
> so it's somewhat imperfect. I think it's also going to be surprising that the 
> clang release that starts parsing a [[gsl::*]] attribute also starts warning 
> on other ("unknown") [[gsl::*]] attributes for people in this new mode.
> 
> It seems to me that we can achieve the ideal (don't warn on safely-ignorable 
> unknown attributes, but warn on all other unknown attributes) if we ask the 
> user to give us a list of safely-ignorable attributes and attribute 
> namespaces that they intend to use. (That even lets us do typo-correction for 
> attributes we don't support.) In the cfe-dev thread, there was mention that 
> there are hundreds of attributes supported by clang, and so hundreds of 
> attributes would need to be listed, but that doesn't follow: you only need to 
> list the attributes that you actually intend to use. That should be a much 
> smaller list (especially once modules adoption picks up, and you don't need 
> to list those attributes used by your dependencies).

I am pretty strongly opposed to having the user specify the list of attributes 
on the command line. We spend a lot of time worrying about users who cannot 
spell, so giving them another place to make typos (but one that pushes them 
towards needing more advanced build strategies like response files) seems 
counter-intuitive. We also would have to solve some interesting issues like the 
user specifying "foo_attr" on the command line, using 
`__attribute__((foo_attr))` in code and having it warned on as an unknown 
attribute because we happen to know about `__declspec(foo_attr)` but not the 
`__attribute__((foo_attr))` spelling, etc.

I think am coming at this from a different angle -- the user's code is compiled 
with 1 compiler (Clang) vs N compilers (including Clang). For a user who is 
using N compilers, they should be using the preprocessor to solve this issue. 
This is the longstanding status quo solution and is the reason we standardized 
`__has_cpp_attribute` and friends, and I'm not strongly motivated to maintain 
an additional solution that pushes the issue into the build system for only one 
of their N compilers because the user will still have to come up with ad hoc 
solutions for the other N - 1 compilers, so this doesn't seem like it gets them 
much (compared to the macro approach, which can work for all compilers). What I 
am strongly motivated to support are users who are using Clang as their only 
compiler. These users have no reason to need the macro boilerplate, except for 
3rd party tooling that is entirely unknown to Clang (static analyzers being a 
primary source). Given that well-behaved 3rd party tools should only be 
introducing attributes in their own vendor namespace (a namespace Clang is 
unlikely to know anything about), and we have no reason to believe 3rd parties 
won't come up with numerous attributes the user may want to use, it seems very 
useful to allow users to inhibit the unknown attribute warnings for those 
vendor namespaces without making the user maintain a separate, manual list of 
attributes in the build system. I also don't see Clang introducing attributes 
from those vendor namespaces with any frequency, so that's why I'm not too 
worried about upgrades introducing new warnings.




Comment at: test/SemaCXX/attr-cxx

[PATCH] D60872: Add new warning knob for unknown attribute namespaces

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Hmm. So there are a few different situations where we might meet an unknown 
attribute (I'm sure I missed some):

1. The attribute had a typo in it (eg: [[clagn::fallthrough]]).
2. The attribute has semantic effects (ignoring it is incorrect and will -- at 
best -- not compile or -- at worst -- generate a subtly broken program, eg: 
[[gnu::aligned]] or [[gnu::target]]).
3. The attribute has important effects (ignoring it is probably correct but 
suboptimal in some way, eg: [[no_unique_address]], [[gnu::packed]], ...).
4. The attribute has unimportant effects (is entirely safely ignorable) or is 
only intended to affect the behavior of a different tool (eg: gsl, static 
analyzer, clang-tidy, etc).

I think the ideal would be to warn on unknown attributes in cases 1 and 2, 
optionally warn on unknown attributes in case 3, and to not warn in case 4. 
Without user hints, we can't tell which is which in general.

"Do not warn on unknown namespaces" gives us some part of not warning on case 
4. However, it also turns off warnings in cases 1-3 (eg, we won't warn on 
`[[gcc::noinline]]` as a typo for `[[gnu::noinline]]`), and doesn't turn off 
warnings for case-4 attributes in, say, namespace `gnu`, so it's somewhat 
imperfect. I think it's also going to be surprising that the clang release that 
starts parsing a [[gsl::*]] attribute also starts warning on other ("unknown") 
[[gsl::*]] attributes for people in this new mode.

It seems to me that we can achieve the ideal (don't warn on safely-ignorable 
unknown attributes, but warn on all other unknown attributes) if we ask the 
user to give us a list of safely-ignorable attributes and attribute namespaces 
that they intend to use. (That even lets us do typo-correction for attributes 
we don't support.) In the cfe-dev thread, there was mention that there are 
hundreds of attributes supported by clang, and so hundreds of attributes would 
need to be listed, but that doesn't follow: you only need to list the 
attributes that you actually intend to use. That should be a much smaller list 
(especially once modules adoption picks up, and you don't need to list those 
attributes used by your dependencies).


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

https://reviews.llvm.org/D60872



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

[PATCH] D60872: Add new warning knob for unknown attribute namespaces

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Seems pretty good to me - if you'd like to wait for more/other feedback from 
@rsmith or the like, that's OK too.


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

https://reviews.llvm.org/D60872



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

[PATCH] D60872: Add new warning knob for unknown attribute namespaces

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 195790.
aaron.ballman marked 3 inline comments as done.
aaron.ballman added a comment.

Updated based on review feedback.


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

https://reviews.llvm.org/D60872

Files:
  include/clang/Basic/DiagnosticCommonKinds.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Sema/Sema.h
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaStmtAttr.cpp
  lib/Sema/SemaType.cpp
  test/SemaCXX/attr-cxx0x.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -17,7 +17,6 @@
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -2551,6 +2550,16 @@
   ::emitAttrList(OS, Descriptor.MacroName, Attrs);
 }
 
+void addAttrNamespacesTo(std::set &S) const {
+  for (const Record *R : Attrs) {
+std::vector Spellings = GetFlattenedSpellings(*R);
+for (const auto &Spell : Spellings) {
+  if (!Spell.nameSpace().empty())
+S.insert(Spell.nameSpace());
+}
+  }
+}
+
 void classifyAttrOnRoot(Record *Attr) {
   bool result = classifyAttr(Attr);
   assert(result && "failed to classify on root"); (void) result;
@@ -2621,6 +2630,19 @@
 #endif
 }
 
+void emitAttrNamespaces(raw_ostream &OS) const {
+  // Gather the unique set of attribute namespaces Clang knows about. We
+  // want this set to be ordered so we can do efficient lookups.
+  std::set Namespaces;
+  for (const auto &Class : Classes) {
+Class->addAttrNamespacesTo(Namespaces);
+  }
+  // Emit the list of namespaces.
+  for (const auto &Namespace : Namespaces) {
+OS << "ATTR_NAMESPACE(" << Namespace << ")\n";
+  }
+}
+
 void emitDefaultDefines(raw_ostream &OS) const {
   for (auto &Class : Classes) {
 Class->emitDefaultDefines(OS);
@@ -2682,6 +2704,7 @@
   // Add defaulting macro definitions.
   Hierarchy.emitDefaultDefines(OS);
   emitDefaultDefine(OS, "PRAGMA_SPELLING_ATTR", nullptr);
+  emitDefaultDefine(OS, "ATTR_NAMESPACE", nullptr);
 
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
   std::vector PragmaAttrs;
@@ -2702,6 +2725,7 @@
 
   // Emit the ad hoc groups.
   emitAttrList(OS, "PRAGMA_SPELLING_ATTR", PragmaAttrs);
+  Hierarchy.emitAttrNamespaces(OS);
 
   // Emit the attribute ranges.
   OS << "#ifdef ATTR_RANGE\n";
@@ -2711,6 +2735,7 @@
 
   Hierarchy.emitUndefs(OS);
   OS << "#undef PRAGMA_SPELLING_ATTR\n";
+  OS << "#undef ATTR_NAMESPACE\n";
 }
 
 // Emits the enumeration list for attributes.
Index: test/SemaCXX/attr-cxx0x.cpp
===
--- test/SemaCXX/attr-cxx0x.cpp
+++ test/SemaCXX/attr-cxx0x.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -fcxx-exceptions -verify -pedantic -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -fcxx-exceptions -verify -pedantic -DUNKNOWN_ATTRS -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -fcxx-exceptions -verify -pedantic -Wno-unknown-attributes -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -fcxx-exceptions -verify -pedantic -Wno-unknown-attributes -Wunknown-attribute-namespaces -DUNKNOWN_NAMESPACE -std=c++11 %s
 
 int align_illegal alignas(3); //expected-error {{requested alignment is not a power of 2}}
 char align_big alignas(int);
@@ -46,7 +48,25 @@
 
 static_assert(alignof(int(int)) >= 1, "alignof(function) not positive"); // expected-error{{invalid application of 'alignof' to a function type}}
 
-[[__carries_dependency__]]  // expected-warning{{unknown attribute '__carries_dependency__' ignored}}
+#if defined(UNKNOWN_ATTRS) || defined(UNKNOWN_NAMESPACE)
+// expected-warning@+2 {{unknown attribute '__carries_dependency__' ignored}}
+#endif
+[[__carries_dependency__]]
 void func(void);
 
 alignas(4) auto PR19252 = 0;
+
+#if defined(UNKNOWN_ATTRS) || defined(UNKNOWN_NAMESPACE)
+// expected-warning@+2 {{unknown attribute 'frobble' ignored}}
+#endif
+[[frobble]] int unknown1;
+
+#if defined(UNKNOWN_ATTRS) || defined(UNKNOWN_NAMESPACE)
+// expected-warning@+2 {{unknown attribute 'bobble' ignored}}
+#endif
+[[frobble::bobble]] int unknown2;
+
+#ifdef UNKNOWN_ATTRS
+// expected-warning@+2 {{unknown attribute 'unknown_attribute' ignored}}
+#endif
+[[gsl::unknown_attribute]] int unknown3;
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7441,9 +7441,7 @@
 
 case ParsedAttr::UnknownAttribute:
   if (attr.isCXX11Attribute() && TAL == TAL_DeclChunk)
-state.getSema()

[PATCH] D60872: Add new warning knob for unknown attribute namespaces

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 7 inline comments as done.
aaron.ballman added inline comments.



Comment at: lib/Sema/SemaDeclAttr.cpp:8623
+  } else if (A.hasScope()) {
+#define ATTR(A)
+#define ATTR_NAMESPACE(A) .Case(#A, false)

dblaikie wrote:
> dblaikie wrote:
> > Not sure how it's done elsewhere - but I'd sink these #defines down to 
> > immediately before the #include
> I think most other uses of .inc files only #define the macros they need to - 
> assuming the others aren't defined, rather than explicitly providing an empty 
> definition? (but I'm not sure - I guess that means teh .inc file needs a 
> #ifndef X \ #define X #endif - but perhaps .inc files usually have those?)
This file is generated without a default for `ATTR`, so there are a few places 
where we have to define it to an empty macro. I could add it to the emitter and 
remove a few of these spurious definitions, but that can be done in a follow-up.



Comment at: lib/Sema/SemaDeclAttr.cpp:8623-8624
+  } else if (A.hasScope()) {
+#define ATTR(A)
+#define ATTR_NAMESPACE(A) .Case(#A, false)
+if (llvm::StringSwitch(A.getScopeName()->getName())

aaron.ballman wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > Not sure how it's done elsewhere - but I'd sink these #defines down to 
> > > immediately before the #include
> > I think most other uses of .inc files only #define the macros they need to 
> > - assuming the others aren't defined, rather than explicitly providing an 
> > empty definition? (but I'm not sure - I guess that means teh .inc file 
> > needs a #ifndef X \ #define X #endif - but perhaps .inc files usually have 
> > those?)
> This file is generated without a default for `ATTR`, so there are a few 
> places where we have to define it to an empty macro. I could add it to the 
> emitter and remove a few of these spurious definitions, but that can be done 
> in a follow-up.
I think it's pretty awkward either way, so I'll go with your approach.



Comment at: lib/Sema/SemaDeclAttr.cpp:8629-8630
+  Warning = diag::warn_unknown_attribute_namespace_ignored;
+#undef ATTR_NAMESPACE
+#undef ATTR
+  } else if (A.isC2xAttribute() || A.isCXX11Attribute()) {

dblaikie wrote:
> Should AttrList.inc handle undefing all its attributes - so all its users 
> don't have to?
Good catch -- it already does that for me. I'll remove.



Comment at: utils/TableGen/ClangAttrEmitter.cpp:2553
 
+void copyAttrNamespaces(std::set &S) const {
+  for (const Record *R : Attrs) {

dblaikie wrote:
> Maybe "addAttrNamespacesTo" (not clear whether this would overwrite the 
> contens of the std::set, or add to it)
Good catch, I missed renaming it after a signature change where it was 
accepting an iterator instead of the container directly.


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

https://reviews.llvm.org/D60872



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

[PATCH] D60872: Add new warning knob for unknown attribute namespaces

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: lib/Sema/SemaDeclAttr.cpp:8623
+  } else if (A.hasScope()) {
+#define ATTR(A)
+#define ATTR_NAMESPACE(A) .Case(#A, false)

dblaikie wrote:
> Not sure how it's done elsewhere - but I'd sink these #defines down to 
> immediately before the #include
I think most other uses of .inc files only #define the macros they need to - 
assuming the others aren't defined, rather than explicitly providing an empty 
definition? (but I'm not sure - I guess that means teh .inc file needs a 
#ifndef X \ #define X #endif - but perhaps .inc files usually have those?)



Comment at: lib/Sema/SemaDeclAttr.cpp:8623-8624
+  } else if (A.hasScope()) {
+#define ATTR(A)
+#define ATTR_NAMESPACE(A) .Case(#A, false)
+if (llvm::StringSwitch(A.getScopeName()->getName())

Not sure how it's done elsewhere - but I'd sink these #defines down to 
immediately before the #include



Comment at: lib/Sema/SemaDeclAttr.cpp:8629-8630
+  Warning = diag::warn_unknown_attribute_namespace_ignored;
+#undef ATTR_NAMESPACE
+#undef ATTR
+  } else if (A.isC2xAttribute() || A.isCXX11Attribute()) {

Should AttrList.inc handle undefing all its attributes - so all its users don't 
have to?



Comment at: utils/TableGen/ClangAttrEmitter.cpp:2553
 
+void copyAttrNamespaces(std::set &S) const {
+  for (const Record *R : Attrs) {

Maybe "addAttrNamespacesTo" (not clear whether this would overwrite the contens 
of the std::set, or add to it)


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

https://reviews.llvm.org/D60872



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