[PATCH] D14203: [analyzer] Improve pointer arithmetic checker.

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.

================
Comment at: lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp:28
@@ -24,1 +27,3 @@
 namespace {
+enum class AllocKind {
+  SingletonNew,
----------------
dcoughlin wrote:
> Is it necessary to distinguish so many cases here? For example, why do we need to distinguish between PlacementNew and OverloadedNew?
> 
> Another thought: given the expense of tracking stuff in the GDM could we instead track whether pointer arithmetic is explicitly disallowed for a given region? Then we wouldn't have to track any data for the "good" pointers.
> 
> Also, how does AllocKind relate to the AllocationFamily from MallocChecker? Could that checker's state be used so we don't have to track any additional information here?
> 
> If you do keep AllocKind, I think it would be good to add a comment describing how this enum is used and the intended meaning of each element.
I think this way it is more explicit that we are not going to reason about PlacementNew and OverLoadedNew. Turning this information into comments and reduce the number of elements of the enum is fine. I am going to do that. 

Storing information only for singletons or only for arrays seems like a good idea for me. However it is possible that there are more pointers for single objects than arrays? In this case it would be better to store data only for "good" pointers.

AllocationFamily does not distinguish placement and overloaded new for instance. The current approach to this checker is to be conservative and do not try to reason about them. Other than that in the future I planned to add heuristics when a malloc call is likely to allocate and array and when it is likely to allocate a single object (heuristics on the AST). The AllocationFamily do not distinguish these cases.   
 

================
Comment at: lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp:29
@@ +28,3 @@
+enum class AllocKind {
+  SingletonNew,
+  ArrayNew,
----------------
dcoughlin wrote:
> I'm not sure "singleton" is the right term here. I associate "singleton" with the design pattern of only having a single instance of a class allocated at a given time (a form of global shared state).  Also, SingletonMalloc doesn't appear to be used anywhere.
What about SingleObjectNew? Currently this checker does not reason about malloced pointers, because it does not distinguish the case where the malloc is used to allocate an array and where it is used to allocate a single object. I was planning to do this classification based on some heuristics on AST matching. 

================
Comment at: lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp:88
@@ -28,1 +87,3 @@
+  mutable std::unique_ptr<BuiltinBug> BT_polyArray;
+  mutable llvm::SmallVector<IdentifierInfo *, 8> AllocFunctions;
 
----------------
dcoughlin wrote:
> I think it would be better to use llvm::SmallSet here.
I will change that, thanks.

================
Comment at: lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp:137
@@ -43,2 +136,3 @@
 
-  const MemRegion *LR = LV.getAsRegion();
+const MemRegion *PointerArithChecker::getArrayRegion(const MemRegion *Region,
+                                                     bool &Polymorphic,
----------------
dcoughlin wrote:
> I think it would be good to add a doc comment for this function describing what the function does and its parameters as well as whether they are input or output parameters.
> 
> I also wonder if this logic is better expressed as a loop rather than recursion. What do you think?
I will rewrite it without recursion and check the results but I suspect that you are right.

================
Comment at: lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp:150
@@ +149,3 @@
+    return getArrayRegion(Region, Polymorphic, AKind, C);
+  default:
+    break;
----------------
dcoughlin wrote:
> In general, I think it is better to avoid default in cases like these so that when an enum case is added the compiler issues a warning and thus forces the person adding the change to think about what the behavior of the new case should be.
I will enumerate the rest of the kinds here.

================
Comment at: lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp:198
@@ +197,3 @@
+            this, "Dangerous pointer arithmetic",
+            "Pointer arithmetic does not account for polymorphic object sizes "
+            "and attempting to perform pointer arithmetic on a polymorphic "
----------------
dcoughlin wrote:
> I think "polymorphic" is a bit jargony.  Can this diagnostic be explained in terms of base and derived classes?
Sure, I will try to come up with something.


http://reviews.llvm.org/D14203