[analyzer] Obtain a ReturnStmt from a CFGAutomaticObjDtor

[rnkovacs]

Summary

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.

[rnkovacs]

I'm not sure how to test this.
I'll need it in D49361 when I update it to use the changed checkEndFunction() callback, and that will kind of test this too.

[NoQ]

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 D49361 instead.

[NoQ]

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.

[rnkovacs]

In 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 D49361 instead.

This is what I could come up with, what do you think?

[xazax.hun]

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.

[NoQ]

In 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.

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

 // This is required to ensure that callbacks are fired in order
 // and do not duplicate or get lost.
 // Feel free to extend this checker with any callback you need to check.
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 
 class AnalysisOrderChecker
     : public Checker<check::PreStmt<CastExpr>,
                      check::PostStmt<CastExpr>,
                      check::PreStmt<ArraySubscriptExpr>,
                      check::PostStmt<ArraySubscriptExpr>,
                      check::PreStmt<CXXNewExpr>,
                      check::PostStmt<CXXNewExpr>,
                      check::PreStmt<OffsetOfExpr>,
                      check::PostStmt<OffsetOfExpr>,
                      check::PreCall,
                      check::PostCall,
+                     check::EndFunction,
                      check::NewAllocator,
                      check::Bind,
                      check::RegionChanges,
                      check::LiveSymbols> {
 
   bool isCallbackEnabled(AnalyzerOptions &Opts, StringRef CallbackName) const {
     return Opts.getBooleanOption("*", false, this) ||
         Opts.getBooleanOption(CallbackName, false, this);
void checkPostCall(const CallEvent &Call, CheckerContext &C) const {
     if (isCallbackEnabled(C, "PostCall")) {
       llvm::errs() << "PostCall";
       if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(Call.getDecl()))
         llvm::errs() << " (" << ND->getQualifiedNameAsString() << ')';
       llvm::errs() << '\n';
     }
   }
 
+  void checkEndFunction(const ReturnStmt *S, CheckerContext &C) 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<CFGStmt>())
+        llvm::errs() << "CFGStmt\n";
+      else if (LastElement.getAs<CFGAutomaticObjDtor>())
+        llvm::errs() << "CFGAutomaticObjDtor\n";
+    }
+  }
+
   void checkNewAllocator(const CXXNewExpr *CNE, SVal Target,
                          CheckerContext &C) const {
     if (isCallbackEnabled(C, "NewAllocator"))
       llvm::errs() << "NewAllocator\n";
   }
 
   void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const {
     if (isCallbackEnabled(C, "Bind"))

lib/StaticAnalyzer/Core/CoreEngine.cpp

void CoreEngine::HandleBlockEdge(const BlockEdge &L, ExplodedNode *Pred) {
   // Check if we are entering the EXIT block.
   if (Blk == &(L.getLocationContext()->getCFG()->getExit())) {
     assert(L.getLocationContext()->getCFG()->getExit().empty() &&
            "EXIT block cannot contain Stmts.");
 
     // Get return statement..
     const ReturnStmt *RS = nullptr;
     if (!L.getSrc()->empty()) {
-      if (Optional<CFGStmt> LastStmt = L.getSrc()->back().getAs<CFGStmt>()) {
+      CFGElement LastElement = L.getSrc()->back();
+      if (Optional<CFGStmt> LastStmt = LastElement.getAs<CFGStmt>()) {
         RS = dyn_cast<ReturnStmt>(LastStmt->getStmt());
+      } else if (Optional<CFGAutomaticObjDtor> AutoDtor =
+                 LastElement.getAs<CFGAutomaticObjDtor>()) {
+        RS = dyn_cast<ReturnStmt>(AutoDtor->getTriggerStmt());
       }
     }
 
     // Process the final state transition.
     SubEng.processEndOfFunction(BuilderCtx, Pred, RS);
 
     // This path is done. Don't enqueue any more nodes.
     return;

test/Analysis/end-function-return-stmt.cpp

+//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