NoQ added a comment.
Hi, looks very interesting! We definitely have troubles with some initializer
lists.
It looks like you're primarily interested in the alpha/unsupported checker, but
your patch probably affects behavior of the default setup as well. If you've
found any difference in results for the default setup, please include a test
case! These are much more valuable. You can often use tools like
`creduce`/`cvise` to reduce entire real-world translation units to tiny test
cases, under the test predicate that "two compiler versions produce different
output".
================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:564-565
- SVal getBindingForElement(RegionBindingsConstRef B, const ElementRegion *R);
+ SVal getBindingForElement(RegionBindingsConstRef B, const ElementRegion *R,
+ QualType T = QualType());
----------------
There's a bit of a confusing tradition here, mostly documented in my unsorted
long rants in earlier patches, let me try to explain.
`RegionStore` was built to recognize that type punning is a thing. You can
write a value while treating the target location/region R as if it's supposed
to hold a value of type T₁ and then load the value by treating the same memory
location as if it holds a value of type T₂≠T₁. You can achieve it by having
unions or by reinterpret-casting pointers. RegionStore needs to handle this
reasonably well, no matter what T₁ and T₂ are. Even though strict aliasing
rules put some restrictions on what T₁ and T₂ could be, the analyzer is still
required to work correctly when the programmer actively relies on
`-fno-strict-aliasing`.
On top of that, every `MemRegion` R can have a type T(R) associated with it
(accessed as `R->getValueType()`). This was probably a bad idea from the start,
for at least three reasons:
- Again, type punning exists. Just because the region has a type, doesn't mean
it holds a value of that type.
- In many cases we deal with regions with no associated type. For example, an
unknown (symbolic) pointer of type `void *` definitely points to a certain
memory region (we call it `SymbolicRegion`), but the type of the object in that
region is most certainly not `void`.
- Finally, polymorphism: C++ pointers to base class type may actually point to
objects of any derived class type. Not only the specific derived type is often
unknown, but also the true derived type may not be necessarily available in the
current translation unit, so we often don't have the AST to even describe it.
This makes the type of `MemRegion` largely immaterial and hard to rely upon.
Which is why `State->getSVal(R)` accepts an optional parameter T₂ which may be
different from both T₁ and T(R).
However, after some back-and-forth bugfixing, we've settled on agreeing that
when T(R) exists, it doesn't make sense to pass any T₂ into `State->getSVal(R,
T₂)` other than `T₂=T(R)`. Basically, if R already carries a type, then you
don't have a good reason to pass a different type as T₂, given that you can
always pass a different region R' instead, that represents the same memory
location as R but has the desired value type T₂. So T₂ is only required when
it's fundamentally impossible to discover the type from the region. So in order
to eliminate the confusion, `getSVal()` tries to get rid of the extra parameter
`T₂` as early as possible, and have a single source of truth going further down
the call stack.
I honestly no longer think this is a healthy strategy long-term. Ideally I
think we should get rid of region types entirely; they cause more problems than
they solve. But that's a much bigger change, and I'm worried that a partial
step in this direction, not supported by any strategic vision, will only
multiply confusion.
So, long story short, given that `ElementRegion`s are always typed, I'm
wondering if it's easier to simply recast the region in your case as well,
instead of reintroducing such duplication of types.
================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1920
+ if (ElemExpr) {
+ return svalBuilder.makeCompoundVal(VarT, InitList);
+ }
----------------
The lifetime of `InitList` is the same as lifetime of `factory`. Because
`factory` is a local variable, this is going to be a use-after-free. You need
to use the factory that lives inside `svalBuilder` instead, accessible through
methods `getEmptySValList ()` and `prependSVal()` of
`svalBuilder.getBasicValueFactory()`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144977/new/
https://reviews.llvm.org/D144977
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
