Page MenuHomePhabricator

Add new warning knob for unknown attribute namespaces
AcceptedPublic

Authored by aaron.ballman on Apr 18 2019, 8:09 AM.

Details

Summary

Being noisy when ignoring unknown attributes is important because it can be very difficult for a user to tell the difference between silently ignoring an attribute and accepting an attribute that doesn't have observable semantics for a particular compiland. However, there are times when the user has a code base with vendor-scoped attributes the implementation doesn't support -- for instance, a static analyzer may add a bunch of [[analyzer::attr]] attributes that Clang doesn't know about.

To support this scenario, this patch allows you to ignore unknown attributes only when the attribute is a scoped attribute in a namespace for which Clang does not support any attributes. It is expected to be used as -Wno-unknown-attributes -Wunknown-attribute-namespaces so that unknown attribute warnings are disabled except for unknown attributes in unknown namespaces. Note that attributes in the standards namespace are considered to always be known to Clang and thus will still be warned on even when passing -Wunknown-attribute-namespaces this way. e.g.,

[[gnu::unknown]] int I; // Still diagnosed because we support other attributes in the gnu namespace.
[[unknown::unknown]] int J; // Is not diagnosed because we do not support any attributes in the unknown namespace.
[[unknown]] int K; // Still diagnosed because Clang should know about all attributes in the global attribute namespace.

If there is a better way to surface the warning flags, I'm happy to explore it.

Diff Detail

Event Timeline

aaron.ballman created this revision.Apr 18 2019, 8:09 AM

Thanks for coming up with this patch.

I am not sure whether this solves the original feature requester's problem entirely. It assumes that clang supports all attributes of the std and gnu namespaces. That is, the warning for [[gnu::future_gcc_extension]] cannot be disabled without also disabling the warning for [[gnu::missspelled]].

Thanks for coming up with this patch.

I am not sure whether this solves the original feature requester's problem entirely. It assumes that clang supports all attributes of the std and gnu namespaces. That is, the warning for [[gnu::future_gcc_extension]] cannot be disabled without also disabling the warning for [[gnu::missspelled]].

Correct -- it was not intended to support the original requester's problem entirely, on purpose (and the OP said this was okay for their needs). We want to diagnose unknown attributes in vendor namespaces we support because otherwise the user cannot tell the difference between [[gnu::we_support_this]] and [[gnu::we_dont_support_this]] without considerable effort. That confusion is significantly reduced when we don't support an entire vendor namespace and there are compelling use cases for that situation (such as static analysis vendors introducing attributes to control the analyzer). If you need per-attribute control, you should use the preprocessor with __has_cpp_attribute.

dblaikie added inline comments.Apr 18 2019, 11:02 AM
lib/Sema/SemaDeclAttr.cpp
8623

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

8623–8624

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

8629–8630

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

utils/TableGen/ClangAttrEmitter.cpp
2553

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

aaron.ballman marked 7 inline comments as done.Apr 18 2019, 11:23 AM
aaron.ballman added inline comments.
lib/Sema/SemaDeclAttr.cpp
8623

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.

8623–8624

I think it's pretty awkward either way, so I'll go with your approach.

8629–8630

Good catch -- it already does that for me. I'll remove.

utils/TableGen/ClangAttrEmitter.cpp
2553

Good catch, I missed renaming it after a signature change where it was accepting an iterator instead of the container directly.

aaron.ballman marked 3 inline comments as done.

Updated based on review feedback.

dblaikie accepted this revision.Apr 18 2019, 11:36 AM

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

This revision is now accepted and ready to land.Apr 18 2019, 11:36 AM

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

aaron.ballman marked 2 inline comments as done.Apr 20 2019, 8:55 AM

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.

test/SemaCXX/attr-cxx0x.cpp
64–67

This logic is backwards. We want to suppress the warning in this case, not issue it. :-)

72

This is the case we wanted warned on.