vsavchenko created this revision.
Herald added subscribers: steakhal, ASDenysPetrov, martong, Charusso, dkrupp,
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware,
xazax.hun.
Herald added a reviewer: aaron.ballman.
vsavchenko requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
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 suggests how we can use `__attribute__((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.
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 usally enough.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D93110
Files:
clang/include/clang/Basic/Attr.td
clang/lib/Sema/SemaDeclAttr.cpp
clang/lib/Sema/SemaStmtAttr.cpp
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
clang/test/Analysis/suppression-attr.m
clang/test/SemaCXX/suppress.cpp
Index: clang/test/SemaCXX/suppress.cpp
===================================================================
--- clang/test/SemaCXX/suppress.cpp
+++ clang/test/SemaCXX/suppress.cpp
@@ -6,17 +6,20 @@
[[gsl::suppress("in-a-namespace")]];
}
-[[gsl::suppress("readability-identifier-naming")]]
-void f_() {
+[[gsl::suppress("readability-identifier-naming")]] void f_() {
int *p;
[[gsl::suppress("type", "bounds")]] {
p = reinterpret_cast<int *>(7);
}
- [[gsl::suppress]] int x; // expected-error {{'suppress' attribute takes at least 1 argument}}
- [[gsl::suppress()]] int y; // expected-error {{'suppress' attribute takes at least 1 argument}}
- int [[gsl::suppress("r")]] z; // expected-error {{'suppress' attribute cannot be applied to types}}
+ [[gsl::suppress]] int x; // expected-error {{'suppress' attribute takes at least 1 argument}}
+ [[gsl::suppress()]] int y; // expected-error {{'suppress' attribute takes at least 1 argument}}
+ int [[gsl::suppress("r")]] z; // expected-error {{'suppress' attribute cannot be applied to types}}
[[gsl::suppress(f_)]] float f; // expected-error {{'suppress' attribute requires a string}}
+ // Only gsl spelling requires at least 1 argument
+ __attribute__((suppress)) double d; // no-error
+ __attribute__((suppress())) double e; // no-error
+ __attribute__((suppress)) { int a; } // no-error
}
union [[gsl::suppress("type.1")]] U {
Index: clang/test/Analysis/suppression-attr.m
===================================================================
--- /dev/null
+++ clang/test/Analysis/suppression-attr.m
@@ -0,0 +1,110 @@
+// RUN: %clang_analyze_cc1 -fblocks \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=osx.cocoa.MissingSuperCall \
+// RUN: -analyzer-checker=osx.cocoa.NSError \
+// RUN: -Wno-unused-value -Wno-objc-root-class -verify %s
+
+#define SUPPRESS __attribute__((suppress))
+
+@protocol NSObject
+- (id)retain;
+- (oneway void)release;
+@end
+@interface NSObject <NSObject> {
+}
+- (id)init;
++ (id)alloc;
+@end
+typedef int NSInteger;
+typedef char BOOL;
+typedef struct _NSZone NSZone;
+@class NSInvocation, NSMethodSignature, NSCoder, NSString, NSEnumerator;
+@protocol NSCopying
+- (id)copyWithZone:(NSZone *)zone;
+@end
+@protocol NSCoding
+- (void)encodeWithCoder:(NSCoder *)aCoder;
+@end
+@class NSDictionary;
+@interface NSError : NSObject <NSCopying, NSCoding> {
+}
++ (id)errorWithDomain:(NSString *)domain code:(NSInteger)code userInfo:(NSDictionary *)dict;
+@end
+
+void dereference_1() {
+ int *x = 0;
+ *x; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+}
+
+void dereference_suppression_1() {
+ int *x = 0;
+ SUPPRESS { *x; } // no-warning
+}
+
+void dereference_2() {
+ int *x = 0;
+ if (*x) { // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+ }
+}
+
+void dereference_suppression_2() {
+ int *x = 0;
+ SUPPRESS if (*x) { // no-warning
+ }
+}
+
+void dereference_3(int cond) {
+ int *x = 0;
+ if (cond) {
+ (*x)++; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+ }
+}
+
+void dereference_suppression_3(int cond) {
+ int *x = 0;
+ SUPPRESS if (cond) {
+ (*x)++; // no-warning
+ }
+}
+
+void dereference_4() {
+ int *x = 0;
+ int y = *x; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+}
+
+void dereference_suppression_4() {
+ int *x = 0;
+ SUPPRESS int y = *x; // no-warning
+}
+
+@interface UIResponder : NSObject {
+}
+- (char)resignFirstResponder;
+@end
+
+@interface Test : UIResponder {
+}
+@end
+@implementation Test
+
+- (BOOL)resignFirstResponder {
+ return 0;
+} // expected-warning {{The 'resignFirstResponder' instance method in UIResponder subclass 'Test' is missing a [super resignFirstResponder] call}}
+
+- (void)methodWhichMayFail:(NSError **)error {
+ // expected-warning@-1 {{Method accepting NSError** should have a non-void return value to indicate whether or not an error occurred}}
+}
+@end
+
+@interface TestSuppress : UIResponder {
+}
+@end
+@implementation TestSuppress
+
+- (BOOL)resignFirstResponder SUPPRESS {
+ return 0;
+} // no-warning
+
+- (void)methodWhichMayFail:(NSError **)error SUPPRESS { // no-warning
+}
+@end
Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -12,12 +12,15 @@
//===----------------------------------------------------------------------===//
#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Attr.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclObjC.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/ParentMap.h"
+#include "clang/AST/ParentMapContext.h"
#include "clang/AST/Stmt.h"
#include "clang/AST/StmtCXX.h"
#include "clang/AST/StmtObjC.h"
@@ -2869,6 +2872,107 @@
return Out;
}
+//===----------------------------------------------------------------------===//
+// Manual suppressions
+//===----------------------------------------------------------------------===//
+
+namespace {
+llvm::Optional<const SuppressAttr *>
+findSuppressAttribute(ArrayRef<const Attr *> Attrs) {
+ const auto *It =
+ llvm::find_if(Attrs, [](const Attr *A) { return isa<SuppressAttr>(A); });
+
+ if (It != Attrs.end()) {
+ return cast<SuppressAttr>(*It);
+ }
+
+ return llvm::None;
+}
+
+// TODO: In the future, when we come up with good stable IDs for checkers
+// we can return a list of kinds to ignore, or all if no arguments were
+// provided.
+llvm::Optional<const SuppressAttr *> findSuppressAttribute(const BugReport &BR,
+ ASTContext &AC) {
+ PathDiagnosticLocation Location = BR.getLocation();
+
+ if (const Stmt *BugStmt = Location.asStmt()) {
+ // If location is statement, it can actually be an expression buried
+ // deeply in other expressions, so the attribute can be few levels above
+ // in the AST. For this reason, we traverse this statement's parents
+ // and check if they have suppressions.
+ //
+ // The statement itself can have suppression attributes, so let's
+ // start with it.
+ llvm::SmallVector<DynTypedNode, 8> Stack{DynTypedNode::create(*BugStmt)};
+
+ while (!Stack.empty()) {
+ const auto &Current = Stack.back();
+ Stack.pop_back();
+
+ if (const auto *AS = Current.get<AttributedStmt>()) {
+ // Only AttributedStmt out of all statements can have attributes.
+ if (auto Suppress = findSuppressAttribute(AS->getAttrs())) {
+ return Suppress;
+ }
+ // Suppressions can still be further up, so let's continue.
+ } else if (const auto *VD = Current.get<VarDecl>()) {
+ // Bug location could've been somewhere in the init value of
+ // a freshly declared variable. Even though it looks like the
+ // user applied attribute to a statement, it will apply to a
+ // variable declaration, and this is where we check for it.
+ if (VD->hasAttr<SuppressAttr>()) {
+ return VD->getAttr<SuppressAttr>();
+ }
+ } else if (const auto *D = Current.get<Decl>()) {
+ // If we got to some other form of declaration, we don't want
+ // to go further. Declaration attributes (in cases other than for
+ // variables) should be used ONLY to suppress warnings reported directly
+ // on that declaration.
+ continue;
+ }
+
+ // Let's continue with the current node's parent(s).
+ for (const auto &Parent : AC.getParents(Current)) {
+ Stack.push_back(Parent);
+ }
+ }
+ } else if (const Decl *BugDecl = Location.asDecl()) {
+ // If we want to report on a declaration, we don't want to check parents
+ // or stuff like that, we simply want to check if that declaration has
+ // a supression attribute.
+ if (BugDecl->hasAttr<SuppressAttr>()) {
+ return BugDecl->getAttr<SuppressAttr>();
+ }
+ } else if (Location.hasValidLocation()) {
+ // This is the trickiest part. Bug location is simply a SourceLocation
+ // object. We don't know a node to attach it to.
+ //
+ // At the moment, we check the declaration containing the bug for
+ // suppression attributes. It is indeed to crude, probably a better
+ // solution would be to suppress issues reported outside of the body
+ // of this declaration in this manner (e.g. reported on curly braces
+ // or on one of the function arguments, or on class parent).
+ if (const Decl *DeclWithIssue = BR.getDeclWithIssue()) {
+ if (DeclWithIssue->hasAttr<SuppressAttr>()) {
+ return DeclWithIssue->getAttr<SuppressAttr>();
+ }
+ }
+ }
+
+ return llvm::None;
+}
+
+bool isSuppressed(const BugReport &R, ASTContext &AC) {
+ // TODO: Introduce stable IDs for checkers and check for those here
+ // to be more specific. Attribute without arguments should still
+ // be considered as "suppress all".
+ // It is already much finer granularity than what we have now
+ // (i.e. removing the whole function from the analysis).
+ return findSuppressAttribute(R, AC).hasValue();
+}
+} // end anonymous namespace
+
void BugReporter::emitReport(std::unique_ptr<BugReport> R) {
bool ValidSourceLoc = R->getLocation().isValid();
assert(ValidSourceLoc);
@@ -2877,13 +2981,17 @@
if (!ValidSourceLoc)
return;
+ // If the user asked to suppress this report, we should skip it.
+ if (isSuppressed(*R, getContext()))
+ return;
+
// Compute the bug report's hash to determine its equivalence class.
llvm::FoldingSetNodeID ID;
R->Profile(ID);
// Lookup the equivance class. If there isn't one, create it.
void *InsertPos;
- BugReportEquivClass* EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);
+ BugReportEquivClass *EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);
if (!EQ) {
EQ = new BugReportEquivClass(std::move(R));
Index: clang/lib/Sema/SemaStmtAttr.cpp
===================================================================
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -53,7 +53,7 @@
static Attr *handleSuppressAttr(Sema &S, Stmt *St, const ParsedAttr &A,
SourceRange Range) {
- if (A.getNumArgs() < 1) {
+ if (A.isCXX11Attribute() && A.getNumArgs() < 1) {
S.Diag(A.getLoc(), diag::err_attribute_too_few_arguments) << A << 1;
return nullptr;
}
@@ -66,7 +66,7 @@
return nullptr;
// FIXME: Warn if the rule name is unknown. This is tricky because only
- // clang-tidy knows about available rules.
+ // clang-tidy and static analyzer know about available rules.
DiagnosticIdentifiers.push_back(RuleName);
}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===================================================================
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4564,7 +4564,7 @@
}
static void handleSuppressAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
- if (!checkAttributeAtLeastNumArgs(S, AL, 1))
+ if (AL.isCXX11Attribute() && !checkAttributeAtLeastNumArgs(S, AL, 1))
return;
std::vector<StringRef> DiagnosticIdentifiers;
Index: clang/include/clang/Basic/Attr.td
===================================================================
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -2360,7 +2360,8 @@
}
def Suppress : StmtAttr {
- let Spellings = [CXX11<"gsl", "suppress">];
+ let Spellings = [CXX11<"gsl", "suppress">,
+ Clang<"suppress">];
let Args = [VariadicStringArgument<"DiagnosticIdentifiers">];
let Documentation = [SuppressDocs];
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
