[analyzer] Fix [[clang::suppress]] for template instantiations #168954
Conversation
BugSuppression works by traversing the lexical decl context of the decl with issue to record what source ranges should be suppressed by some attribute. It happens so that it uses a DynamicRecursiveASTVisitor, which has a couple of traversal options. Namely: - ShouldVisitTemplateInstantiations - ShouldWalkTypesOfTypeLocs - ShouldVisitImplicitCode - ShouldVisitLambdaBody By default, these have the correct values, except for ShouldVisitTemplateInstantiations. We should traverse template instantiations because that might be where the bug is reported - thus, where we might have a [[clang::suppress]] that we should honor. In this patch I'll explicitly set these traversal options to avoid further confusion. rdar://164646398
steakhal
requested review from
NagyDonat,
Xazax-hun,
haoNoQ and
ziqingluo-90
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balázs Benics (steakhal) ChangesBugSuppression works by traversing the lexical decl context of the decl with issue to record what source ranges should be suppressed by some attribute. It happens so that it uses a DynamicRecursiveASTVisitor, which has a couple of traversal options. Namely:
By default, these have the correct values, except for ShouldVisitTemplateInstantiations. We should traverse template instantiations because that might be where the bug is reported - thus, where we might have a [[clang::suppress]] that we should honor. In this patch I'll explicitly set these traversal options to avoid further confusion. rdar://164646398 Full diff: https://github.com/llvm/llvm-project/pull/168954.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp b/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
index 5b5f9df9cb0dc..a7300b7183590 100644
--- a/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
@@ -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;
};
diff --git a/clang/test/Analysis/suppression-attr.cpp b/clang/test/Analysis/suppression-attr.cpp
index 89bc3c47dbd51..9ba56d976fddb 100644
--- a/clang/test/Analysis/suppression-attr.cpp
+++ b/clang/test/Analysis/suppression-attr.cpp
@@ -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
+ // 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 {
|
🐧 Linux x64 Test Results
|
ziqingluo-90
left a comment
ziqingluo-90
left a comment
There was a problem hiding this comment.
The fix LGTM.
I have one question for learning about how BugSuppression works though.
| 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 |
There was a problem hiding this comment.
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>();?
There was a problem hiding this comment.
I'm trying to understand how
BugSuppressionworks: The bug is at theclang_analyzer_warnIfReached()call so the decl context being visited is the instantiated definition ofClazz::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.
There was a problem hiding this comment.
having a suppress for a statement would only suppress issues raised within the source range of the effective statement (CallExpr).
Aha, this explains it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
|
Thank you for the review @ziqingluo-90. I'll adjust the commit message to reflect on your question before landing the change. EDIT: enhanced the PR summary. I plan to merge the PR tomorrow unless someone objects. |
haoNoQ
left a comment
haoNoQ
left a comment
There was a problem hiding this comment.
I'm somewhat surprised that it's necessary to visit template instantiations. This machine simply memorizes source locations. Template instantiations have the same source locations as the template itself right? (...right? 😳)
LGTM because I'm sure there's a perfectly reasonable explanation for that.
I see what you mean. I'll have a look tomorrow. |
It actually didn't take long. Check the AST of the example: https://godbolt.org/z/MsT5jx3nE Funnily, the primary template is outside of the CXXRecordDecl. Only the template instantiation is under the CXXRecordDecl. |
Ohhh. Yeah I'm assuming that this is because compiling uninstantiated templates is so difficult that you can't even tell whether that template actually belongs inside the class at this stage of compilation. You only learn that after substituting the parameter. (I may be completely wrong but this is the usual explanation for these quirks IIRC.) (There's no way to put different instantiations in different classes through something like right? I hope it's just a simplification that the compilation process does simply because it can.) |
|
On the other hand, I think I know why this wasn't a problem before. It wasn't a problem before because most of the time method templates (or methods of class templates) aren't written out of line. This is because you have to keep them in the header anyway because otherwise you won't be able to instantiate them. So might as well write inline. Which means that there's probably a small opportunity for optimization there. It may be a good idea to include the uninstantiated template in the scan range while continuing to skip all instantiations? Because that's presumably how everything else works anyway. And this way we won't need to scan all the instantiations that we still don't need to scan. And this could be, like, somewhat noticeable for performance, I think. |
Then I think explicit template specializations would break. The unfortunate fact is that primary templates are not that useful for us. To sidestep this, what if we would not overrule the original DeclWithIssue and just traverse that one. That will be the correct fn to traverse. But honestly, I just probably wouldnt touch this. |
4 participants
BugSuppression works by traversing the lexical decl context of the decl-with-issue to record what source ranges should be suppressed by some attribute.
Note that the decl-with-issue will be changed to the lexical decl context of the original decl-with-issue, to make suppression attributes work that were attached to the CXXRecordDecl containing the CXXMethodDecl (bug report's DeclWithIssue).
It happens so that it uses a DynamicRecursiveASTVisitor, which has a couple of traversal options. Namely:
By default, these have the correct values, except for ShouldVisitTemplateInstantiations. We should traverse template instantiations because that might be where the bug is reported - thus, where we might have a [[clang::suppress]] that we should honor.
In this patch I'll explicitly set these traversal options to avoid further confusion.
rdar://164646398