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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
