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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits