[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:25
 
-/// Get the stored dynamic size for the region \p MR.
+/// \returns The stored dynamic size for the region \p MR.
 DefinedOrUnknownSVal getDynamicSize(ProgramStateRef State, const MemRegion *MR,
----------------
Naming: I believe we should keep using the word "Extent". There's no need to introduce a new term for the existing concept; it makes it harder to explain on the mailing list :) Let's make a follow-up patch to change the naming back (so that not to screw the review).


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:41-44
+/// Set the dynamic size of a CXXNewExpr \p NE by its region \p MR.
+ProgramStateRef setDynamicSize(ProgramStateRef State, const MemRegion *MR,
+                               const CXXNewExpr *NE,
+                               const LocationContext *LCtx, SValBuilder &SVB);
----------------
This function is probably going to be used exactly once in the whole code. There's no need to turn it into a public API.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:99
+          .Case("clang_analyzer_region", &ExprInspectionChecker::analyzerRegion)
+          .Case("clang_analyzer_size", &ExprInspectionChecker::analyzerDumpSize)
+          .Case("clang_analyzer_elementCount",
----------------
`clang_analyzer_dump_extent()`? Or just `clang_analyzer_dump(clang_analyzer_getExtent())`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:100
+          .Case("clang_analyzer_size", &ExprInspectionChecker::analyzerDumpSize)
+          .Case("clang_analyzer_elementCount",
+                &ExprInspectionChecker::analyzerDumpElementCount)
----------------
`clang_analyzer_dumpElementCount()`. We need subjects in our sentences because they tend to vary quite a bit here and therefore cannot be implied (dump, return, express, ...).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:183-185
+  const Expr *Arg = nullptr;
+  if (!(Arg = getArgExpr(CE, C)))
+    return nullptr;
----------------
```lang=c++
  const Expr *Arg = getArgExpr(CE, C);
  if (!Arg)
    return nullptr;
```

No need for gymnastics >.>


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:283
+  llvm::raw_svector_ostream Out(Msg);
+  Out << MR;
+  reportBug(Out.str(), C);
----------------
So, how is it different from the existing `clang_analyzer_dump()`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:289-291
+  const MemRegion *MR = nullptr;
+  if (!(MR = getArgRegion(CE, C)))
+    return;
----------------
```lang=c++
  const MemRegion *MR = getArgRegion(CE, C);
  if (!MR)
    return;
```
... and so on.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:310-311
+
+  QualType Ty = TVR ? TVR->getDesugaredValueType(C.getASTContext())
+                    : CE->getArg(0)->IgnoreParenImpCasts()->getType();
+
----------------
How is this better than `getValueType()`? Are you sure you're not getting a pointer type instead in the `!TVR` case? I.e., can you test this on a non-heap `SymRegion`?


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:85
+    if (CI->getValue().isUnsigned())
+      Size = SVB.makeIntVal(CI->getValue(), /*IsUnsigned=*/false);
+
----------------
Charusso wrote:
> That one is interesting. Some of the checkers / SValBuilder(?) generate unsigned integers which should not happen, I believe. May we need a FIXME and an assertion about signedness. What do you think?
`SymbolExtent` has type `size_t` and both `malloc` and `operator new` accept `size_t` as a parameter. Therefore everything needs to be //unsigned// and we need to assert this.

That said, array indexes are //signed// (as per implementation of `ElementRegion`).


================
Comment at: clang/test/Analysis/misc-ps-region-store.m:1190
+        tmp2[x] = am; // expected-warning \
+        {{Access out-of-bound array element (buffer overflow)}}
       }
----------------
Charusso wrote:
> That is the single regression which I do not get.
Well, please debug. Like, look at the full report, dump egraph, see what changed. Try to `creduce` the example further under the condition "behavior changed with the patch", maybe that'll clear something up (though if it's a true positive after creduce it doesn't guarantee that it's a true positive before creduce).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726