Page MenuHomePhabricator

[analyzer] Implement fine-grained suppressions via attributes
Needs ReviewPublic

Authored by vsavchenko on Dec 11 2020, 5:50 AM.

Details

Summary

At the moment, the only possible way to suppress analyzer warnings
is by cutting the code out with #ifdef. It is practically impossible
to do locally, so whole functions should stay not analyzed.

This patch introduces new attribute analyzer_suppress for these
purposes. Even without specifying checker IDs, it is far more
precise than the existing practice. It can be applied to statements
in question. Its proximity to the warning and the fact that this is
part of the code, drastically increases its chances to survive code
refactoring.

At the moment, new suppression attribute is limited to statements
and variable declarations. This is a deliberate design choice.
It is not clear and open for discussions how it should behave when
placed on function or class declarations.

First choice here is to suppress warnings within the declaration
and the body. This includes forward declarations, nested classes
and so on. This way is more obvious and probably more intuitive,
but creates a whole bunch of problems. First of all, it is a
rather crude solution. Suppressions in large scopes such as
functions and classes should be discouraged and that's why we
actually went to the statement level. The second problem is
with issues reported on declarations. The only way the user can
suppress such warnings is to put the annotation directly on
the declaration. However, this will suppress issues within the
body of this declaration, including nested functions and classes.
The paradox of this situation is that in this case the current
approach is actually counter-intuitive.

The second approach addresses these two problems. It states that
the attribute placed on the declaration suppresses only warnings
reported on this declaration, but not in its body. And while
this way seems reasonable when we talk about the stated problems,
for the regular user, who is not familiar with all this, it might
seem that suppressions on functions and classes are non-obvious
or simply ignored by the analyzer.

Additionally, further on, when we decide on stable checker
identifiers, we can implement finer approach, where the user can
specify those to suppress specific checks. Current code still
can be relevant as it gives "suppress all" option, which is usually
enough.

Diff Detail

Unit TestsFailed

TimeTest
40 msx64 windows > Clang.CoverageMapping::branch-constfolded.cpp
Script: -- : 'RUN: at line 3'; c:\ws\w64\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w64\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name branch-constfolded.cpp C:\ws\w64\llvm-project\premerge-checks\clang\test\CoverageMapping\branch-constfolded.cpp | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\clang\test\CoverageMapping\branch-constfolded.cpp
50 msx64 windows > Clang.CoverageMapping::branch-macros.cpp
Script: -- : 'RUN: at line 4'; c:\ws\w64\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w64\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name branch-macros.cpp C:\ws\w64\llvm-project\premerge-checks\clang\test\CoverageMapping\branch-macros.cpp | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\clang\test\CoverageMapping\branch-macros.cpp
50 msx64 windows > Clang.CoverageMapping::branch-mincounters.cpp
Script: -- : 'RUN: at line 4'; c:\ws\w64\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w64\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name branch-logical-mixed.cpp C:\ws\w64\llvm-project\premerge-checks\clang\test\CoverageMapping\branch-mincounters.cpp | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\clang\test\CoverageMapping\branch-mincounters.cpp
30 msx64 windows > Clang.CoverageMapping::branch-templates.cpp
Script: -- : 'RUN: at line 4'; c:\ws\w64\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w64\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name branch-templates.cpp C:\ws\w64\llvm-project\premerge-checks\clang\test\CoverageMapping\branch-templates.cpp | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\clang\test\CoverageMapping\branch-templates.cpp
60 msx64 windows > Clang.Profile::branch-logical-mixed.cpp
Script: -- : 'RUN: at line 4'; c:\ws\w64\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w64\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -main-file-name branch-logical-mixed.cpp C:\ws\w64\llvm-project\premerge-checks\clang\test\Profile\branch-logical-mixed.cpp -o - -emit-llvm -fprofile-instrument=clang | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe -allow-deprecated-dag-overlap C:\ws\w64\llvm-project\premerge-checks\clang\test\Profile\branch-logical-mixed.cpp
View Full Test Results (6 Failed)

Event Timeline

vsavchenko created this revision.Dec 11 2020, 5:50 AM
vsavchenko requested review of this revision.Dec 11 2020, 5:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2020, 5:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Right now, I reused existing suppress attribute. However I did it in a rather unconventional manner. I allowed 0 arguments in one spelling and >1 in another, which seems odd.
I can see a couple other possible solutions here:

  • Choose a "keyword" that would be used to suppress all warnings (e.g. "suppress(all)")
  • Introduce two different suppress attributes (maybe even rename existing attribute to be GSLSuppress)

Thanks for working on this!

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2912

Since IIUC a node can have multiple parents - does that mean we could end up traversing a node multiple times?
BTW do you have an example of a node that have multiple parents?

2941

There's a note for ASTContext::getParents:

"New callers should use ParentMapContext::getParents() directly."

https://clang.llvm.org/doxygen/classclang_1_1ASTContext.html#a32d11844fdb82310b9059784fd4ceb6b

Shall we do that?

vsavchenko added inline comments.Dec 16 2020, 6:39 AM
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2912

The vast majority of AST nodes have exactly one parent. As for the nodes that do have multiple parents, I only know about InitListExpr.
And yes, I think we can end up traversing a node multiple times, but no one seems to be bothered by that

2941

Sure!

xazax.hun added inline comments.Dec 16 2020, 7:19 AM
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2902

What will this location return? In case of a leak warning, we might get a different instance of the same warning on separate paths. We usually pick the shortest path, but it can change when we slightly alter the source code. Maybe we want the user to put the suppression at the uniqueing location when such location exist? (The allocation in case of a leak warnings.) I think that would result in a better user experience and more robust suppression mechanism. An open question is how to educate the user about the correct way of suppression. Should we emit a suppress location to the user explicitly?

Right now, I reused existing suppress attribute. However I did it in a rather unconventional manner. I allowed 0 arguments in one spelling and >1 in another, which seems odd.
I can see a couple other possible solutions here:

  • Choose a "keyword" that would be used to suppress all warnings (e.g. "suppress(all)")
  • Introduce two different suppress attributes (maybe even rename existing attribute to be GSLSuppress)

I don't think it's a problem that we use the same semantic attribute but with different syntax requirements for the arguments. The semantics for GSL suppression and Clang suppression are the same so it makes sense to me to use the same semantic attribute so we don't wind up checking isa<ThisAttr, ThatAttr>(D) everywhere. However, if it does start to get awkward, I think splitting the attributes into two different semantic attributes is preferable to using a keyword.

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. We get away with [[gsl::suppress]] because that's very specific to the C++ core guidelines, but we can't really hand-wave away the problem with [[clang::suppress]]. Have you explored how this attribute will work with clang frontend diagnostics or clang-tidy diagnostics? (This ignores some of the even harder diagnostics like ones that percolate back up from the backend, but we should probably keep those in mind as well.)

clang/include/clang/Basic/Attr.td
2390

The documentation will need to be updated for this. I have no idea if that may be a bit awkward because we're documenting two different suppression mechanisms (that do roughly the same thing).

clang/lib/Sema/SemaDeclAttr.cpp
4666–4667

The behavior here will be that [[gsl::suppress]] in C++11 mode and [[clang::suppress]] in C++11 mode require at least one argument, while [[clang::supress]] in C2x mode and __attribute__((suppress)) in all language modes allow any number of arguments (including zero). That doesn't seem like what you want. I think you need something more like:

// The [[gsl::suppress]] spelling requires at least one argument, but the GNU
// and [[clang::suppress]] spellings do not require any arguments.
if (AL.hasScope() && AL.getScopeName()->isStr("gsl") &&
    !checkAttributeAtLeastNumArgs(S, AL, 1))
  return;
clang/lib/Sema/SemaStmtAttr.cpp
56–57

Same issue here.

(In other news, it looks like this attribute could stand to be updated to be a DeclOrStmtAttr once D92800 lands -- that's not your problem to solve, more just an observation.)

63–64
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2890

The coding standard has us elide braces on single-line ifs (same comment applies elsewhere).

vsavchenko added inline comments.Dec 17 2020, 6:03 AM
clang/include/clang/Basic/Attr.td
2390

I decided not to change it yet because I was not sure that this is going to be the final solution.

clang/lib/Sema/SemaDeclAttr.cpp
4666–4667

That's much better, thanks!

clang/lib/Sema/SemaStmtAttr.cpp
56–57

This is actually something that I wanted to ask out of curiosity, how it works as a declaration attribute if it's declared as StmtAttr

63–64

Sure

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2890

Sure!

2902

Ah, leaks. I thought about those, and probably should've mentioned it here.
I think it is counter-intuitive to have separate locations for warnings themselves and for suppressions. Because this would be the first place where the user will try to put suppression. It is not obvious what to do with locations when we report that something didn't happen when it should've. But this group of checkers is relatively small, usually we do have a precise location where something bad happens. So, I believe that leaks should be addressed separately and not affect design for suppressions for the vast majority of checkers.
Speaking of leaks, RetainReleaseChecker reports leaks on the allocation and they can be suppressed that way.

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.

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.

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.

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.

Squash all three commits, introduce new attribute and add documentation.

vsavchenko retitled this revision from [analyzer] Implement a first version of suppressions via attributes to [analyzer] Implement fine-grained suppressions via attributes.Jan 20 2021, 5:03 AM
vsavchenko edited the summary of this revision. (Show Details)
NoQ added a comment.Jan 28 2021, 10:31 PM

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.

Given that there are already multiple suppression mechanisms circulating around (clang-tidy's // NOLINT, frontend pragmas, static analyzer's #ifdef __clang_analyzer__, now also attribute), i guess a path forward would be to eventually add support for multiple mechanisms everywhere. This may be tedious to implement but benefits may be well worth it. Consistency across tools is important when tools are used together; for instance, when clang-tidy integrates static analyzer, static analyzer automatically starts supporting // NOLINT through clang-tidy's diagnostic consumer magic. This would also be amazing for discoverability: users can keep using their preferred mechanism and it'd just work out of the box when they add a new tool to their arsenal.

clang/include/clang/Basic/Attr.td
2396

[evil mode]
What's the expected behavior when the same variable (esp. global) has multiple redeclarations and only some of them wear the attribute?
[/evil mode]

I guess this mostly applies to globals and we don't have much checkers that would warn on globals (maybe some of those WebKit checkers by @jkorous may be affected).

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
592–593

Even though the attribute is specific to the static analyzer and from that point of view it's appropriate to keep this info in the BugReporter object (that will always be specific to the static analyzer), i believe we should ultimately move this into a higher-level entity in libAnalysis such as AnalysisConsumer. This would allow other tools such as clang-tidy to access that information when integrated into the static analyzer (eg., a-la D95403) and the user treats the entire conglomerate of tools as just "the static analyzer" and expects consistency across checks. That, of course, relies on our ability to look up attributes by source locations.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
31–32

So, like, i wish this could be replaced with bool isSuppressed(const PathDiagnostic &); so that to untie this from Static Analyzer-specific BugReport object.

There's no straightforward way to jump from BugReport to its specific PathDiagnostic, especially given that the same bug report may produce different PathDiagnostics for different consumers. But by the time suppressions kick in we can be certain that PathDiagnostic objects are fully constructed.

It's an interesting question whether different PathDiagnostics for the same bug report may interact with suppressions differently. For now this can't happen because all of them will have the same location and the same uniqueing location. We should obviously avoid such differences, probably protect ourselves against them with an assertion.

39

Depression fuel! :D

clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
71

A recursive visitor would cause you to visit nested declarations (eg., lambdas, or methods of nested classes) multiple times. Is this necessary? A plain old ConstStmtVisitor that's taught to visit child statements recursively could easily avoid that. That's probably cheap though.

Fix failing tests

vsavchenko added inline comments.Jan 29 2021, 12:59 AM
clang/include/clang/Basic/Attr.td
2396

You're right, there is no way to support local variables and not globals (except for more manual approach in Sema when we assign these attributes).

As of now, it will work on the declaration/definition where the bug is reported. It is not perfect, but I won't say that it is counter-intuitive either.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
31–32

In order to move fully from BugReport to PathDiagnostic, we can pre-map the whole TU with suppressed "areas" (in the same way we do it now for declarations with issues). If we do that, we need to decide where and when such initialization should take place.

clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
71

I don't see how I can easily use StmtVisitors without A LOT OF boilerplate code that will essentially produce a RecursiveASTVisitor (it is very possible that I just don't know about some trick). I guess we can add some optimization if needed, but I never had performance problems with recursive visitors.

NoQ added inline comments.Jan 29 2021, 1:10 AM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
31–32

We still have access to decl-with-issue and uniqueing-decl in PathDiagnostic if that's what seems problematic to you (?)

clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
71

The typical idiom is as follows:

void VisitChildren(const Stmt *S) {
  // Not a visitor callback - just a helper function.
  for (const Stmt *ChildS : S->children())
    if (ChildS)
      Visit(ChildS);
}

void VisitStmt(const Stmt *S) {
  // The default behavior that makes the visitor recursive over statements.
  VisitChildren(S); 
}

void VisitAttributedStmt(const AttributedStmt *AS) {
  VisitAttributedNode(AS);
  VisitChildren(AS); // This *optionally* goes into every overridden function.
}

Also you'll probably have to manually unwrap VisitVarDecl into VisitDeclStmt with a loop over decls. But generally i think that's relatively little boilerplate for the much better control it provides.

I never had performance problems with recursive visitors

Me too until i started doing clang-tidy --enable-check-profile :D So, like, i don't think that's going to be a real problem but kind of why not avoid it entirely.

vsavchenko added inline comments.Jan 29 2021, 1:32 AM
clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
71

That makes sense, but it's not a no-brainer tradeoff IMO. It simply gives me confidence that I'm not losing any portion of the AST out of my sight.

In D93110#2529917, @NoQ wrote:

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.

Given that there are already multiple suppression mechanisms circulating around (clang-tidy's // NOLINT, frontend pragmas, static analyzer's #ifdef __clang_analyzer__, now also attribute), i guess a path forward would be to eventually add support for multiple mechanisms everywhere. This may be tedious to implement but benefits may be well worth it. Consistency across tools is important when tools are used together; for instance, when clang-tidy integrates static analyzer, static analyzer automatically starts supporting // NOLINT through clang-tidy's diagnostic consumer magic. This would also be amazing for discoverability: users can keep using their preferred mechanism and it'd just work out of the box when they add a new tool to their arsenal.

To be clear, I'm fine with multiple mechanisms, but not fine with multiple of the same mechanisms. e.g., I don't think it's an issue that we can suppress via the command line, pragmas, NOLINT comments, and attribute usage, but I do think it's a problem if there's one attribute used by clang-tidy and a different attribute used by the static analyzer, and a third attribute for clang itself. Using different attributes causes users to have to consider what basically amount to implementation details of their toolchain and makes it harder to do things like integrate clang-tidy and the clang static analyzer together.

In D93110#2529917, @NoQ wrote:

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.

Given that there are already multiple suppression mechanisms circulating around (clang-tidy's // NOLINT, frontend pragmas, static analyzer's #ifdef __clang_analyzer__, now also attribute), i guess a path forward would be to eventually add support for multiple mechanisms everywhere. This may be tedious to implement but benefits may be well worth it. Consistency across tools is important when tools are used together; for instance, when clang-tidy integrates static analyzer, static analyzer automatically starts supporting // NOLINT through clang-tidy's diagnostic consumer magic. This would also be amazing for discoverability: users can keep using their preferred mechanism and it'd just work out of the box when they add a new tool to their arsenal.

To be clear, I'm fine with multiple mechanisms, but not fine with multiple of the same mechanisms. e.g., I don't think it's an issue that we can suppress via the command line, pragmas, NOLINT comments, and attribute usage, but I do think it's a problem if there's one attribute used by clang-tidy and a different attribute used by the static analyzer, and a third attribute for clang itself. Using different attributes causes users to have to consider what basically amount to implementation details of their toolchain and makes it harder to do things like integrate clang-tidy and the clang static analyzer together.

We can integrate these solutions together for clang-tidy and for clang static analyzer, it's not a problem at all. I would've used the existing Suppress attribute if it was without gsl part. As I mentioned in the summary, at the moment, we want to use it ONLY inside functions. I can see that SuppressAttr is not used anywhere in the whole, but I can't be sure that it's not used somewhere externally. So, I couldn't just reduce the scope of it.

In D93110#2529917, @NoQ wrote:

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.

Given that there are already multiple suppression mechanisms circulating around (clang-tidy's // NOLINT, frontend pragmas, static analyzer's #ifdef __clang_analyzer__, now also attribute), i guess a path forward would be to eventually add support for multiple mechanisms everywhere. This may be tedious to implement but benefits may be well worth it. Consistency across tools is important when tools are used together; for instance, when clang-tidy integrates static analyzer, static analyzer automatically starts supporting // NOLINT through clang-tidy's diagnostic consumer magic. This would also be amazing for discoverability: users can keep using their preferred mechanism and it'd just work out of the box when they add a new tool to their arsenal.

To be clear, I'm fine with multiple mechanisms, but not fine with multiple of the same mechanisms. e.g., I don't think it's an issue that we can suppress via the command line, pragmas, NOLINT comments, and attribute usage, but I do think it's a problem if there's one attribute used by clang-tidy and a different attribute used by the static analyzer, and a third attribute for clang itself. Using different attributes causes users to have to consider what basically amount to implementation details of their toolchain and makes it harder to do things like integrate clang-tidy and the clang static analyzer together.

We can integrate these solutions together for clang-tidy and for clang static analyzer, it's not a problem at all. I would've used the existing Suppress attribute if it was without gsl part.

Sorry, I failed at being clear. Let me take another stab at it.

<ideally>
I don't want multiple unrelated semantic attributes to handle this concept. We can have a single semantic attribute with multiple ways of spelling it (e.g., [[gsl::suppress]] and [[clang::suppress]]) easily enough. I want to avoid code that looks like if (const auto *A = D->getAttr<SuppressAttr>()) { ... } else if (const auto *A = D->getAttr<SomeOtherSuppressAttr>()) { ... } because this causes maintenance problems where someone invariably forgets the less-used suppression attribute somewhere. (I would be fine if we had a common base class used by multiple suppression semantic attributes, if that was necessary to avoid this problem. )

I also don't want multiple attribute spellings for different parts of the toolchain. e.g., a user should not have to write [[clang::suppress(...)]] for a frontend suppression and [[tidy::suppress(...)]] for a tidy suppression, and [[csa::suppress(...)]] for a static analyzer suppression. This leaks implementation details out to users that they're not likely to have insights into anyway and it causes problems when we shift a diagnostic around or combine tools (like the static analyzer and tidy being able to run one another's checks).

I don't think it's an issue to have multiple attribute spellings to support different coding style guidelines or to be compatible with another compiler because users will be able to reason about those annotations more easily than which part of the toolchain a diagnostic comes from. So I don't think it's an issue to have [[gsl::suppress(...)]] and [[clang::suppress(...)]] as spellings so long as we wind up with a single semantic attribute interface under the hood.
</ideally>

As I mentioned in the summary, at the moment, we want to use it ONLY inside functions. I can see that SuppressAttr is not used anywhere in the whole, but I can't be sure that it's not used somewhere externally. So, I couldn't just reduce the scope of it.

We have the notion of attribute accessors to help with this sort of thing. It's reasonable to add an accessor to see whether the attribute was spelled with the gsl spelling, and if so, make alternative diagnostic decisions from it. e.g. https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Attr.td#L650