[PATCH] D93110: [analyzer] Implement a first version of suppressions via attributes

Tue Dec 22 06:54:48 PST 2020

aaron.ballman added a comment.

In D93110#2460375 <https://reviews.llvm.org/D93110#2460375>, @vsavchenko wrote:

> In D93110#2458613 <https://reviews.llvm.org/D93110#2458613>, @aaron.ballman wrote:
>
>> Have you explored how this attribute will work with clang frontend diagnostics or clang-tidy diagnostics?
>
> Actually, this attribute is not used anywhere in the codebase (even in `clang-tidy`!).  I think that it can be used for warning suppressions as well, it is more comfortable than pragmas IMO.  However, I think that the problem of false positive or, for that matter, suppressions is much more visible with static analysis tools.

Agreed that an attribute will feel like a more natural way to suppress diagnostics than using a pragma. However, use of the pragma in the wild suggests people are just as interested in suppressing frontend diagnostics as they are any other kind. I'd like to make sure that whatever we design can be used for all of these purposes. What I want to avoid is having a static analyzer-specific suppression attribute, and a different one for clang-tidy, and a third one for the frontend. I also want to avoid confusing behavior. That's why I was wondering if you were considering the wider scope as part of this design.

For instance, given code like:

  signed i = ...;
  unsigned u = ...;
  int *ptr = ...;
  int what = ...;

  int val = i < u ? *ptr : *ptr / what;

This code would have at least three different diagnostics -- a signed/unsigned comparison, a possible null dereference of ptr (twice), and a possible divide by zero. If the user tries to add `[[clang::suppress]]` to that statement, it won't suppress all of the diagnostics. Also, depending on how the suppression works, it may also be even more unintuitive because clang-tidy runs the clang static analyzer and the clang frontend, so the user may be able to suppress some warnings that look like they come from clang-tidy and not others. I think that's going to be confusing behavior for users and we don't want to live in that state for very long and we should be considering the bigger design of the feature (which may require an RFC given how many components are involved).

I'm not suggesting we have to roll out complete support for everything with the initial patch. I'm fine if the initial support only suppresses static analyzer warnings so long as there's a clear path forward for the other components and (hopefully) some promise of extending support to those within the next release or two. I don't want a repeat of what happened with `[[gsl::suppress]]` (where we added the attribute to Clang, didn't utilize it in tree, the C++ Core Guideline then changed the syntax for the attribute to be incompatible with what we provided, and we're left with something that's not particularly useful for anyone).

>> One usability concern I have with adding [[clang::suppress]] is that users are going to want to use it to suppress all the various kinds of diagnostics and not just clang static analyzer warnings.
>
> Documentation will explicitly state that this mechanism is for the clang static analyzer and if users would like to use it for others things as well, that would prove that it is a good approach and can be fully supported in other parts of Clang.

I don't think documentation will be sufficient long-term. The attribute name is basically self-documenting in terms of what the user will expect it to do and it won't behave in the obvious way.

> Additionally, I was thinking about third parties developing their own static analysis tools using Clang as a parser for C/C++/Obj-C.  They would probably also like to use this attribute.  This is one reason why we probably shouldn't complain about unknown identifiers used with this attribute.

That's a great point and is definitely something that should be considered as part of the wider concern.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93110