[analyzer] ExprEngine: Fill the Store with initial value of parameters

[Charusso]

Summary

This patch modifies the ExprEngine::getInitialState() by forcing the
creation of the initial values of the parameters of the first function call.

With that we could point out the origin of such values in bug reporting.

[NoQ]

This change in the state adds no new information to the state while also burdening the Store with more data to carry and making debugging harder (not only dumps will now have a bunch of explicit bindings, but also these bindings will be much bulkier being derived symbols).

If you want better tracking, just fix the tracking. You already have all the necessary information in the Store to do that.

clang/test/Analysis/osobject-retain-release.cpp

73

The original message is pretty bad but i don't think it became better. The object wasn't "stored into b" by the user; the user simply called a function. The way you made the Store look as if the object was actively stored didn't help; it's up to the visitor to explain why the store looked the way it looked, it's not the job of the Store to look like something else to appease the visitor.

[Charusso]

In D81062#2070823, @NoQ wrote:

This change in the state adds no new information to the state while also burdening the Store with more data to carry and making debugging harder (not only dumps will now have a bunch of explicit bindings, but also these bindings will be much bulkier being derived symbols).

I do not see any debug burden because there were no information to begin the debug. The *new* information occurs at the appropriate place now.

If you want better tracking, just fix the tracking. You already have all the necessary information in the Store to do that.

To fix the tracking we need to provide a NoteTag at the point when we encounter a MemRegion. We need to store these MemRegions so that later we could mark them interesting. If we mark them interesting later we also show such notes. That is why this patch and patches like that are essential to begin this change.

PS: To fix the tracking we also need to fill the members of a record type and remove the LazyCompoundVal. If you think we should not store too much data in the Store then transferring the trackExpressionValue() API to NoteTags is insoluble. We cannot create NoteTags on something which is not in the Store already.

clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h

public:
   }
 
   std::string VisitSymbolConjured(const SymbolConjured *S) {
     return "symbol of type '" + S->getType().getAsString() +
            "' conjured at statement '" + printStmt(S->getStmt()) + "'";
   }
 
   std::string VisitSymbolDerived(const SymbolDerived *S) {
-    return "value derived from (" + Visit(S->getParentSymbol()) +
+    std::string Prefix = "";
+    if (const auto *SRV = dyn_cast<SymbolRegionValue>(S->getParentSymbol()))
+      Prefix = "initial ";
+    return Prefix + "value derived from (" + Visit(S->getParentSymbol()) +
            ") for " + Visit(S->getRegion());
   }
 
   std::string VisitSymbolExtent(const SymbolExtent *S) {
     return "extent of " + Visit(S->getRegion());
   }
 
   std::string VisitSymbolMetadata(const SymbolMetadata *S) {

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

if (!MD->isStatic()) {
         if (Optional<Loc> LV = V.getAs<Loc>()) {
           state = state->assume(*LV, true);
           assert(state && "'this' cannot be null");
         }
       }
     }
   }
 
+  if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+    // Fill the Store with initial value of parameters.
+    for (const ParmVarDecl *PVD : FD->parameters()) {
+      const auto *TVR =
+          state->getRegion(PVD, InitLoc)->castAs<TypedValueRegion>();
+      state = state->bindLoc(loc::MemRegionVal(TVR),
+                             svalBuilder.getRegionValueSymbolVal(TVR), InitLoc);
+    }
+  }
+
   return state;
 }
 
 ProgramStateRef ExprEngine::createTemporaryRegionIfNeeded(
     ProgramStateRef State, const LocationContext *LC,
     const Expr *InitWithAdjustments, const Expr *Result,
     const SubRegion **OutRegionWithAdjustments) {
   // FIXME: This function is a hack that works around the quirky AST

clang/test/Analysis/explain-svals.c

void test_1(int param, void *ptr) {
   clang_analyzer_explain_voidp(ptr);   // expected-warning-re{{{{^argument 'ptr'$}}}}
   if (param == 42)
     clang_analyzer_explain_int(param); // expected-warning-re{{{{^signed 32-bit integer '42'$}}}}
 }
 
 void test_2(struct S s) {
   clang_analyzer_explain_S(s);      //expected-warning-re{{{{^lazily frozen compound value of parameter 's'$}}}}
   clang_analyzer_explain_voidp(&s); // expected-warning-re{{{{^pointer to parameter 's'$}}}}
-  clang_analyzer_explain_int(s.z);  // expected-warning-re{{{{^initial value of field 'z' of parameter 's'$}}}}
+  clang_analyzer_explain_int(s.z);  // expected-warning-re{{{{^initial value derived from \(argument 's'\) for field 'z' of parameter 's'$}}}}
 }

clang/test/Analysis/explain-svals.cpp

void test_2(char *ptr, int ext) {
   clang_analyzer_explain(x); // expected-warning-re{{{{^pointer to element of type 'int' with index 0 of heap segment that starts at symbol of type 'int \*' conjured at statement 'new int \[ext\]'$}}}}
   // Sic! What gets computed is the extent of the element-region.
   clang_analyzer_explain(clang_analyzer_getExtent(x)); // expected-warning-re{{{{^signed 32-bit integer '4'$}}}}
   delete[] x;
 }
 
 void test_3(S s) {
   clang_analyzer_explain(&s); // expected-warning-re{{{{^pointer to parameter 's'$}}}}
-  clang_analyzer_explain(s.z); // expected-warning-re{{{{^initial value of field 'z' of parameter 's'$}}}}
+  clang_analyzer_explain(s.z); // expected-warning-re{{{{^initial value derived from \(argument 's'\) for field 'z' of parameter 's'$}}}}
   clang_analyzer_explain(&s.s2[5].y[3]); // expected-warning-re{{{{^pointer to element of type 'int' with index 3 of field 'y' of base object 'S::S3' inside element of type 'struct S::S2' with index 5 of field 's2' of parameter 's'$}}}}
   if (!s.s2[7].x) {
     clang_analyzer_explain(s.s2[7].x); // expected-warning-re{{{{^concrete memory address '0'$}}}}
     // FIXME: we need to be explaining '1' rather than '0' here; not explainer bug.
     clang_analyzer_explain(s.s2[7].x + 1); // expected-warning-re{{{{^concrete memory address '0'$}}}}
   }
 }
 

clang/test/Analysis/expr-inspection.c

for (; y < 3; ++y) {
     }
   }
 }
 
 // CHECK:      "program_state": {
 // CHECK-NEXT:   "store": { "pointer": "{{0x[0-9a-f]+}}", "items": [
 // CHECK-NEXT:     { "cluster": "y", "pointer": "{{0x[0-9a-f]+}}", "items": [
 // CHECK-NEXT:       { "kind": "Direct", "offset": 0, "value": "2 S32b" }
+// CHECK-NEXT:     ]},
+// CHECK-NEXT:     { "cluster": "x", "pointer": "{{0x[0-9a-f]+}}", "items": [
+// CHECK-NEXT:       { "kind": "Direct", "offset": 0, "value": "reg_$0<int x>" }
 // CHECK-NEXT:     ]}
 // CHECK-NEXT:   ]},
 // CHECK-NEXT:   "environment": { "pointer": "{{0x[0-9a-f]+}}", "items": [
 // CHECK-NEXT:     { "lctx_id": {{[0-9]+}}, "location_context": "#0 Call", "calling": "foo", "location": null, "items": [
 // CHECK-NEXT:       { "stmt_id": {{[0-9]+}}, "pretty": "clang_analyzer_printState", "value": "&code{clang_analyzer_printState}" }
 // CHECK-NEXT:     ]}
 // CHECK-NEXT:   ]},
 // CHECK-NEXT:   "constraints": [

clang/test/Analysis/osobject-retain-release.cpp

static OSArray *generateArrayHasCode() {
     return new OSArray;
   }
 
   static OSArray *withCapacity(unsigned int capacity);
   static void consumeArray(OS_CONSUME OSArray * array);
 
   static OSArray* consumeArrayHasCode(OS_CONSUME OSArray * array) { // expected-note{{Parameter 'array' starts at +1, as it is marked as consuming}}
     return nullptr; // expected-warning{{Potential leak of an object of type 'OSArray'}}
-// expected-note@-1{{Object leaked: allocated object of type 'OSArray' is not referenced later in this execution path and has a retain count of +1}}
+// expected-note@-1{{Object leaked: object allocated and stored into 'array' is not referenced later in this execution path and has a retain count of +1}}
   }
 
 
   static OS_RETURNS_NOT_RETAINED OSArray *MaskedGetter();
   static OS_RETURNS_RETAINED OSArray *getOoopsActuallyCreate();
 
   static const OSMetaClass * const metaClass;
 };
bool test_meta_cast_no_leak(OSMetaClassBase *arg) {
   return arg && arg->metaCast("blah") != nullptr;
 }
 
 static void consumedMismatch(OS_CONSUME OSObject *a,
                              OSObject *b) { // expected-note{{Parameter 'b' starts at +0}}
   a->release();
   b->retain(); // expected-note{{Reference count incremented. The object now has a +1 retain count}}
 } // expected-warning{{Potential leak of an object of type 'OSObject'}}
-// expected-note@-1{{Object leaked: allocated object of type 'OSObject' is not referenced later in this execution path and has a retain count of +1}}
+// expected-note@-1{{Object leaked: object allocated and stored into 'b' is not referenced later in this execution path and has a retain count of +1}}
 
 void escape(void *);
 void escape_with_source(void *p) {}
 bool coin();
 
 typedef int kern_return_t;
 typedef kern_return_t IOReturn;
 typedef kern_return_t OSReturn;