[analyzer] Fix clang_analyzer_getExtent for heap regions

[steakhal]

Summary

This patch fixes an interesting case with the clang_analyzer_getExtent analyzer debug intrinsic.

Previously, one could not query the extent for a heap-allocated object.
I'm resolving this issue, by querying the extent of the base region of the given region.
This way, we will be able to query the extent of a new/malloced region in tests.

This should not change any meaningful behavior inside the analyzer.

[vsavchenko]

Looking great! Thanks!

[vsavchenko]

clang/test/Analysis/explain-svals.cpp

55–56

I guess this should go now, right?

[martong]

clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp

257

Wait a bit, I think it should consider the offset too. So, getDynamicSizeWithOffset should be used IMHO.

[martong]

clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp

257

Or we should have a clang_analyzer_getExtentOfBaseRegion to be precise.

[martong]

Ah, I see you need nonloc::SymbolVal in your next patch, and getDynamicSizeWithOffset returns an Unknown if the extend is symbolic.

Anyway, I still feel misleading that clang_analyzer_getExtent does not handle the offset. Could we change getDynamicSizeWithOffset to return with a symbolic offset instead unknown without regression?

[steakhal]

In D99658#2661452, @martong wrote:

Ah, I see you need nonloc::SymbolVal in your next patch, and getDynamicSizeWithOffset returns an Unknown if the extend is symbolic.

Anyway, I still feel misleading that clang_analyzer_getExtent does not handle the offset. Could we change getDynamicSizeWithOffset to return with a symbolic offset instead unknown without regression?

I'm gonna investigate. Thank you all for the snappy review!

BTW I really don't like the naming of these. Both 'dynamic' and 'size' are somewhat overloaded. I very much prefer extent to any of these.

clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp

257

I was thinking exactly the same!
I would rather choose getDynamicSizeWithOffset() instead of creating another one.

clang/test/Analysis/explain-svals.cpp

55–56

uh I missed it :D

[steakhal]

Fix comments.
I could not manage to create an unknown extent, where the behavior would diverge.

[martong]

LG!

[NoQ]

What about clang_analyzer_getExtent(&x[2]) then?

[steakhal]

In D99658#2665730, @NoQ wrote:

What about clang_analyzer_getExtent(&x[2]) then?

I guess (extent of heap segment that starts at symbol of type 'int *' conjured at statement 'new int [ext]') - 8 is the value I expect - which is the size of the remaining part of the memory region from x+2 in bytes.
We can argue about the semantics of this debug intrinsic or the name of it though.

[NoQ]

I mean, the extent of an ElementRegion is the size of a single element. The reason why our intrinsic isn't doing what you expect is because we represent the pointer with offset as ElementRegion regardless of whether operator [] was used. On the other hand, an SVal doesn't ever represent a region at all, it only points to its first byte, regardless of the structure of MemRegion inside it; for that reason clang_analyzer_getExtent() is impossible to implement correctly in our current model.

So i think both behaviors are incorrect but if you think it makes it easier to write tests then absolutely go for it!

[steakhal]

In D99658#2671747, @NoQ wrote:

I mean, the extent of an ElementRegion is the size of a single element. The reason why our intrinsic isn't doing what you expect is because we represent the pointer with offset as ElementRegion regardless of whether operator [] was used. On the other hand, an SVal doesn't ever represent a region at all, it only points to its first byte, regardless of the structure of MemRegion inside it; for that reason clang_analyzer_getExtent() is impossible to implement correctly in our current model.

So i think both behaviors are incorrect but if you think it makes it easier to write tests then absolutely go for it!

Oh, I got it. Hm, yes, both are wrong xD

TBH since the new dumpExtent debug functions 89d210fe1a7a1c6cbf926df0595b6f107bc491d5 landed, I no longer need this, thus I'm abandoning this change.
Thank you all for the review though.

clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp

void ExprInspectionChecker::analyzerGetExtent(const CallExpr *CE,
 
   auto MR = dyn_cast_or_null<SubRegion>(C.getSVal(CE->getArg(0)).getAsRegion());
   if (!MR) {
     reportBug("Obtaining extent of a non-region", C);
     return;
   }
 
   ProgramStateRef State = C.getState();
-  DefinedOrUnknownSVal Size = getDynamicSize(State, MR, C.getSValBuilder());
-
+  SVal Size = getDynamicSizeWithOffset(State, C.getSVal(CE->getArg(0)));
   State = State->BindExpr(CE, C.getLocationContext(), Size);
   C.addTransition(State);
 }
 
 void ExprInspectionChecker::analyzerPrintState(const CallExpr *CE,
                                                CheckerContext &C) const {
   C.getState()->dump();
 }

clang/test/Analysis/explain-svals.cpp

 void test_2(char *ptr, int ext) {
   clang_analyzer_explain((void *) "asdf"); // expected-warning-re{{{{^pointer to element of type 'char' with index 0 of string literal "asdf"$}}}}
   clang_analyzer_explain(strlen(ptr)); // expected-warning-re{{{{^metadata of type 'unsigned long' tied to pointee of argument 'ptr'$}}}}
   clang_analyzer_explain(conjure()); // expected-warning-re{{{{^symbol of type 'int' conjured at statement 'conjure\(\)'$}}}}
   clang_analyzer_explain(glob); // expected-warning-re{{{{^value derived from \(symbol of type 'int' conjured at statement 'conjure\(\)'\) for global variable 'glob'$}}}}
   clang_analyzer_explain(glob_ptr); // expected-warning-re{{{{^value derived from \(symbol of type 'int' conjured at statement 'conjure\(\)'\) for global variable 'glob_ptr'$}}}}
   clang_analyzer_explain(clang_analyzer_getExtent(ptr)); // expected-warning-re{{{{^extent of pointee of argument 'ptr'$}}}}
   int *x = new 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'$}}}}
+  clang_analyzer_explain(clang_analyzer_getExtent(x)); // expected-warning-re{{{{^extent of heap segment that starts at symbol of type 'int \*' conjured at statement 'new int \[ext\]'$}}}}
+  clang_analyzer_explain(clang_analyzer_getExtent(x + 2)); // expected-warning-re{{{{^\(extent of heap segment that starts at symbol of type 'int \*' conjured at statement 'new int \[ext\]'\) - 8$}}}}
   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.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) {