Szelethus added a comment.
In https://reviews.llvm.org/D48436#1191458, @NoQ wrote:
> Ok, let's commit this and see how to fix it later.
Thanks! ^-^
> I still think it's more important to come up with clear rules of who is
> responsible for initializing fields than making sure our warnings are
> properly grouped together.
I'll admit that the main reason why I only added a TODO about this issue and
delayed it for later is that I'm a little unsure about some details myself. I
spent serious effort on some ideas, which I later realized didn't work out too
well, so I'm looking for a solid solution on some issues (like base class
handling) that is both efficient to a degree and makes sense from a user
standpoint before writing a doc file of some sort.
Currently, I'm working on refactoring the checker (which is why I didn't update
some of my patches, as those changes would become obsolete), which will greatly
increase readability and reliability (let's face it, the code about pointer
chasing is a spaghetti).
>> [...]ideally we wouldn't like to have a warning for an object (`t`) and a
>> separate warning for it's field (`t.bptr.x`)[...]
>
> I don't quite understand this. How would the first warning look like? What
> would it warn about?
I'm afraid I explained my thoughts very poorly. I have a much better grasp on
this issue now, and I have a very good solution in testing. I'll upload a new
diff during the week that will clarify this.
Here's what I meant with the correct code:
struct DynTBase {
// This is the line I meant but forgot to add.
int x; // expected-note{{uninitialized field 'this->bptr->x'}}
};
struct DynTDerived : DynTBase {
// TODO: we'd expect the note: {{uninitialized field 'this->bptr->y'}}
int y; // no-note
};
struct DynamicTypeTest {
DynTBase *bptr;
int i = 0;
DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // expected-warning{{1
uninitialized field}}
};
void f() {
DynTDerived d;
DynamicTypeTest t(&d);
};
In this one, note that the constructed object `t` has two uninitialized fields,
`t.bptr->x` and `t.bptr->y`. Currently, my checker misses the second one. The
first one gets picked up rather easily, but then the problem arises about how
do we deal with `t.bptr->y`. I proposed two ideas:
1. In `isNonUnionUninit`, obtain the dynamic type of the object we're
analyzing. This is the easier solution, as the current implementation
explicitly checks each base. This would result in one warning with a note for
each field.
2. Change `willObjectBeAnalyzedLater` so that bases are analyzed on their own,
and eliminate checking bases explicitly
(https://reviews.llvm.org/D45532#inline-415396). The checker won't run when `d`
is constructed, because it has a default ctor, but will run after `t`. How do
we catch `t.bptr->y?` If we don't analyze bases explicitly, we can't just
obtain the dynamic type in `isNonUnionUninit`, because then we'd miss
`t.bptr->x`. If we decide that for fields we will explicitly check bases, that
would be inconsistent, because in `t`s case we would analyze base classes on
their own (if it had any), but we wouldn't for its fields. As you said,
> With that i guess you'll still have to deep-scan fields and bases, which
> prevents us from simplifying our code.
I really just wanted to emphasize the point that I need more time to figure
this out.
================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:669-671
+ Optional<nonloc::LazyCompoundVal> CurrentObject = getObjectVal(Ctor,
Context);
+ if (!CurrentObject)
+ return false;
----------------
NoQ wrote:
> All uses of `getObjectVal` so far are followed by retrieving the parent
> region from the `LazyCompoundVal`. Why do you need to obtain the
> `LazyCompoundVal` in the first place, i.e. do the second `getSVal` "for
> '*this'" in `getObjectVal`? Why not just operate over the this-region on the
> current Store? I think there isn't even a guarantee that these two regions
> are the same. Like, in this case they probably will be the same, but we
> shouldn't rely on that.
Fair point, that part was written when I knew very little about how these
things worked. I'll add a TODO to get that fixed before commiting.
https://reviews.llvm.org/D48436
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
