[PATCH] D71612: [analyzer] Add PlacementNewChecker

Thu Jan 9 10:05:07 PST 2020

NoQ added a comment.

In D71612#1812036 <https://reviews.llvm.org/D71612#1812036>, @martong wrote:

> In D71612#1790045 <https://reviews.llvm.org/D71612#1790045>, @NoQ wrote:
>
> > I wonder if this checker will find any misuses of placement into `llvm::TrailingObjects`.
>
>
> I've evaluated this checker on LLVM/Clang source and it did not find any results, though there are many placement new calls there. I think this is good, because there are no false positives.

In such cases it's usually a good idea to verify that the checker works correctly by artificially injecting a bug into the codebase. If the bug is not found, the checker is probably not working. If the bug is found, change it to be more and more realistic, so that to see what limitations does the checker have in terms of false negatives. Monitor analyzer stats closely (max-nodes limits, block count limits, inlining limits) in order to see what exactly goes wrong (or debug on the Exploded Graph as usual, depending on how it goes wrong). This exercise often uncovers interesting omissions :)

Can we enable the check by default then? What pieces are missing? Like, the symbolic extent/offset case is not a must. I think we have everything except maybe some deeper testing. I'd rather spend some time on the above exercise, but then enable by default if it goes well.

> The two failures are caused by non existing `.inc` files, e.g.:
> 
>   /mnt/ssd/egbomrt/WORK/llvm1/git/llvm-project/llvm/unittests/Option/OptionParsingTest.cpp:23:10: fatal error: 'Opts.inc' file not found
>   #include "Opts.inc"
>            ^~~~~~~~~~
>   1 error generated.

I've seen these errors with scan-build-py as well. It looks like LLVM's build system cannot be correctly reduced to a compilation database (though it's just one place - maybe it can be easily fixed).

================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91
+  SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State);
+  const auto SizeOfTargetCI = SizeOfTarget.getAs<nonloc::ConcreteInt>();
+  if (!SizeOfTargetCI)
----------------
xazax.hun wrote:
> 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.)
Technically, there's also a chance of the object size being symbolic (i.e., "placement `new[]`"), however placement `new[]` is already very weird due to the requirement to store the "allocated" size within the storage together with the actual buffer (IIRC the size of such header is implementation-defined and the operator almost always returns an updated pointer which should be later passed to `delete[]`).

> How about committing this only supporting concrete values and introduce the visitor/symbolic support in a follow-up?

Sure!

================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:60-61
+    SVal ElementCount = C.getSVal(SizeExpr);
+    Optional<NonLoc> ElementCountNL = ElementCount.getAs<NonLoc>();
+    if (ElementCountNL) {
+      // size in Bytes = ElementCountNL * TypeSize
----------------
Let's combine these two lines into a single if-statement:
```lang=c++
if (auto ElementCountNL = ElementCount.getAs<NonLoc>()) {
  ...
}
```
(also, yeah, this is one of the blessed use cases for `auto`)

================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:75
+                  TypeSize.getQuantity());
+    return SvalBuilder.makeIntVal(I, false);
+  }
----------------
Soo why not use `makeArrayIndex` here as well?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612