[PATCH] D49811: [analyzer] Obtain a ReturnStmt from a CFGAutomaticObjDtor
NoQ added a comment. Herald added a subscriber: Szelethus. In https://reviews.llvm.org/D49811#1175927, @NoQ wrote: > Devin has recently pointed out that we might have as well reordered CFG > elements to have return statement kick in after automatic destructors, so > that callbacks were called in a different order. I don't see any problems > with that solution, but let's stick to the current solution for now, because > who knows. In https://bugs.llvm.org/show_bug.cgi?id=11645#c9 Ted wrote: > If we treat an occurrence of "return" in the CFG is meaning "bind an > expression result for the return value" and not as a transfer of control then > it is is fine for the destructors to appear after the "return". From this > view, the transfer back to the caller is when we hit the Exit block. Another possible solution is to check in the destructor if the newly dangling pointer is already bound to the return statement. Repository: rC Clang https://reviews.llvm.org/D49811 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49811: [analyzer] Obtain a ReturnStmt from a CFGAutomaticObjDtor
This revision was automatically updated to reflect the committed changes.
Closed by commit rC338777: [analyzer] Obtain a ReturnStmt from a
CFGAutomaticObjDtor. (authored by rkovacs, committed by ).
Repository:
rC Clang
https://reviews.llvm.org/D49811
Files:
lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
lib/StaticAnalyzer/Core/CoreEngine.cpp
test/Analysis/end-function-return-stmt.cpp
Index: lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -223,8 +223,12 @@
// Get return statement..
const ReturnStmt *RS = nullptr;
if (!L.getSrc()->empty()) {
- if (Optional LastStmt = L.getSrc()->back().getAs()) {
+ CFGElement LastElement = L.getSrc()->back();
+ if (Optional LastStmt = LastElement.getAs()) {
RS = dyn_cast(LastStmt->getStmt());
+ } else if (Optional AutoDtor =
+ LastElement.getAs()) {
+RS = dyn_cast(AutoDtor->getTriggerStmt());
}
}
Index: lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
+++ lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
@@ -16,6 +16,7 @@
#include "ClangSACheckers.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/Analysis/CFGStmtMap.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
@@ -37,6 +38,7 @@
check::PostStmt,
check::PreCall,
check::PostCall,
+ check::EndFunction,
check::NewAllocator,
check::Bind,
check::RegionChanges,
@@ -121,6 +123,23 @@
}
}
+ void checkEndFunction(const ReturnStmt *S, CheckerContext ) const {
+if (isCallbackEnabled(C, "EndFunction")) {
+ llvm::errs() << "EndFunction\nReturnStmt: " << (S ? "yes" : "no") << "\n";
+ if (!S)
+return;
+
+ llvm::errs() << "CFGElement: ";
+ CFGStmtMap *Map = C.getCurrentAnalysisDeclContext()->getCFGStmtMap();
+ CFGElement LastElement = Map->getBlock(S)->back();
+
+ if (LastElement.getAs())
+llvm::errs() << "CFGStmt\n";
+ else if (LastElement.getAs())
+llvm::errs() << "CFGAutomaticObjDtor\n";
+}
+ }
+
void checkNewAllocator(const CXXNewExpr *CNE, SVal Target,
CheckerContext ) const {
if (isCallbackEnabled(C, "NewAllocator"))
Index: test/Analysis/end-function-return-stmt.cpp
===
--- test/Analysis/end-function-return-stmt.cpp
+++ test/Analysis/end-function-return-stmt.cpp
@@ -0,0 +1,34 @@
+//RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:EndFunction=true %s 2>&1 | FileCheck %s
+
+// At the end of a function, we can only obtain a ReturnStmt if the last
+// CFGElement in the CFGBlock is either a CFGStmt or a CFGAutomaticObjDtor.
+
+void noReturnStmt() {}
+
+struct S {
+ S();
+ ~S();
+};
+
+int dtorAfterReturnStmt() {
+ S s;
+ return 0;
+}
+
+S endsWithReturnStmt() {
+ return S();
+}
+
+// endsWithReturnStmt()
+// CHECK: EndFunction
+// CHECK-NEXT: ReturnStmt: yes
+// CHECK-NEXT: CFGElement: CFGStmt
+
+// dtorAfterReturnStmt()
+// CHECK: EndFunction
+// CHECK-NEXT: ReturnStmt: yes
+// CHECK-NEXT: CFGElement: CFGAutomaticObjDtor
+
+// noReturnStmt()
+// CHECK: EndFunction
+// CHECK-NEXT: ReturnStmt: no
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49811: [analyzer] Obtain a ReturnStmt from a CFGAutomaticObjDtor
xazax.hun accepted this revision. xazax.hun added a comment. We could also print something about the ReturnStmt, like source location or pretty print of its expressions so we can check that we picked the right one in case we have multiple. But consider this as an optional task if you have nothing better to do. I am ok with committing this as is. https://reviews.llvm.org/D49811 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49811: [analyzer] Obtain a ReturnStmt from a CFGAutomaticObjDtor
rnkovacs updated this revision to Diff 158680.
rnkovacs added a comment.
In https://reviews.llvm.org/D49811#1175726, @NoQ wrote:
> I guess you could write a test with `debug.AnalysisOrder` (by making its
> `checkEndFunction` callback (that you'll have to define) print different
> things depending on the return statement), not sure if it's worth it; you can
> also merge this commit with https://reviews.llvm.org/D49361 instead.
This is what I could come up with, what do you think?
https://reviews.llvm.org/D49811
Files:
lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
lib/StaticAnalyzer/Core/CoreEngine.cpp
test/Analysis/end-function-return-stmt.cpp
Index: test/Analysis/end-function-return-stmt.cpp
===
--- /dev/null
+++ test/Analysis/end-function-return-stmt.cpp
@@ -0,0 +1,34 @@
+//RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:EndFunction=true %s 2>&1 | FileCheck %s
+
+// At the end of a function, we can only obtain a ReturnStmt if the last
+// CFGElement in the CFGBlock is either a CFGStmt or a CFGAutomaticObjDtor.
+
+void noReturnStmt() {}
+
+struct S {
+ S();
+ ~S();
+};
+
+int dtorAfterReturnStmt() {
+ S s;
+ return 0;
+}
+
+S endsWithReturnStmt() {
+ return S();
+}
+
+// endsWithReturnStmt()
+// CHECK: EndFunction
+// CHECK-NEXT: ReturnStmt: yes
+// CHECK-NEXT: CFGElement: CFGStmt
+
+// dtorAfterReturnStmt()
+// CHECK: EndFunction
+// CHECK-NEXT: ReturnStmt: yes
+// CHECK-NEXT: CFGElement: CFGAutomaticObjDtor
+
+// noReturnStmt()
+// CHECK: EndFunction
+// CHECK-NEXT: ReturnStmt: no
Index: lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -223,8 +223,12 @@
// Get return statement..
const ReturnStmt *RS = nullptr;
if (!L.getSrc()->empty()) {
- if (Optional LastStmt = L.getSrc()->back().getAs()) {
+ CFGElement LastElement = L.getSrc()->back();
+ if (Optional LastStmt = LastElement.getAs()) {
RS = dyn_cast(LastStmt->getStmt());
+ } else if (Optional AutoDtor =
+ LastElement.getAs()) {
+RS = dyn_cast(AutoDtor->getTriggerStmt());
}
}
Index: lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
+++ lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
@@ -16,6 +16,7 @@
#include "ClangSACheckers.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/Analysis/CFGStmtMap.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
@@ -37,6 +38,7 @@
check::PostStmt,
check::PreCall,
check::PostCall,
+ check::EndFunction,
check::NewAllocator,
check::Bind,
check::RegionChanges,
@@ -121,6 +123,23 @@
}
}
+ void checkEndFunction(const ReturnStmt *S, CheckerContext ) const {
+if (isCallbackEnabled(C, "EndFunction")) {
+ llvm::errs() << "EndFunction\nReturnStmt: " << (S ? "yes" : "no") << "\n";
+ if (!S)
+return;
+
+ llvm::errs() << "CFGElement: ";
+ CFGStmtMap *Map = C.getCurrentAnalysisDeclContext()->getCFGStmtMap();
+ CFGElement LastElement = Map->getBlock(S)->back();
+
+ if (LastElement.getAs())
+llvm::errs() << "CFGStmt\n";
+ else if (LastElement.getAs())
+llvm::errs() << "CFGAutomaticObjDtor\n";
+}
+ }
+
void checkNewAllocator(const CXXNewExpr *CNE, SVal Target,
CheckerContext ) const {
if (isCallbackEnabled(C, "NewAllocator"))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49811: [analyzer] Obtain a ReturnStmt from a CFGAutomaticObjDtor
NoQ added a comment. Devin has recently pointed out that we might have as well reordered CFG elements to have return statement kick in after automatic destructors, so that callbacks were called in a different order. I don't see any problems with that solution, but let's stick to the current solution for now, because who knows. Repository: rC Clang https://reviews.llvm.org/D49811 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49811: [analyzer] Obtain a ReturnStmt from a CFGAutomaticObjDtor
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Yet another great catch! I guess you could write a test with `debug.AnalysisOrder` (by making its `checkEndFunction` callback (that you'll have to define) print different things depending on the return statement), not sure if it's worth it; you can also merge this commit with https://reviews.llvm.org/D49361 instead. Repository: rC Clang https://reviews.llvm.org/D49811 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49811: [analyzer] Obtain a ReturnStmt from a CFGAutomaticObjDtor
rnkovacs added a comment. I'm not sure how to test this. I'll need it in https://reviews.llvm.org/D49361 when I update it to use the changed `checkEndFunction()` callback, and that will kind of test this too. Repository: rC Clang https://reviews.llvm.org/D49811 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49811: [analyzer] Obtain a ReturnStmt from a CFGAutomaticObjDtor
rnkovacs created this revision.
rnkovacs added reviewers: NoQ, xazax.hun, george.karpenkov.
Herald added subscribers: mikhail.ramalho, a.sidorin, dkrupp, szepet,
baloghadamsoftware, whisperity.
The `CoreEngine` only gives us a `ReturnStmt` if the last element in the
`CFGBlock` is a `CFGStmt`, otherwise the `ReturnStmt` is `nullptr`.
This patch adds support for the case when the last element is a
`CFGAutomaticObjDtor`, by returning its `TriggerStmt` as a `ReturnStmt`.
Repository:
rC Clang
https://reviews.llvm.org/D49811
Files:
lib/StaticAnalyzer/Core/CoreEngine.cpp
Index: lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -223,8 +223,12 @@
// Get return statement..
const ReturnStmt *RS = nullptr;
if (!L.getSrc()->empty()) {
- if (Optional LastStmt = L.getSrc()->back().getAs()) {
+ CFGElement LastElement = L.getSrc()->back();
+ if (Optional LastStmt = LastElement.getAs()) {
RS = dyn_cast(LastStmt->getStmt());
+ } else if (Optional AutoDtor =
+ LastElement.getAs()) {
+RS = dyn_cast(AutoDtor->getTriggerStmt());
}
}
Index: lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -223,8 +223,12 @@
// Get return statement..
const ReturnStmt *RS = nullptr;
if (!L.getSrc()->empty()) {
- if (Optional LastStmt = L.getSrc()->back().getAs()) {
+ CFGElement LastElement = L.getSrc()->back();
+ if (Optional LastStmt = LastElement.getAs()) {
RS = dyn_cast(LastStmt->getStmt());
+ } else if (Optional AutoDtor =
+ LastElement.getAs()) {
+RS = dyn_cast(AutoDtor->getTriggerStmt());
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
