[PATCH] [Review request][analyzer]Information about special handling for a particular region/symbol.

[Anna Zaks]

  Thanks Anton!

  I have no more comments. Jordan does not have time to review, so you are all set.

  Looking forward to the follow up patches!

  Anna.


================
Comment at: include/clang/StaticAnalyzer/Core/CheckerManager.h:368-370
@@ -367,3 +367,5 @@
   ///        parameters to the given call.
-  /// \param IsConst Specifies if the pointer is const.
+  /// \param Kind The reason of pointer escape.
+  /// \param HTraits Information about special handling for a particular 
+  ///        region/symbol.
   /// \returns Checkers can modify the state by returning a new one.
----------------
Антон Ярцев wrote:
> Anna Zaks wrote:
> > I know this is not new to this patch, but PointerEscapeKind and the traits info are very related...
> > 
> > Would it make sense to extend the kind enum and use it for traits? One benefit I see is that all the info about the escape would be in one place. We would only have one checker callback.. On the other hand, I don't recall why they are different concepts now..
> I don't particulary like the idea to keep traits into the PointerEscapeKind enum.
> They are related, but at the same time they differ in their meaning and nature.
> Escape kinds describe reasons of a pointer escapes, kinds are mutually exclusive.
> Traits currently describe invalidation traits (preserve contents, invalidate 
> indirect regions, etc.). Any number of traits may be attached to a region at the
> same time.
> Escape kinds and traits are independent - each escape kind may come with any set 
> of traits.
> 
> Foud the reason for the separate ConstEscape callback in out mail correspondence:
> Re: [PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators
> 29.03.2013 4:00
> >>>>>>>> Anton, I think handling this similarly to pointer escape is the best.
> >>>>>>>> However, I am concerned that if we simply extend pointer escape to 
> >>>>>>>> trigger on another "Kind" of escape, all the other users of 
> >>>>>>>> pointerEscape will need to know about it (and do nothing for this kind 
> >>>>>>>> of escape). How about a new checker callback, which will rely on the 
> >>>>>>>> same core code as _checkPointerEscape? This way the checker developers
> >>>>>>>> would not need to know about this special case, and we can reuse all
> >>>>>>>> the code that determines when the escape should be triggered.
> >>>>>> Anna, As for me it would be better to leave the single callback as this 
> >>>>>> is just another type of pointer escape, if I understand correctly. 
> >>>>>> Have no other arguments for this :)
> >>>>>> In addition, the "Kind" approach is relatively new, so hopefully a few 
> >>>>>> users of pointerEscape be affected. 
> >>>> Anton, The main concern is that every user of the callback will now have 
> >>>> to check if the kind is ConstPointer (or whatever we call it, maybe 
> >>>> multiple kinds..) and do nothing in that case. So every user will need to 
> >>>> know about this special case and handle it specially.
> >> Anton, I've committed the new callback. Looks like no email was sent out.. 
> >> So just look at r178310.
> 
OK.

================
Comment at: include/clang/StaticAnalyzer/Core/CheckerManager.h:377
@@ -374,3 +376,3 @@
                               PointerEscapeKind Kind,
-                              bool IsConst = false);
+                              RegionAndSymHandlingTraits *HTraits = NULL);
 
----------------
Антон Ярцев wrote:
> Anna Zaks wrote:
> > When does it make sense for traits to be missing? (Why the NULL initialization? Also, I think it should be '0', not "NULL")
> Currently pointer escape on bind do not deal with traits ( ExprEngine.cpp, processPointerEscapedOnBind() ), NULL initialization just allows to leave the code of processPointerEscapedOnBind() unchanged. This is the only reason for NULL initialization. Do you think NULL initialization should be removed?
> 
> Changed "NULL" to "0". Searched LLVM code standard for "0" vs "NULL", found nothing. What is wrong with "NULL"? :)
> 
Is that expected to change? Maybe we should just pass NULL at that one call site, instead of using default initialization.

There are probably threads discussing which one to use "NULL" or "0" and I assume we came to a decision to use '0'. I can see arguments on both sides, one is more expressive, the other one is shorter and cannot be redefined...


http://llvm-reviews.chandlerc.com/D1486