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

Sat Apr 20 08:58:44 PDT 2019

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

In D60872#1473205 <https://reviews.llvm.org/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-cxx0x.cpp:64-67
+#if defined(UNKNOWN_ATTRS) || defined(UNKNOWN_NAMESPACE)
+// expected-warning at +2 {{unknown attribute 'bobble' ignored}}
+#endif
+[[frobble::bobble]] int unknown2;
----------------
This logic is backwards. We want to suppress the warning in this case, not issue it. :-)

================
Comment at: test/SemaCXX/attr-cxx0x.cpp:72
+#endif
+[[gsl::unknown_attribute]] int unknown3;
----------------
This is the case we wanted warned on.

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

https://reviews.llvm.org/D60872