[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 ↗

(On Diff #166370)

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

clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

832–834 ↗

(On Diff #166370)

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 ↗

(On Diff #166370)

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

832–834 ↗

(On Diff #166370)

Does it matter though? What is the difference?

[NoQ]

clang/include/clang/Analysis/ProgramPoint.h

343–345 ↗

(On Diff #166370)

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

832–834 ↗

(On Diff #166370)

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!

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 ReturnStmt *S,
+                             const LocationContext *LC,
+                             const ProgramPointTag *tag = nullptr)
+      : ProgramPoint(S, FunctionExitKind, LC, tag) {}
+
+  const CFGBlock *getBlock() const {
+    return &getLocationContext()->getCFG()->getExit();
+  }
+
+  const ReturnStmt *getStmt() const {
+    return reinterpret_cast<const ReturnStmt *>(getData1());
+  }
+
+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:

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(RS, Pred->getLocationContext(), checkFn.Checker);
     CheckerContext C(Bldr, Eng, Pred, L);
     checkFn(RS, C);
   }
 }
 
 namespace {
 
   struct CheckBranchConditionContext {

lib/StaticAnalyzer/Core/ExprEngine.cpp

static void dumpProgramPoint(ProgramPoint Loc,
                                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;
 

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>()) {
+      if (const ReturnStmt *RS = FE->getStmt())
+        return PathDiagnosticLocation::createBegin(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);

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();
 }

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