NoQ added inline comments.
================
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:
> 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.
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
