Skip to content

[analyzer] Fix [[clang::suppress]] for template instantiations #168954

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+30 −2
Open

[analyzer] Fix [[clang::suppress]] for template instantiations #168954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,12 @@ class CacheInitializer : public DynamicRecursiveASTVisitor {
}
}

CacheInitializer(Ranges &R) : Result(R) {}
CacheInitializer(Ranges &R) : Result(R) {
ShouldVisitTemplateInstantiations = true;
ShouldWalkTypesOfTypeLocs = false;
ShouldVisitImplicitCode = false;
ShouldVisitLambdaBody = true;
}
Ranges &Result;
};

Expand Down
25 changes: 24 additions & 1 deletion clang/test/Analysis/suppression-attr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,27 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s

void clang_analyzer_warnIfReached();

struct Clazz {
template <typename T>
static void templated_memfn();
};

// This must come before the 'templated_memfn' is defined!
static void instantiate() {
Clazz::templated_memfn<int>();
}

template <typename T>
void Clazz::templated_memfn() {
// When we report a bug in a function, we traverse the lexical decl context
// of it while looking for suppression attributes to record what source
Copy link
Contributor

@ziqingluo-90 ziqingluo-90 Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand how BugSuppression works:
The bug is at the clang_analyzer_warnIfReached() call so the decl context being visited is the instantiated definition of Clazz::templated_memfn<int>() {...}. During visiting, the analyzer will find that the call expression has the attribute.

What would happen if I move [[clang::suppress]] to line 12: [[clang::suppress]] Clazz::templated_memfn<int>();?

Copy link
Contributor Author

@steakhal steakhal Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand how BugSuppression works: The bug is at the clang_analyzer_warnIfReached() call so the decl context being visited is the instantiated definition of Clazz::templated_memfn<int>() {...}. [...]

You are right. Sharp eyes. I lied a bit because we also swap the decl context to the parent context, which is going to be the class definition itself, that has the template instantiations as child nodes in the AST - including the one where we have the suppression. However, if we skip traversing instantiations then we would not see the suppression.
https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp#L175-L189

  } else {
    // This is the fast path. However, we should still consider the topmost
    // declaration that isn't TranslationUnitDecl, because we should respect
    // attributes on the entire declaration chain.
    while (true) {
      // Use the "lexical" parent. Eg., if the attribute is on a class, suppress
      // warnings in inline methods but not in out-of-line methods.
      const Decl *Parent =
          dyn_cast_or_null<Decl>(DeclWithIssue->getLexicalDeclContext());
      if (Parent == nullptr || isa<TranslationUnitDecl>(Parent))
        break;

      DeclWithIssue = Parent;
    }
  }

What would happen if I move [[clang::suppress]] to line 12: [[clang::suppress]] Clazz::templated_memfn<int>();?

I've not tried, but I'm almost certain that having a suppress for a statement (such as your proposed attribute at that CallExpr) would only suppress issues raised within the source range of the effective statement (CallExpr).
Since the bug is raised within the callee of that CallExpr, the bug should not be suppressed by that attribute.

If it would suppress it, then that would be a bug in the suppression algorithm.

Copy link
Contributor

@ziqingluo-90 ziqingluo-90 Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having a suppress for a statement would only suppress issues raised within the source range of the effective statement (CallExpr).

Aha, this explains it.

Copy link
Collaborator

@haoNoQ haoNoQ Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's correct.

FWIW it's quite valuable to have a *variant* of the suppress attribute that *would* work when applied at call site. This is particularly useful when the warning is positioned in a system header. Eg. you dereference a null smart pointer, and the warning is in the implementation of the overloaded dereference operator, so it'd make a lot of sense to put suppression at the call site of the dereference operator, so that you could keep getting warnings about the rest of null dereferences through that operator.

But we probably shouldn't handle such cases by default. That's too coarse as it allows you to accidentally suppress bug reports across thousands of lines of code that you didn't even know were invoked from inside that source range. So we only want such a behavior under very specific circumstances.

Copy link
Contributor Author

@steakhal steakhal Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eg. you dereference a null smart pointer, and the warning is in the implementation of the overloaded dereference operator, so it'd make a lot of sense to put suppression at the call site of the dereference operator, so that you could keep getting warnings about the rest of null dereferences through that operator.

Eh, I'm not sure. Wouldn't that mean that in practice most callsites would need that attribute? Like if there is a branch in that definition, that happens to be triggered from almost all callsites. At that point, it would be very inconvenient to apply a suppression attribute at the callsites.

I agree that it feels niche.

Copy link
Collaborator

@haoNoQ haoNoQ Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's a branch then there's probably an assertion failure on the null side of the branch. So there won't be a bug report and there's nothing to suppress.

But it's very common to not have a branch, just dereference it blindly inside operator*() to form a null reference if the pointer was null. That's where the warning would show up if and only if the call site misuses the pointer, so it makes perfect sense to have suppression at the call site too.

Basically if your code never uses raw pointers and only uses standard smart pointers (and our experimental explicit models for those standard smart pointers aren't turned on) then *every* null dereference warning would suffer from this problem, and *every* false positive among those otherwise reasonably-reliable warnings would need to be suppressed at the call site this way.

(Ok ok not every warning. The call sites that perform dereference with operator->() wouldn't need it because the dereference happens at the call site, the operator simply passes the null pointer through. I'm only talking about dereferences with operator*() which would create a null reference before they leave the system header.)

But either way, as long as we believe that emitting warnings in the system headers is a good idea (simply because the root cause of the problem isn't necessarily in the system header), we have to accept that we need a way to suppress these warnings without touching system headers (when the root cause of the *false positive* isn't necessarily in the system header either).

// ranges should the suppression apply to.
// In the past, that traversal didn't follow template instantiations, only
// primary templates.
[[clang::suppress]] clang_analyzer_warnIfReached(); // no-warning

}

namespace [[clang::suppress]]
suppressed_namespace {
Expand Down