ziqingluo-90 updated this revision to Diff 519719.
ziqingluo-90 marked 3 inline comments as done.
ziqingluo-90 added a comment.
Addressed the comments:
- 1) about using a more traditional way of AST traversal; and
- 2) about the concern on `ObjcMethodDecl` traversal.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146342/new/
https://reviews.llvm.org/D146342
Files:
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-objc-method-traversal.mm
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-objc-method-traversal.mm
===================================================================
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-objc-method-traversal.mm
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -std=c++20 -Wno-objc-root-class -Wno-return-type -Wunsafe-buffer-usage %s -verify %s
+
+// This test is to show that ObjC methods are traversed by the
+// end-of-TU analysis properly. Particularly, a method body will not
+// be repeatedly visited whenever a method prototype of it is visited.
+
+@interface I
++ f:(int *) p; // duplicated method prototype declaration
+
++ f:(int *) p;
+
++ f:(int *) p;
+@end
+
+@implementation I
++ f:(int *) p { // expected-warning{{'p' is an unsafe pointer used for buffer access}}
+ int tmp;
+ tmp = p[5]; // expected-note{{used in buffer access here}}
+}
+@end
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -13,6 +13,7 @@
//===----------------------------------------------------------------------===//
#include "clang/Sema/AnalysisBasedWarnings.h"
+#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclObjC.h"
#include "clang/AST/EvaluatedExprVisitor.h"
@@ -2316,44 +2317,53 @@
public:
CallableVisitor(Sema &S, bool EmitFixits) : S(S), EmitFixits(EmitFixits) {}
- // The only interface that clients of `CallableVisitor` should call:
- void traverseTU(const TranslationUnitDecl *TU) {
- for (auto I = TU->decls_begin(); I != TU->decls_end(); ++I)
- TraverseDecl(*I);
- }
-
bool TraverseDecl(Decl *Node) {
- DiagnosticsEngine &Diags = S.getDiagnostics();
-
if (!Node)
return true;
- // Do not do any analysis if we are going to just ignore them.
+
+ // Do not analyze any `Decl` if we are going to just ignore them.
+ DiagnosticsEngine &Diags = S.getDiagnostics();
+
if (Diags.getSuppressSystemWarnings() &&
S.SourceMgr.isInSystemHeader(Node->getLocation()))
return true;
- // To analyze callables:
- if (isa<FunctionDecl, BlockDecl, ObjCMethodDecl>(Node)) {
- // For code in dependent contexts, we'll do this at instantiation time:
- if (cast<DeclContext>(Node)->isDependentContext())
- return true;
+ // Continue to traverse descendants:
+ return RecursiveASTVisitor::TraverseDecl(Node);
+ }
+
+ bool VisitFunctionDecl(FunctionDecl *Node) {
+ if (cast<DeclContext>(Node)->isDependentContext())
+ return true; // Not to analyze dependent decl
+ // `FunctionDecl->hasBody()` returns true if the function has a body
+ // somewhere defined. But we want to know if this `Node` has a body
+ // child. So we use `doesThisDeclarationHaveABody`:
+ if (Node->doesThisDeclarationHaveABody()) {
+ UnsafeBufferUsageReporter R(S);
- bool ThisNodeHasBody = false;
+ checkUnsafeBufferUsage(Node, R, EmitFixits);
+ }
+ return true;
+ }
- if (auto *FD = dyn_cast<FunctionDecl>(Node))
- // `FunctionDecl->hasBody()` returns true if the function has a body
- // somewhere defined. But we want to know if this `Node` has a body
- // child. So we use `doesThisDeclarationHaveABody`:
- ThisNodeHasBody = FD->doesThisDeclarationHaveABody();
- else
- ThisNodeHasBody = Node->hasBody();
- if (ThisNodeHasBody) {
- UnsafeBufferUsageReporter R(S);
+ bool VisitBlockDecl(BlockDecl *Node) {
+ if (cast<DeclContext>(Node)->isDependentContext())
+ return true; // Not to analyze dependent decl
- checkUnsafeBufferUsage(Node, R, EmitFixits);
- }
+ UnsafeBufferUsageReporter R(S);
+
+ checkUnsafeBufferUsage(Node, R, EmitFixits);
+ return true;
+ }
+
+ bool VisitObjCMethodDecl(ObjCMethodDecl *Node) {
+ if (cast<DeclContext>(Node)->isDependentContext())
+ return true; // Not to analyze dependent decl
+ if (Node->hasBody()) {
+ UnsafeBufferUsageReporter R(S);
+
+ checkUnsafeBufferUsage(Node, R, EmitFixits);
}
- // Continue to traverse descendants:
- return RecursiveASTVisitor::TraverseDecl(Node);
+ return true;
}
bool TraverseStmt(Stmt *Node, DataRecursionQueue *Queue = nullptr) {
@@ -2382,7 +2392,9 @@
// Emit unsafe buffer usage warnings and fixits.
if (!Diags.isIgnored(diag::warn_unsafe_buffer_operation, SourceLocation()) ||
!Diags.isIgnored(diag::warn_unsafe_buffer_variable, SourceLocation())) {
- CallableVisitor(S, EmitFixits).traverseTU(TU);
+ CallableVisitor(S, EmitFixits)
+ .TraverseTranslationUnitDecl(
+ std::remove_const_t<TranslationUnitDecl *>(TU));
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
