NoQ added a comment.
Yup, this looks great and that's exactly how i imagined it.
================
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h:261-265
+inline bool isLocType(const QualType &T) {
+ return T->isAnyPointerType() || T->isReferenceType() ||
+ T->isBlockPointerType();
+}
+
----------------
We have a fancy static `Loc::isLocType()`.
================
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:126-127
if (V.isUndef()) {
+ assert(!FR->getDecl()->getType()->isReferenceType() &&
+ "References must be initialized!");
return addFieldToUninits(
----------------
Good catch.
It might still be possible to initialize a reference with an already-undefined
pointer if core checkers are turned off, but we don't support turning them off,
so i guess it's fine.
================
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:177
- if (isPrimitiveUninit(V)) {
+ const SVal &PointeeV = State->getSVal(loc::MemRegionVal(R));
+ if (isPrimitiveUninit(PointeeV)) {
----------------
Just `SVal`. And you should be able to pass `R` directly into `getSVal`.
================
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:210
assert(V.getAs<loc::MemRegionVal>() && "V must be loc::MemRegionVal!");
+ auto Tmp = V.getAs<loc::MemRegionVal>();
+
----------------
We usually just do `.getAsRegion()`.
And then later `dyn_cast` it.
================
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:212-216
+ // We can't reason about symbolic regions, assume its initialized.
+ // Note that this also avoids a potential infinite recursion, because
+ // constructors for list-like classes are checked without being called, and
+ // the Static Analyzer will construct a symbolic region for Node *next; or
+ // similar code snippets.
----------------
Ok, i have a new concern that i didn't think about before.
There's nothing special about symbolic regions. You can make a linked list
entirely of concrete regions:
```
struct List {
List *next;
List(): next(this) {}
};
void foo() {
List list;
}
```
In this case the finite-ness of the type system won't save us.
I guess we could also memoize base regions that we visit :/ This is guaranteed
to terminate because all of our concrete regions are based either on AST nodes
(eg. global variables) or on certain events that happened previously during
analysis (eg. local variables or temporaries, as they depend on the stack frame
which must have been previously entered). I don't immediately see a better
solution.
================
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:223
+ const auto *R = Tmp->getRegionAs<TypedValueRegion>();
+ assert(R);
+
----------------
We might have eliminated symbolic regions but we may still have concrete
function pointers (which are `TypedRegion`s that aren't `TypedValueRegion`s,
it's pretty weird), and i guess we might even encounter an `AllocaRegion`
(which is completely untyped). I guess we should not try to dereference them
either.
================
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:240-244
+ if (Tmp->getRegion()->getSymbolicBase())
return None;
- }
- V = State->getSVal(*Tmp, DynT);
+ DynT = DynT->getPointeeType();
+ R = Tmp->getRegionAs<TypedValueRegion>();
----------------
This code seems to be duplicated with the "0th iteration" before the loop. I
guess you can put everything into the loop.
Repository:
rC Clang
https://reviews.llvm.org/D51057
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
