[Analyzer] Allow creation of stack frame for functions without definition

[baloghadamsoftware]

Summary

Retrieving the parameter location of functions was disabled because it may causes crashes due to the fact that functions may have multiple declarations and without definition it is difficult to ensure that always the same declration is used. Now parameters are stored in ParamRegions which are independent of the declaration of the function, therefore the same parameters always have the same regions, independently of the function declaration used actually. This allows us to remove the limitation described above.

[baloghadamsoftware]

I tested this on several open-source projects: BitCoin, CURL, OpenSLL, PostGreS, TMux, Xerces and even on LLVM itself with most of the projects enabled. No new crash and no change in findings. So it seems to be stable.

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

179

We introduced ParamRegions to overcome this, but please provide me the tests that crash when deleting these lines without ParamRegions you mentioned D49443#1193290.

clang/test/Analysis/temporaries.cpp

893

I wonder whether clang_analyzer_checkInlined() works correctly with this patch: it seems it only checks for stack frame which now any function with definition can have.

[martong]

Retrieving the parameter location of functions was disabled because it may causes crashes due to the fact that functions may have multiple declarations and without definition it is difficult to ensure that always the same declration is used. Now parameters are stored in ParamRegions which are independent of the declaration of the function, therefore the same parameters always have the same regions, independently of the function declaration used actually. This allows us to remove the limitation described above.

I am not sure if this "summary" for the patch is aligned with its title. I think the title says it all, I'd just skip this summary, it is way too blurry for me.
Because:

it may causes crashes

We still need the test cases for that, and I am not sure if we will ever get one.

without definition it is difficult to ensure that always the same declration is used.

That is not difficult in other parts of the compiler infrastructure (e.g in ASTImporter), this statement is just a vague allusion to one or several hypothetical bugs in the analyzer.

the same parameters always have the same regions, independently of the function declaration used actually.

You mentioned personally to me already, but don't forget to add test cases for that.

clang/test/Analysis/temporaries.cpp

893

So, why not try that in a test with a function that does not have a definition?

[baloghadamsoftware]

Added unittest for checking the sameness for the regions of same params with different redeclarations.

[martong]

clang/unittests/StaticAnalyzer/ParamRegionTest.cpp

22

I am concerned about the redeclaration chain of ParmVarDecls. In the following example:

void foo(int a);
void foo(int a);

we have the prev decl set only for the FunctionDecls and not for the ParmVarDecl (please double check).
So in the test we should go through the redecls() of the FunctionDecl of the ParmVarDecl. And we should get a PVD from each redeclaration by the index.

84

I think a raw string literal with clang-formatted code in it would make the test more valuable.

[NoQ]

This looks straightforward, thanks!

clang/test/Analysis/explain-svals.cpp

101

Wonderful!

I think "parameter 1" would make a bit more sense. Or, even better, "1st parameter" like in regular warnings.

(this probably needs to be fixed in an older patch)

clang/test/Analysis/temporaries.cpp

893

it seems it only checks for stack frame which now any function with definition can have

Before that, it checks that clang_analyzer_checkInlined() was evaluated during analysis. You can't evaluate this line if you didn't analyze ~C() either as top-level function or through inlining.

clang/unittests/StaticAnalyzer/ParamRegionTest.cpp

84

I also think it's going to be valuable to test what's going to happen if the forward-declaration is after the definition; that's where we were previously confused.

[baloghadamsoftware]

SVal explanation changed to Xth parameter format. Unit test changed: there is redeclaration after the definition now, all test code in raw string.

[saugustine]

Unfortunately, after this change there are several variables only used in asserts, which creates build failures when assertions are disabled.

I will be submitting https://reviews.llvm.org/D81522 shortly to fix.

[baloghadamsoftware]

In D80286#2083753, @saugustine wrote:

Unfortunately, after this change there are several variables only used in asserts, which creates build failures when assertions are disabled.

I will be submitting https://reviews.llvm.org/D81522 shortly to fix.

What is the point of building unit tests with assertions disabled? To me this looks like a CMake configuration problem. Unit tests should always be built with assertions enabled since they are based on assertions. They are just tests, they do not decrease the performance of the production code.

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

bool CallEvent::isGlobalCFunction(StringRef FunctionName) const {
   return CheckerContext::isCLibraryFunction(FD, FunctionName);
 }
 
 AnalysisDeclContext *CallEvent::getCalleeAnalysisDeclContext() const {
   const Decl *D = getDecl();
   if (!D)
     return nullptr;
 
-  // TODO: For now we skip functions without definitions, even if we have
-  // our own getDecl(), because it's hard to find out which re-declaration
-  // is going to be used, and usually clients don't really care about this
-  // situation because there's a loss of precision anyway because we cannot
-  // inline the call.
-  RuntimeDefinition RD = getRuntimeDefinition();
-  if (!RD.getDecl())
-    return nullptr;
-
   AnalysisDeclContext *ADC =
       LCtx->getAnalysisDeclContext()->getManager()->getContext(D);
 
-  // TODO: For now we skip virtual functions, because this also rises
-  // the problem of which decl to use, but now it's across different classes.
-  if (RD.mayHaveOtherDefinitions() || RD.getDecl() != ADC->getDecl())
-    return nullptr;
-
   return ADC;
 }
 
 const StackFrameContext *
 CallEvent::getCalleeStackFrame(unsigned BlockCount) const {
   AnalysisDeclContext *ADC = getCalleeAnalysisDeclContext();
   if (!ADC)
     return nullptr;

clang/test/Analysis/explain-svals.cpp

void test_5(int i) {
     clang_analyzer_explain(this); // expected-warning-re{{{{^pointer to 'this' object$}}}}
     clang_analyzer_explain(&x[i]); // expected-warning-re{{{{^pointer to element of type 'int' with index 'argument 'i'' of field 'x' of 'this' object$}}}}
     clang_analyzer_explain(__builtin_alloca(i)); // expected-warning-re{{{{^pointer to region allocated by '__builtin_alloca\(i\)'$}}}}
   }
 };
 } // end of anonymous namespace
 
 void test_6() {
-  clang_analyzer_explain(conjure_S()); // expected-warning-re{{{{^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$}}}}
+  clang_analyzer_explain(conjure_S()); // expected-warning-re{{{{^lazily frozen compound value of 1st parameter of function 'clang_analyzer_explain\(\)'$}}}}
   clang_analyzer_explain(conjure_S().z); // expected-warning-re{{{{^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$}}}}
 }
 
 class C_top_level {
 public:
   C_top_level(int param) {
     clang_analyzer_explain(&param); // expected-warning-re{{{{^pointer to parameter 'param'$}}}}
   }

clang/test/Analysis/temporaries.cpp

 
 namespace dont_forget_destructor_around_logical_op {
 int glob;
 
 class C {
 public:
   ~C() {
     glob = 1;
-    // FIXME: Why is destructor not inlined in C++17
     clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-    // expected-warning@-3{{TRUE}}
-#endif
+    // expected-warning@-2{{TRUE}}
 #endif
   }
 };
 
 C get();
 
 bool is(C);
 
 
 void test(int coin) {
   // Here temporaries are being cleaned up after && is evaluated. There are two
   // temporaries: the return value of get() and the elidable copy constructor
   // of that return value into is(). According to the CFG, we need to cleanup
   // both of them depending on whether the temporary corresponding to the
   // return value of get() was initialized. However, we didn't track
   // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
   if (coin) {
-    // FIXME: Why is destructor not inlined in C++17
     clang_analyzer_eval(glob);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-    // expected-warning@-3{{TRUE}}
-#else
-    // expected-warning@-5{{UNKNOWN}}
-#endif
+    // expected-warning@-2{{TRUE}}
 #else
-    // expected-warning@-8{{UNKNOWN}}
+    // expected-warning@-4{{UNKNOWN}}
 #endif
   } else {
     // The destructor is not called on this branch.
     clang_analyzer_eval(glob); // expected-warning{{UNKNOWN}}
   }
 }
 } // namespace dont_forget_destructor_around_logical_op
 

clang/unittests/StaticAnalyzer/ParamRegionTest.cpp

 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace ento {
 namespace {
 
 class ParamRegionTestConsumer : public ExprEngineConsumer {
+  void checkForSameParamRegions(MemRegionManager &MRMgr,
+                                const StackFrameContext *SFC,
+                                const ParmVarDecl *PVD) {
+    for (const auto *D2: PVD->redecls()) {
+      const auto *PVD2 = cast<ParmVarDecl>(D2);
+      assert(MRMgr.getVarRegion(PVD, SFC) == MRMgr.getVarRegion(PVD2, SFC));
+    }
+  }
+
   void performTest(const Decl *D) {
     StoreManager &StMgr = Eng.getStoreManager();
     MemRegionManager &MRMgr = StMgr.getRegionManager();
     const StackFrameContext *SFC =
         Eng.getAnalysisDeclContextManager().getStackFrame(D);
 
     if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
       for (const auto *P : FD->parameters()) {
         const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
         if (SFC->inTopFrame())
           assert(isa<NonParamVarRegion>(Reg));
         else
           assert(isa<ParamVarRegion>(Reg));
+        checkForSameParamRegions(MRMgr, SFC, P);
       }
     } else if (const auto *CD = dyn_cast<CXXConstructorDecl>(D)) {
       for (const auto *P : CD->parameters()) {
         const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
         if (SFC->inTopFrame())
           assert(isa<NonParamVarRegion>(Reg));
         else
           assert(isa<ParamVarRegion>(Reg));
+        checkForSameParamRegions(MRMgr, SFC, P);
       }
     } else if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
       for (const auto *P : MD->parameters()) {
         const TypedValueRegion *Reg = MRMgr.getVarRegion(P, SFC);
         if (SFC->inTopFrame())
           assert(isa<NonParamVarRegion>(Reg));
         else
           assert(isa<ParamVarRegion>(Reg));
+        checkForSameParamRegions(MRMgr, SFC, P);
       }
     }
   }
 
 public:
   ParamRegionTestConsumer(CompilerInstance &C) : ExprEngineConsumer(C) {}
 
   bool HandleTopLevelDecl(DeclGroupRef DG) override {
     for (const auto *D : DG) {
       performTest(D);
     }
     return true;
   }
 };
 
 class ParamRegionTestAction : public ASTFrontendAction {
 public:
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
                                                  StringRef File) override {
     return std::make_unique<ParamRegionTestConsumer>(Compiler);
   }
 };
 
 TEST(ParamRegion, ParamRegionTest) {
   EXPECT_TRUE(
       tooling::runToolOnCode(std::make_unique<ParamRegionTestAction>(),
-                             R"(void foo(int n) {
+                             R"(void foo(int n);
+                                void baz(int p);
+
+                                void foo(int n) {
                                   auto lambda = [n](int m) {
                                     return n + m;
                                   };
 
                                   int k = lambda(2);
                                 }
 
                                 void bar(int l) {
                                   foo(l);
                                 }
 
                                 struct S {
                                   int n;
                                   S(int nn): n(nn) {}
                                 };
 
                                 void baz(int p) {
                                   S s(p);
-                                })"));
+                                }
+
+                                void bar(int l);
+                                void baz(int p);)"));
   EXPECT_TRUE(
       tooling::runToolOnCode(std::make_unique<ParamRegionTestAction>(),
                              R"(@interface O
                                 + alloc;
                                 - initWithInt:(int)q;
                                 @end
 
                                 void qix(int r) {
                                   O *o = [[O alloc] initWithInt:r];
                                 })",
                              "input.m"));
 }
 
 } // namespace
 } // namespace ento
 } // namespace clang