[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker

Gábor Horváth via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Jul 8 10:21:46 PDT 2018

xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.

Overall looks good, some nits inline. Let's run it on some projects to exercise this change.

Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:27
-// FIXME: member functions that return a pointer to the container's internal
-// buffer may be called on the object many times, so the object's memory
-// region should have a list of pointer symbols associated with it.
-REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+typedef llvm::ImmutableSet<SymbolRef> PtrSet;
I think we should use `using` now instead of `typedef`.

Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:126
+      NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+      if (!NewSet.isEmpty()) {
+        State = State->set<RawPtrMap>(ObjRegion, NewSet);
Is it possible here the set to be empty? We just added a new element to it above. Maybe turn this into an assert or just omit this if it is impossible?

Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:138
       const Expr *Origin = Call.getOriginExpr();
-      State = allocation_state::markReleased(State, *StrBufferPtr, Origin);
-      State = State->remove<RawPtrMap>(TypedR);
+      const PtrSet *PS = State->get<RawPtrMap>(ObjRegion);
+      for (const auto &Symbol : *PS)
Using both contains and get will result in two lookups. Maybe it would be better to just use get and check if the result is nullptr?

Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:159
+    if (State->contains<RawPtrMap>(Entry.first)) {
+      const PtrSet *OldSet = State->get<RawPtrMap>(Entry.first);
Same comment as above.

Comment at: test/Analysis/dangling-internal-buffer.cpp:111
+void multiple_symbols(bool Cond) {
+  const char *c1, *d1;
We started to use lowercase letters for variable names. Maybe this is not the best, since it is not following the LLVM coding guidelines. So I think it would be better to rename this `Cond` to start with a lowercase letter in this patch for consistency, and update the tests to follow the LLVM coding styleguide in a separate patch later.

  rC Clang


More information about the cfe-commits mailing list