vrnithinkumar marked 38 inline comments as done.
vrnithinkumar added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:31
namespace {
-class SmartPtrModeling : public Checker<eval::Call> {
+struct RegionState {
+private:
----------------
vsavchenko wrote:
> xazax.hun wrote:
> > I think `RegionState` is not very descriptive. I'd call it something like
> > `RegionNullness`.
> linter: LLVM coding standards require to use `class` keyword in situations
> like this
> (https://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords).
> I would even say that `struct` is good for POD types.
With the new changes, the struct is removed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:31
namespace {
-class SmartPtrModeling : public Checker<eval::Call> {
+struct RegionState {
+private:
----------------
vrnithinkumar wrote:
> vsavchenko wrote:
> > xazax.hun wrote:
> > > I think `RegionState` is not very descriptive. I'd call it something like
> > > `RegionNullness`.
> > linter: LLVM coding standards require to use `class` keyword in situations
> > like this
> > (https://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords).
> > I would even say that `struct` is good for POD types.
> With the new changes, the struct is removed.
With the new changes, the struct is removed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:33
+private:
+ enum Kind { Null, NonNull, Unknown } K;
+ RegionState(Kind InK) : K(InK) {}
----------------
vsavchenko wrote:
> I think that it would be better to put declarations for `enum` and for a
> field separately.
> Additionally, I don't think that `K` is a very good name for a data member.
> It should be evident from the name of the member what is it. Shot names like
> that can be fine only for iterators or for certain `clang`-specific
> structures because of existing traditions (like `SM` for `SourceManager` and
> `LO` for `LanguageOptions`).
With the new changes, the `enum` is removed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:46
+};
+} // end of anonymous namespace
+
----------------
Szelethus wrote:
> xazax.hun wrote:
> > You can merge the two anonymous namespaces, this and the one below.
> [[https://llvm.org/docs/CodingStandards.html#anonymous-namespaces | I prefer
> them like this. ]]
with new change only one anonymous namespace is there
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:60
+private:
+ mutable std::unique_ptr<BugType> BT;
+ void reportBug(CheckerContext &C, const CallEvent &Call) const;
----------------
xazax.hun wrote:
> This is how we used to do it, but we did not update all the checks yet.
> Nowadays we can just initialize bugtypes immediately, see
> https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp#L169
updated to initialize bugtype immediately
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:65
+
+ // STL smart pointers which we are trying to model
+ const llvm::StringSet<> StdSmartPtrs = {
----------------
xazax.hun wrote:
> In LLVM we aim for full sentences as comments with a period at the end.
Updated the comments as LLVM style
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:72-76
+ // STL smart pointer methods which resets to null
+ const llvm::StringSet<> ResetMethods = {"reset", "release", "swap"};
+
+ // STL smart pointer methods which resets to null via null argument
+ const llvm::StringSet<> NullResetMethods = {"reset", "swap"};
----------------
NoQ wrote:
> Please consider `CallDescription` and `CallDescriptionMap`!
Made changes to use `CallDescription` and `CallDescriptionMap`
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:81
+// to track the mem region and curresponding states
+REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *,
RegionState)
+// to track the Symbols which will get inner raw pointer via unique_ptr.get()
----------------
NoQ wrote:
> I ultimately believe this map should go away. The only thing we really need
> is the map from smart pointer region to the symbol for its current raw
> pointer. As long as we have such data we can discover the nullness of that
> symbol (which *is* the nullness of the smart pointer as well) from Range
> Constraints.
Removed this map.
Now maping region to SVal
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:119
+ ProgramStateRef State = C.getState();
+ const auto OC = dyn_cast<CXXMemberOperatorCall>(&Call);
+ if (!OC)
----------------
vsavchenko wrote:
> xazax.hun wrote:
> > Here the const applies for the pointer, not the pointee. We usually do
> > `const auto *OC` instead.
> As I said above, I think we should be really careful about abbreviated and
> extremely short variable names. Longer names make it much easier to read the
> code without paying a lot of attention to declarations.
fixed
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:119
+ ProgramStateRef State = C.getState();
+ const auto OC = dyn_cast<CXXMemberOperatorCall>(&Call);
+ if (!OC)
----------------
vrnithinkumar wrote:
> vsavchenko wrote:
> > xazax.hun wrote:
> > > Here the const applies for the pointer, not the pointee. We usually do
> > > `const auto *OC` instead.
> > As I said above, I think we should be really careful about abbreviated and
> > extremely short variable names. Longer names make it much easier to read
> > the code without paying a lot of attention to declarations.
> fixed
Okay.
Will try to use longer names in future.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:150
+ const CXXRecordDecl *RD = CtorDec->getParent();
+ if (isSmartPointer(RD)) {
+ State =
----------------
xazax.hun wrote:
> I wonder if you wanted to add `isSmartPointer` checks below as well. In that
> case you can hoist this check and early return for non-smart-pointers.
Added the check for early return
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:160-187
+ if (const auto IC = dyn_cast<CXXInstanceCall>(&Call)) {
+ const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl());
+ if (!MethodDecl)
+ return;
+ auto ArgsNum = IC->getNumArgs();
+
+ if (ArgsNum == 0 && isResetMethod(MethodDecl)) {
----------------
vsavchenko wrote:
> Considering the fact that more cases and more functions will get supported in
> the future, I vote for merging common parts of these two blocks.
Merged
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:195
+ // Clean up dead regions from the region map.
+ TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>();
+ for (auto E : TrackedRegions) {
----------------
xazax.hun wrote:
> You probably also want to clean up the `SymbolRegionMap`. It is ok to not do
> it right now, but we usually tend to add `FIXMEs` or `TODOs` early and
> aggressively to make sure we do not forget stuff.
This map is removed in the new changes
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:213-214
+
+ if (!BT)
+ BT.reset(new BugType(this, "unique_ptr", "Dont call unique_ptr"));
+ auto R = std::make_unique<PathSensitiveBugReport>(
----------------
NoQ wrote:
> These days we don't bother with lazy initialization, the usual syntax is:
> ```lang=c++
> class YourChecker {
> // ...
> BugType BT { this, "unique_ptr", "Dont call unique_ptr" };
> // ...
> };
> ```
updated to initialize bugtype immediately
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:225
+ if (RecordDec->getDeclName().isIdentifier()) {
+ return StdSmartPtrs.count(RecordDec->getName().lower());
+ }
----------------
xazax.hun wrote:
> This looks good for now. But we sometimes cache the pointer to identifier
> info objects so after the initial lookup we can just do pointer comparison
> instead of more expensive operations. Also add a fixme to check for the `std`
> namespace.
>
> Also, this method could even be promoted to a free functions making the list
> of SmarPtrs global.
Added std namespace check
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:233
+ return false;
+ if (MethodDec->getDeclName().isIdentifier()) {
+ return ResetMethods.count(MethodDec->getName().lower());
----------------
NoQ wrote:
> vsavchenko wrote:
> > I'm not sure about it myself, but can `DeclName` be `isEmpty()`? If yes,
> > it is a potential null-pointer dereference. Again, I don't know it for a
> > fact, but I think it should be checked.
> NOTE: π Call π Description π Map π
Changed to use `CallDescription`
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:233
+ return false;
+ if (MethodDec->getDeclName().isIdentifier()) {
+ return ResetMethods.count(MethodDec->getName().lower());
----------------
vrnithinkumar wrote:
> NoQ wrote:
> > vsavchenko wrote:
> > > I'm not sure about it myself, but can `DeclName` be `isEmpty()`? If yes,
> > > it is a potential null-pointer dereference. Again, I don't know it for a
> > > fact, but I think it should be checked.
> > NOTE: π Call π Description π Map π
> Changed to use `CallDescription`
Changed to use `CallDescription`
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:233
+ return false;
+ if (MethodDec->getDeclName().isIdentifier()) {
+ return ResetMethods.count(MethodDec->getName().lower());
----------------
vrnithinkumar wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > vsavchenko wrote:
> > > > I'm not sure about it myself, but can `DeclName` be `isEmpty()`? If
> > > > yes, it is a potential null-pointer dereference. Again, I don't know
> > > > it for a fact, but I think it should be checked.
> > > NOTE: π Call π Description π Map π
> > Changed to use `CallDescription`
> Changed to use `CallDescription`
Changed to use CallDescription
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81315/new/
https://reviews.llvm.org/D81315
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
