[PATCH] D71612: [analyzer] Add PlacementNewChecker

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

xazax.hun accepted this revision.
xazax.hun added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91
+  SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State);
+  const auto SizeOfTargetCI = SizeOfTarget.getAs<nonloc::ConcreteInt>();
+  if (!SizeOfTargetCI)
----------------
NoQ wrote:
> xazax.hun wrote:
> > martong wrote:
> > > xazax.hun wrote:
> > > > Here, instead of getting `SizeOfTarget` and `SizeOfPlace` as `ConcreteInt`s, I think you should rather use `evalBinOp` to compare them. That method is more future proof as if we cannot constraint these values down to a single integer but we still have some information about them a sufficiently smart solver could prove the relationship between the symbolic values.
> > > I am not sure if `evalBinOp` is that useful here, because we need the concrete values anyway when we issue the diagnostics. We'd like to present the concrete sizes in bytes.
> > The reason why evalbinop might be useful because we might have symbolic sizes:
> > ```
> > void f(int a) {
> >  char *buffer = new char[a];
> > }
> > ```
> > 
> > So in the code snippet above you cannot get a concrete integer for the size of the buffer. But in case we already have some constraints about the value of `a`, the constraint solver might be able to tell if we are sure that the type will not fit into the buffer. I can imagine that this scenario is relatively rare, but I think we need relatively little code to support this. 
> > 
> > So you could potentially warn when:
> > ```
> > void f(int a) {
> >   char *buffer = new char[a];
> >   if (a > 3)
> >     return;
> >   int *p = new (buffer) int;
> > }
> > ```
> > 
> > I know, this is silly code, but we might not know if there are reasonable code that has similar patterns.
> For this sort of stuff i'd strongly recommend
> 1. explaining the range constraints for the buffer size in the warning message and
> 2. making a bug visitor that'll explain how these constraints change across the path.
> 
> I.e., "Assuming that 'a' is less than or equal to 3" => "Buffer size is constrained to [0, 3]" => "Storage provided to placement new is //at most// 3 bytes, whereas the allocated type requires 4 bytes".
> 
> The same applies to our alpha array bound checkers. We really need this stuff explained in them. Without such facilities i'd rather stick to concrete values.
This makes sense. How about committing this only supporting concrete values and introduce the visitor/symbolic support in a follow-up? (If Gabor Marton is motivated to implement it :) I am also ok if this will not get implemented for now.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71612/new/

https://reviews.llvm.org/D71612