[analyzer] Associate diagnostics created in checkEndFunction with a return statement, if possible

[george.karpenkov]

Summary

If not possible, use the last line of the declaration, as before.

[NoQ]

clang/include/clang/Analysis/ProgramPoint.h

343–345

I suspect we can always retrieve it as LC->getCFG()->getExit(), no need to store it separately.

clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

834–836

This looks correct, but my completely irrational intuition suggests that createBegin() might be more appropriate. I.e., we're not computing something at the ; that follows the return-statement, but instead we are already below the return statement.

[george.karpenkov]

clang/include/clang/Analysis/ProgramPoint.h

343–345

I think it's better to be consistent with the surrounding code here,
to make it obvious it behaves in the same way as BlockEntrancePoint

clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

834–836

Does it matter though? What is the difference?

[NoQ]

clang/include/clang/Analysis/ProgramPoint.h

343–345

I don't think it this program point has anything in common with BlockEntrancePoint, apart from the historical misuse of the latter.

clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

834–836

It'll affect the column on which the report would be thrown, assuming code is written normally and the return value is short enough (otherwise it may also affect line).

[NoQ]

Yay!

clang/include/clang/Analysis/ProgramPoint.h

enum Kind { BlockEdgeKind,
               PostLValueKind,
               PostAllocatorCallKind,
               MinPostStmtKind = PostStmtKind,
               MaxPostStmtKind = PostAllocatorCallKind,
               PostInitializerKind,
               CallEnterKind,
               CallExitBeginKind,
               CallExitEndKind,
+              FunctionExitKind,
               PreImplicitCallKind,
               PostImplicitCallKind,
               MinImplicitCallKind = PreImplicitCallKind,
               MaxImplicitCallKind = PostImplicitCallKind,
               LoopExitKind,
               EpsilonKind};
 
 private:

 private:
   friend class ProgramPoint;
   static bool isKind(const ProgramPoint &Location) {
     unsigned k = Location.getKind();
     return k >= MinPostStmtKind && k <= MaxPostStmtKind;
   }
 };
 
+class FunctionExitPoint : public ProgramPoint {
+public:
+  explicit FunctionExitPoint(const CFGBlock *B, const ReturnStmt *S,
+                             const LocationContext *LC,
+                             const ProgramPointTag *tag = nullptr)
+      : ProgramPoint(B, S, FunctionExitKind, LC, tag) {
+    assert((B || S) &&
+           "FunctionExitPoint requires non-null block or statement");
+  }
+
+  const CFGBlock *getBlock() const {
+    return reinterpret_cast<const CFGBlock*>(getData1());
+  }
+
+  const ReturnStmt *getStmt() const {
+    return reinterpret_cast<const ReturnStmt *>(getData2());
+  }
+
+private:
+  friend class ProgramPoint;
+  FunctionExitPoint() = default;
+  static bool isKind(const ProgramPoint &Location) {
+    return Location.getKind() == FunctionExitKind;
+  }
+};
+
 // PostCondition represents the post program point of a branch condition.
 class PostCondition : public PostStmt {
 public:
   PostCondition(const Stmt *S, const LocationContext *L,
                 const ProgramPointTag *tag = nullptr)
     : PostStmt(S, PostConditionKind, L, tag) {}
 
 private:

clang/lib/StaticAnalyzer/Core/CheckerManager.cpp

void CheckerManager::runCheckersForEndFunction(NodeBuilderContext &BC,
                                                ExplodedNode *Pred,
                                                ExprEngine &Eng,
                                                const ReturnStmt *RS) {
   // We define the builder outside of the loop bacause if at least one checkers
   // creates a sucsessor for Pred, we do not need to generate an
   // autotransition for it.
   NodeBuilder Bldr(Pred, Dst, BC);
   for (const auto checkFn : EndFunctionCheckers) {
-    const ProgramPoint &L = BlockEntrance(BC.Block,
-                                          Pred->getLocationContext(),
-                                          checkFn.Checker);
+    const ProgramPoint &L = FunctionExitPoint(
+        BC.Block,
+        RS,
     CheckerContext C(Bldr, Eng, Pred, L);
     checkFn(RS, C);
   }
 }
 
 namespace {
 
   struct CheckBranchConditionContext {

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

                                llvm::raw_string_ostream &Out) {
     const SourceManager &SM = Context.getSourceManager();
     switch (Loc.getKind()) {
     case ProgramPoint::BlockEntranceKind:
       Out << "Block Entrance: B"
           << Loc.castAs<BlockEntrance>().getBlock()->getBlockID();
       break;
 
+    case ProgramPoint::FunctionExitKind: {
+      auto FEP = Loc.getAs<FunctionExitPoint>();
+      Out << "Function Exit: B"
+          << FEP->getBlock()->getBlockID();
+      if (const ReturnStmt *RS = FEP->getStmt()) {
+        Out << "\\l Return: S" << RS->getID(Context) << "\\l";
+        RS->printPretty(Out, /*helper=*/nullptr, Context.getPrintingPolicy(),
+                       /*Indentation=*/2, /*NewlineSymbol=*/"\\l");
+      }
+      break;
+    }
     case ProgramPoint::BlockExitKind:
       assert(false);
       break;
 
     case ProgramPoint::CallEnterKind:
       Out << "CallEnter";
       break;
 

clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

const Stmt *PathDiagnosticLocation::getStmt(const ExplodedNode *N) {
   const LocationContext *LC = N->getLocationContext();
   if (LC->getAnalysisDeclContext()->isBodyAutosynthesized()) {
     // It must be a stack frame because we only autosynthesize functions.
     return cast<StackFrameContext>(findTopAutosynthesizedParentContext(LC))
         ->getCallSite();
   }
   // Otherwise, see if the node's program point directly points to a statement.
   ProgramPoint P = N->getLocation();
-  if (Optional<StmtPoint> SP = P.getAs<StmtPoint>())
+  if (auto SP = P.getAs<StmtPoint>())
     return SP->getStmt();
-  if (Optional<BlockEdge> BE = P.getAs<BlockEdge>())
+  if (auto BE = P.getAs<BlockEdge>())
     return BE->getSrc()->getTerminator();
-  if (Optional<CallEnter> CE = P.getAs<CallEnter>())
+  if (auto CE = P.getAs<CallEnter>())
     return CE->getCallExpr();
-  if (Optional<CallExitEnd> CEE = P.getAs<CallExitEnd>())
+  if (auto CEE = P.getAs<CallExitEnd>())
     return CEE->getCalleeContext()->getCallSite();
-  if (Optional<PostInitializer> PIPP = P.getAs<PostInitializer>())
+  if (auto PIPP = P.getAs<PostInitializer>())
     return PIPP->getInitializer()->getInit();
-  if (Optional<CallExitBegin> CEB = P.getAs<CallExitBegin>())
+  if (auto CEB = P.getAs<CallExitBegin>())
     return CEB->getReturnStmt();
+  if (auto FEP = P.getAs<FunctionExitPoint>())
+    return FEP->getStmt();
 
   return nullptr;
 }
 
 const Stmt *PathDiagnosticLocation::getNextStmt(const ExplodedNode *N) {
   for (N = N->getFirstSucc(); N; N = N->getFirstSucc()) {
     if (const Stmt *S = getStmt(N)) {
       // Check if the statement is '?' or '&&'/'||'.  These are "merges",
const Stmt *PathDiagnosticLocation::getNextStmt(const ExplodedNode *N) {
   return nullptr;
 }
 
 PathDiagnosticLocation
   PathDiagnosticLocation::createEndOfPath(const ExplodedNode *N,
                                           const SourceManager &SM) {
   assert(N && "Cannot create a location with a null node.");
   const Stmt *S = getStmt(N);
+  const LocationContext *LC = N->getLocationContext();
 
   if (!S) {
     // If this is an implicit call, return the implicit call point location.
     if (Optional<PreImplicitCall> PIE = N->getLocationAs<PreImplicitCall>())
       return PathDiagnosticLocation(PIE->getLocation(), SM);
+    if (auto FE = N->getLocationAs<FunctionExitPoint>()) {
+      // TODO: is that correct?
+      if (const ReturnStmt *RS = FE->getStmt())
+        return PathDiagnosticLocation::createEnd(RS, SM, LC);
+    }
     S = getNextStmt(N);
   }
 
   if (S) {
     ProgramPoint P = N->getLocation();
-    const LocationContext *LC = N->getLocationContext();
 
     // For member expressions, return the location of the '.' or '->'.
     if (const auto *ME = dyn_cast<MemberExpr>(S))
       return PathDiagnosticLocation::createMemberLoc(ME, SM);
 
     // For binary operators, return the location of the operator.
     if (const auto *B = dyn_cast<BinaryOperator>(S))
       return PathDiagnosticLocation::createOperatorLoc(B, SM);

clang/test/Analysis/inner-pointer.cpp

const char *escape_via_return_temp() {
   // expected-warning@-2 {{Inner pointer of container used after re/deallocation}}
   // expected-note@-3 {{Inner pointer of container used after re/deallocation}}
 }
 
 const char *escape_via_return_local() {
   std::string s;
   return s.c_str(); // expected-note {{Pointer to inner buffer of 'std::string' obtained here}}
                     // expected-note@-1 {{Inner buffer of 'std::string' deallocated by call to destructor}}
-} // expected-warning {{Inner pointer of container used after re/deallocation}}
-// expected-note@-1 {{Inner pointer of container used after re/deallocation}}
+                    // expected-warning@-2 {{Inner pointer of container used after re/deallocation}}
+                    // expected-note@-3 {{Inner pointer of container used after re/deallocation}}
+}
 
 
 char *c();
 class A {};
 
 void no_CXXRecordDecl() {
   A a, *b;
   *(void **)&b = c() + 1;
   *b = a; // no-crash
 }
 
 void checkReference(std::string &s) {
   const char *c = s.c_str();
 }

clang/test/Analysis/malloc-free-after-return.cpp

 };
 
 int *freeAfterReturnTemp() {
   return S().getData(); // expected-warning {{Use of memory after it is freed}}
 }
 
 int *freeAfterReturnLocal() {
   S X;
-  return X.getData();
-} // expected-warning {{Use of memory after it is freed}}
+  return X.getData(); // expected-warning {{Use of memory after it is freed}}
+}