[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
This revision was automatically updated to reflect the committed changes.
Closed by commit rC339237: [analyzer][UninitializedObjectChecker] Fixed a false
negative by no longer… (authored by Szelethus, committed by ).
Repository:
rC Clang
https://reviews.llvm.org/D48436
Files:
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
test/Analysis/cxx-uninitialized-object.cpp
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -225,12 +225,16 @@
/// Returns the object that was constructed by CtorDecl, or None if that isn't
/// possible.
+// TODO: Refactor this function so that it returns the constructed object's
+// region.
static Optional
getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext );
-/// Checks whether the constructor under checking is called by another
-/// constructor.
-static bool isCalledByConstructor(const CheckerContext );
+/// Checks whether the object constructed by \p Ctor will be analyzed later
+/// (e.g. if the object is a field of another object, in which case we'd check
+/// it multiple times).
+static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
+ CheckerContext );
/// Returns whether FD can be (transitively) dereferenced to a void pointer type
/// (void*, void**, ...). The type of the region behind a void pointer isn't
@@ -273,7 +277,7 @@
return;
// This avoids essentially the same error being reported multiple times.
- if (isCalledByConstructor(Context))
+ if (willObjectBeAnalyzedLater(CtorDecl, Context))
return;
Optional Object = getObjectVal(CtorDecl, Context);
@@ -433,8 +437,8 @@
}
// Checking bases.
- // FIXME: As of now, because of `isCalledByConstructor`, objects whose type
- // is a descendant of another type will emit warnings for uninitalized
+ // FIXME: As of now, because of `willObjectBeAnalyzedLater`, objects whose
+ // type is a descendant of another type will emit warnings for uninitalized
// inherited members.
// This is not the only way to analyze bases of an object -- if we didn't
// filter them out, and didn't analyze the bases, this checker would run for
@@ -661,18 +665,32 @@
return Object.getAs();
}
-// TODO: We should also check that if the constructor was called by another
-// constructor, whether those two are in any relation to one another. In it's
-// current state, this introduces some false negatives.
-static bool isCalledByConstructor(const CheckerContext ) {
- const LocationContext *LC = Context.getLocationContext()->getParent();
+static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
+ CheckerContext ) {
- while (LC) {
-if (isa(LC->getDecl()))
- return true;
+ Optional CurrentObject = getObjectVal(Ctor, Context);
+ if (!CurrentObject)
+return false;
+
+ const LocationContext *LC = Context.getLocationContext();
+ while ((LC = LC->getParent())) {
+
+// If \p Ctor was called by another constructor.
+const auto *OtherCtor = dyn_cast(LC->getDecl());
+if (!OtherCtor)
+ continue;
-LC = LC->getParent();
+Optional OtherObject =
+getObjectVal(OtherCtor, Context);
+if (!OtherObject)
+ continue;
+
+// If the CurrentObject is a subregion of OtherObject, it will be analyzed
+// during the analysis of OtherObject.
+if (CurrentObject->getRegion()->isSubRegionOf(OtherObject->getRegion()))
+ return true;
}
+
return false;
}
Index: test/Analysis/cxx-uninitialized-object.cpp
===
--- test/Analysis/cxx-uninitialized-object.cpp
+++ test/Analysis/cxx-uninitialized-object.cpp
@@ -1040,13 +1040,12 @@
// While a singleton would make more sense as a static variable, that would zero
// initialize all of its fields, hence the not too practical implementation.
struct Singleton {
- // TODO: we'd expect the note: {{uninitialized field 'this->i'}}
- int i; // no-note
+ int i; // expected-note{{uninitialized field 'this->i'}}
+ int dontGetFilteredByNonPedanticMode = 0;
Singleton() {
assert(!isInstantiated);
-// TODO: we'd expect the warning: {{1 uninitialized field}}
-isInstantiated = true; // no-warning
+isInstantiated = true; // expected-warning{{1 uninitialized field}}
}
~Singleton() {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
Szelethus updated this revision to Diff 159687.
Szelethus added a comment.
Added a TODO.
https://reviews.llvm.org/D48436
Files:
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
test/Analysis/cxx-uninitialized-object.cpp
Index: test/Analysis/cxx-uninitialized-object.cpp
===
--- test/Analysis/cxx-uninitialized-object.cpp
+++ test/Analysis/cxx-uninitialized-object.cpp
@@ -1040,13 +1040,12 @@
// While a singleton would make more sense as a static variable, that would zero
// initialize all of its fields, hence the not too practical implementation.
struct Singleton {
- // TODO: we'd expect the note: {{uninitialized field 'this->i'}}
- int i; // no-note
+ int i; // expected-note{{uninitialized field 'this->i'}}
+ int dontGetFilteredByNonPedanticMode = 0;
Singleton() {
assert(!isInstantiated);
-// TODO: we'd expect the warning: {{1 uninitialized field}}
-isInstantiated = true; // no-warning
+isInstantiated = true; // expected-warning{{1 uninitialized field}}
}
~Singleton() {
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -225,12 +225,16 @@
/// Returns the object that was constructed by CtorDecl, or None if that isn't
/// possible.
+// TODO: Refactor this function so that it returns the constructed object's
+// region.
static Optional
getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext );
-/// Checks whether the constructor under checking is called by another
-/// constructor.
-static bool isCalledByConstructor(const CheckerContext );
+/// Checks whether the object constructed by \p Ctor will be analyzed later
+/// (e.g. if the object is a field of another object, in which case we'd check
+/// it multiple times).
+static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
+ CheckerContext );
/// Returns whether FD can be (transitively) dereferenced to a void pointer type
/// (void*, void**, ...). The type of the region behind a void pointer isn't
@@ -273,7 +277,7 @@
return;
// This avoids essentially the same error being reported multiple times.
- if (isCalledByConstructor(Context))
+ if (willObjectBeAnalyzedLater(CtorDecl, Context))
return;
Optional Object = getObjectVal(CtorDecl, Context);
@@ -433,8 +437,8 @@
}
// Checking bases.
- // FIXME: As of now, because of `isCalledByConstructor`, objects whose type
- // is a descendant of another type will emit warnings for uninitalized
+ // FIXME: As of now, because of `willObjectBeAnalyzedLater`, objects whose
+ // type is a descendant of another type will emit warnings for uninitalized
// inherited members.
// This is not the only way to analyze bases of an object -- if we didn't
// filter them out, and didn't analyze the bases, this checker would run for
@@ -661,18 +665,32 @@
return Object.getAs();
}
-// TODO: We should also check that if the constructor was called by another
-// constructor, whether those two are in any relation to one another. In it's
-// current state, this introduces some false negatives.
-static bool isCalledByConstructor(const CheckerContext ) {
- const LocationContext *LC = Context.getLocationContext()->getParent();
+static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
+ CheckerContext ) {
- while (LC) {
-if (isa(LC->getDecl()))
- return true;
+ Optional CurrentObject = getObjectVal(Ctor, Context);
+ if (!CurrentObject)
+return false;
+
+ const LocationContext *LC = Context.getLocationContext();
+ while ((LC = LC->getParent())) {
+
+// If \p Ctor was called by another constructor.
+const auto *OtherCtor = dyn_cast(LC->getDecl());
+if (!OtherCtor)
+ continue;
-LC = LC->getParent();
+Optional OtherObject =
+getObjectVal(OtherCtor, Context);
+if (!OtherObject)
+ continue;
+
+// If the CurrentObject is a subregion of OtherObject, it will be analyzed
+// during the analysis of OtherObject.
+if (CurrentObject->getRegion()->isSubRegionOf(OtherObject->getRegion()))
+ return true;
}
+
return false;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
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();
};
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 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
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Ok, let's commit this and see how to fix it later. 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. > 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? Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:669-671 + Optional CurrentObject = getObjectVal(Ctor, Context); + if (!CurrentObject) +return false; 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. https://reviews.llvm.org/D48436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
Szelethus added a comment. Polite ping :) https://reviews.llvm.org/D48436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
Szelethus added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:681-689
+Optional OtherObject =
+getObjectVal(OtherCtor, Context);
+if (!OtherObject)
+ continue;
+
+// If the CurrentObject is a field of OtherObject, it will be analyzed
+// during the analysis of OtherObject.
NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Szelethus wrote:
> > > > NoQ wrote:
> > > > > It seems safer to look at `CXXConstructExpr::getConstructionKind()`.
> > > > >
> > > > > Taking a `LazyCompoundVal` and converting it back to a region is
> > > > > definitely a bad idea because the region within `LazyCompoundVal` is
> > > > > completely immaterial and carries no meaning for the value
> > > > > represented by this `SVal`; it's not necessarily the region you're
> > > > > looking for.
> > > > Looking at the singleton test case, I think I need to get that region
> > > > somehow, and I'm not too sure what you meant under using
> > > > `CXXConstructExpr::getConstructionKind()`. One thing I could do, is
> > > > similar to how `getObjectVal` works:
> > > > ```
> > > > Loc Tmp = Context.getSValBuilder().getCXXThis(OtherCtor->getParent(),
> > > >
> > > > Context.getStackFrame());
> > > >
> > > > auto OtherThis =
> > > > Context.getState()->getSVal(Tmp).castAs();
> > > >
> > > > OtherThis.getRegion(); // Hooray!
> > > > ```
> > > >
> > > > I have tested it, and it works wonders. Is this a "safe-to-use" region?
> > > You can directly ask `CXXConstructExpr` if it's a field or a base
> > > constructor. I could describe `getConstructionKind()` as some sort of
> > > very limited `ConstructionContext` that's available in the AST: it
> > > explains to you what sort of object is being constructed. I suspect it's
> > > enough for your purposes.
> > I think I'm aware of how it works, but my issue is that it's very limited:
> > ```
> > enum ConstructionKind { CK_Complete, CK_NonVirtualBase, CK_VirtualBase,
> > CK_Delegating }
> > ```
> > Sadly, whether the `CXXConstructExpr` was a field construction or a "top
> > level" (non-field) construction can't be retrieved from
> > `getConstructionKind()`, as I understand it.
> >
> > If I were to return true (say that the constructed object will be analyzed
> > later, thus ignoring it for now) if `getConstructionKind() == CK_Complete`,
> > the singleton testcase will fail (I have tested it). I'm very confident
> > that I need to obtain the constructed object's `MemRegion`.
> Whoops, yeah, i was wrong. I mis-remembered that there's a `CK_` for field.
>
> Then, yeah, the code looks good, let me recall where we were in terms of why
> do we need it><
"it" as if?
https://reviews.llvm.org/D48436
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:681-689
+Optional OtherObject =
+getObjectVal(OtherCtor, Context);
+if (!OtherObject)
+ continue;
+
+// If the CurrentObject is a field of OtherObject, it will be analyzed
+// during the analysis of OtherObject.
Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > It seems safer to look at `CXXConstructExpr::getConstructionKind()`.
> > > >
> > > > Taking a `LazyCompoundVal` and converting it back to a region is
> > > > definitely a bad idea because the region within `LazyCompoundVal` is
> > > > completely immaterial and carries no meaning for the value represented
> > > > by this `SVal`; it's not necessarily the region you're looking for.
> > > Looking at the singleton test case, I think I need to get that region
> > > somehow, and I'm not too sure what you meant under using
> > > `CXXConstructExpr::getConstructionKind()`. One thing I could do, is
> > > similar to how `getObjectVal` works:
> > > ```
> > > Loc Tmp = Context.getSValBuilder().getCXXThis(OtherCtor->getParent(),
> > > Context.getStackFrame());
> > >
> > > auto OtherThis =
> > > Context.getState()->getSVal(Tmp).castAs();
> > >
> > > OtherThis.getRegion(); // Hooray!
> > > ```
> > >
> > > I have tested it, and it works wonders. Is this a "safe-to-use" region?
> > You can directly ask `CXXConstructExpr` if it's a field or a base
> > constructor. I could describe `getConstructionKind()` as some sort of very
> > limited `ConstructionContext` that's available in the AST: it explains to
> > you what sort of object is being constructed. I suspect it's enough for
> > your purposes.
> I think I'm aware of how it works, but my issue is that it's very limited:
> ```
> enum ConstructionKind { CK_Complete, CK_NonVirtualBase, CK_VirtualBase,
> CK_Delegating }
> ```
> Sadly, whether the `CXXConstructExpr` was a field construction or a "top
> level" (non-field) construction can't be retrieved from
> `getConstructionKind()`, as I understand it.
>
> If I were to return true (say that the constructed object will be analyzed
> later, thus ignoring it for now) if `getConstructionKind() == CK_Complete`,
> the singleton testcase will fail (I have tested it). I'm very confident that
> I need to obtain the constructed object's `MemRegion`.
Whoops, yeah, i was wrong. I mis-remembered that there's a `CK_` for field.
Then, yeah, the code looks good, let me recall where we were in terms of why do
we need it><
https://reviews.llvm.org/D48436
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
Szelethus added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:681-689
+Optional OtherObject =
+getObjectVal(OtherCtor, Context);
+if (!OtherObject)
+ continue;
+
+// If the CurrentObject is a field of OtherObject, it will be analyzed
+// during the analysis of OtherObject.
NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > It seems safer to look at `CXXConstructExpr::getConstructionKind()`.
> > >
> > > Taking a `LazyCompoundVal` and converting it back to a region is
> > > definitely a bad idea because the region within `LazyCompoundVal` is
> > > completely immaterial and carries no meaning for the value represented by
> > > this `SVal`; it's not necessarily the region you're looking for.
> > Looking at the singleton test case, I think I need to get that region
> > somehow, and I'm not too sure what you meant under using
> > `CXXConstructExpr::getConstructionKind()`. One thing I could do, is similar
> > to how `getObjectVal` works:
> > ```
> > Loc Tmp = Context.getSValBuilder().getCXXThis(OtherCtor->getParent(),
> > Context.getStackFrame());
> >
> > auto OtherThis =
> > Context.getState()->getSVal(Tmp).castAs();
> >
> > OtherThis.getRegion(); // Hooray!
> > ```
> >
> > I have tested it, and it works wonders. Is this a "safe-to-use" region?
> You can directly ask `CXXConstructExpr` if it's a field or a base
> constructor. I could describe `getConstructionKind()` as some sort of very
> limited `ConstructionContext` that's available in the AST: it explains to you
> what sort of object is being constructed. I suspect it's enough for your
> purposes.
I think I'm aware of how it works, but my issue is that it's very limited:
```
enum ConstructionKind { CK_Complete, CK_NonVirtualBase, CK_VirtualBase,
CK_Delegating }
```
Sadly, whether the `CXXConstructExpr` was a field construction or a "top level"
(non-field) construction can't be retrieved from `getConstructionKind()`, as I
understand it.
If I were to return true (say that the constructed object will be analyzed
later, thus ignoring it for now) if `getConstructionKind() == CK_Complete`, the
singleton testcase will fail (I have tested it). I'm very confident that I need
to obtain the constructed object's `MemRegion`.
https://reviews.llvm.org/D48436
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
NoQ added a comment. Sorry, i'm still lagging with my reviews because there are a lot of them and i have to balance it with doing actual work. I'll hopefully get to this soon. Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:681-689 +Optional OtherObject = +getObjectVal(OtherCtor, Context); +if (!OtherObject) + continue; + +// If the CurrentObject is a field of OtherObject, it will be analyzed +// during the analysis of OtherObject. Szelethus wrote: > NoQ wrote: > > It seems safer to look at `CXXConstructExpr::getConstructionKind()`. > > > > Taking a `LazyCompoundVal` and converting it back to a region is definitely > > a bad idea because the region within `LazyCompoundVal` is completely > > immaterial and carries no meaning for the value represented by this `SVal`; > > it's not necessarily the region you're looking for. > Looking at the singleton test case, I think I need to get that region > somehow, and I'm not too sure what you meant under using > `CXXConstructExpr::getConstructionKind()`. One thing I could do, is similar > to how `getObjectVal` works: > ``` > Loc Tmp = Context.getSValBuilder().getCXXThis(OtherCtor->getParent(), > Context.getStackFrame()); > > auto OtherThis = > Context.getState()->getSVal(Tmp).castAs(); > > OtherThis.getRegion(); // Hooray! > ``` > > I have tested it, and it works wonders. Is this a "safe-to-use" region? You can directly ask `CXXConstructExpr` if it's a field or a base constructor. I could describe `getConstructionKind()` as some sort of very limited `ConstructionContext` that's available in the AST: it explains to you what sort of object is being constructed. I suspect it's enough for your purposes. https://reviews.llvm.org/D48436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
Szelethus added a comment. Polite ping :) https://reviews.llvm.org/D48436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
Szelethus added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:674
+ const LocationContext *LC = Context.getLocationContext();
+ while ((LC = LC->getParent())) {
+
Szelethus wrote:
> NoQ wrote:
> > george.karpenkov wrote:
> > > nit: could we have `while (LC)` followed by `LC = LC->getParent()` ? Do
> > > you intentionally skip the first location context?
> > I guess the predicate we're checking is trivially true for the current
> > location context.
> Completely missed this inline, sorry!
>
> As @NoQ said, since this checker only fires after a constructor call, the
> first location context will surely be that, and I'm only checking whether the
> current ctor was called by another.
I'm actually not sure how you imagined that. The loop condition is `LC`, but if
I assign it to its parent right after that, I won't be sure that it's non-null.
The reason why I placed it there is that early exits could restart the loop at
"random" places.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:681-689
+Optional OtherObject =
+getObjectVal(OtherCtor, Context);
+if (!OtherObject)
+ continue;
+
+// If the CurrentObject is a field of OtherObject, it will be analyzed
+// during the analysis of OtherObject.
NoQ wrote:
> It seems safer to look at `CXXConstructExpr::getConstructionKind()`.
>
> Taking a `LazyCompoundVal` and converting it back to a region is definitely a
> bad idea because the region within `LazyCompoundVal` is completely immaterial
> and carries no meaning for the value represented by this `SVal`; it's not
> necessarily the region you're looking for.
Looking at the singleton test case, I think I need to get that region somehow,
and I'm not too sure what you meant under using
`CXXConstructExpr::getConstructionKind()`. One thing I could do, is similar to
how `getObjectVal` works:
```
Loc Tmp = Context.getSValBuilder().getCXXThis(OtherCtor->getParent(),
Context.getStackFrame());
auto OtherThis = Context.getState()->getSVal(Tmp).castAs();
OtherThis.getRegion(); // Hooray!
```
I have tested it, and it works wonders. Is this a "safe-to-use" region?
https://reviews.llvm.org/D48436
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
Szelethus added a comment.
> I'll think about that a bit more; it might be worth it to track such deferred
> subregions in a state trait and drain it whenever we pop back to an explicit
> constructor.
There are so many things to consider, like the following case:
struct DynTBase {};
struct DynTDerived : DynTBase {
// TODO: we'd expect the note: {{uninitialized field 'this->x'}}
int x; // no-note
};
struct DynamicTypeTest {
DynTBase *bptr;
int i = 0;
// TODO: we'd expect the warning: {{1 uninitialized field}}
DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
};
void f() {
DynTDerived d;
DynamicTypeTest t();
};
As of now, the checker misses this one.
We talked about two approaches so far, the one already in the checker and the
one I proposed now. I think the current implementation solves this issue a lot
more naturally -- `isNonUnionUninit` has to be changed so that the fields
dynamic type is checked rather then its static type.
The proposed method would have a tough time dealing with this -- we don't want
an error for `DynTDerived::DynTDerived()`, since it's a compiler generated
ctor, but ideally we wouldn't like to have a warning for an object (`t`) and a
separate warning for it's field (`t.bptr.x`), but I'm afraid this new approach
would make this a necessity.
So, bottom line, there are a bunch of `TODO`s in the code to make sure this
issue is not forgotten, I strongly believe that we need a separate discussion
for this, if you'd agree :)
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:674
+ const LocationContext *LC = Context.getLocationContext();
+ while ((LC = LC->getParent())) {
+
NoQ wrote:
> george.karpenkov wrote:
> > nit: could we have `while (LC)` followed by `LC = LC->getParent()` ? Do you
> > intentionally skip the first location context?
> I guess the predicate we're checking is trivially true for the current
> location context.
Completely missed this inline, sorry!
As @NoQ said, since this checker only fires after a constructor call, the first
location context will surely be that, and I'm only checking whether the current
ctor was called by another.
https://reviews.llvm.org/D48436
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:674
+ const LocationContext *LC = Context.getLocationContext();
+ while ((LC = LC->getParent())) {
+
george.karpenkov wrote:
> nit: could we have `while (LC)` followed by `LC = LC->getParent()` ? Do you
> intentionally skip the first location context?
I guess the predicate we're checking is trivially true for the current location
context.
https://reviews.llvm.org/D48436
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
NoQ added a comment. > A call to `Derived::Derived()` previously emitted no warnings. However, with > these changes, a warning is emitted for `Base::a`. Yep, a heuristic to skip implicit constructor declarations during analysis and make the first explicit constructor responsible for initializing its bases/fields that have been constructed via autogenerated constructors seems reasonable. With that i guess you'll still have to deep-scan fields and bases, which prevents us from simplifying our code. I'll think about that a bit more; it might be worth it to track such deferred subregions in a state trait and drain it whenever we pop back to an explicit constructor. Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:681-689 +Optional OtherObject = +getObjectVal(OtherCtor, Context); +if (!OtherObject) + continue; + +// If the CurrentObject is a field of OtherObject, it will be analyzed +// during the analysis of OtherObject. It seems safer to look at `CXXConstructExpr::getConstructionKind()`. Taking a `LazyCompoundVal` and converting it back to a region is definitely a bad idea because the region within `LazyCompoundVal` is completely immaterial and carries no meaning for the value represented by this `SVal`; it's not necessarily the region you're looking for. https://reviews.llvm.org/D48436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
Szelethus added a comment. Polite ping ^-^ https://reviews.llvm.org/D48436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
Szelethus added a comment. In https://reviews.llvm.org/D48436#1144380, @NoQ wrote: > I think we need to finish our dialog on who's responsible for initialization > and why do we need to filter constructors at all, cause it's kinda hanging > (i.e. https://reviews.llvm.org/D45532#inline-422673). Agreed. I have some code in the works, and I planned to upload it as a separate patch. Note that I added TODO's around base class handling to avoid forgetting this. Here's my current thinking of the topic: I changed my mind, and base classes should be handled on their on. This has a number of benefits https://reviews.llvm.org/D48436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
NoQ added a comment. I think we need to finish our dialog on who's responsible for initialization and why do we need to filter constructors at all, cause it's kinda hanging (i.e. https://reviews.llvm.org/D45532#inline-422673). https://reviews.llvm.org/D48436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
george.karpenkov accepted this revision.
george.karpenkov added a comment.
This revision is now accepted and ready to land.
LGTM with a nit.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:674
+ const LocationContext *LC = Context.getLocationContext();
+ while ((LC = LC->getParent())) {
+
nit: could we have `while (LC)` followed by `LC = LC->getParent()` ? Do you
intentionally skip the first location context?
https://reviews.llvm.org/D48436
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
Szelethus updated this revision to Diff 152343.
Szelethus added a comment.
Moved `LC = LC ->getParent()` to the `while` statement's argument to avoid a
potential infinite loop. Whoops :)
https://reviews.llvm.org/D48436
Files:
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
test/Analysis/cxx-uninitialized-object.cpp
Index: test/Analysis/cxx-uninitialized-object.cpp
===
--- test/Analysis/cxx-uninitialized-object.cpp
+++ test/Analysis/cxx-uninitialized-object.cpp
@@ -1035,13 +1035,12 @@
// While a singleton would make more sense as a static variable, that would zero
// initialize all of its fields, hence the not too practical implementation.
struct Singleton {
- // TODO: we'd expect the note: {{uninitialized field 'this->i'}}
- int i; // no-note
+ int i; // expected-note{{uninitialized field 'this->i'}}
+ int dontGetFilteredByNonPedanticMode = 0;
Singleton() {
assert(!isInstantiated);
-// TODO: we'd expect the warning: {{1 uninitialized field}}
-isInstantiated = true; // no-warning
+isInstantiated = true; // expected-warning{{1 uninitialized field}}
}
~Singleton() {
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -212,9 +212,11 @@
Optional
getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext );
-/// Checks whether the constructor under checking is called by another
-/// constructor.
-bool isCalledByConstructor(const CheckerContext );
+/// Checks whether the object constructed by \p Ctor will be analyzed later
+/// (e.g. if the object is a field of another object, in which case we'd check
+/// it multiple times).
+bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
+ CheckerContext );
/// Returns whether FD can be (transitively) dereferenced to a void pointer type
/// (void*, void**, ...). The type of the region behind a void pointer isn't
@@ -255,7 +257,7 @@
return;
// This avoids essentially the same error being reported multiple times.
- if (isCalledByConstructor(Context))
+ if (willObjectBeAnalyzedLater(CtorDecl, Context))
return;
Optional Object = getObjectVal(CtorDecl, Context);
@@ -419,8 +421,8 @@
}
// Checking bases.
- // FIXME: As of now, because of `isCalledByConstructor`, objects whose type
- // is a descendant of another type will emit warnings for uninitalized
+ // FIXME: As of now, because of `willObjectBeAnalyzedLater`, objects whose
+ // type is a descendant of another type will emit warnings for uninitalized
// inherited members.
// This is not the only way to analyze bases of an object -- if we didn't
// filter them out, and didn't analyze the bases, this checker would run for
@@ -661,18 +663,32 @@
return Object.getAs();
}
-// TODO: We should also check that if the constructor was called by another
-// constructor, whether those two are in any relation to one another. In it's
-// current state, this introduces some false negatives.
-bool isCalledByConstructor(const CheckerContext ) {
- const LocationContext *LC = Context.getLocationContext()->getParent();
+bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
+ CheckerContext ) {
- while (LC) {
-if (isa(LC->getDecl()))
- return true;
+ Optional CurrentObject = getObjectVal(Ctor, Context);
+ if (!CurrentObject)
+return false;
+
+ const LocationContext *LC = Context.getLocationContext();
+ while ((LC = LC->getParent())) {
+
+// If \p Ctor was called by another constructor.
+const auto *OtherCtor = dyn_cast(LC->getDecl());
+if (!OtherCtor)
+ continue;
-LC = LC->getParent();
+Optional OtherObject =
+getObjectVal(OtherCtor, Context);
+if (!OtherObject)
+ continue;
+
+// If the CurrentObject is a field of OtherObject, it will be analyzed
+// during the analysis of OtherObject.
+if (CurrentObject->getRegion()->isSubRegionOf(OtherObject->getRegion()))
+ return true;
}
+
return false;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
Szelethus created this revision.
Szelethus added reviewers: george.karpenkov, NoQ, xazax.hun, rnkovacs.
Herald added subscribers: cfe-commits, mikhail.ramalho, a.sidorin, szepet,
whisperity.
As of now, all constructor calls are ignored that are being called by a
constructor. The point of this was not to analyze the fields of an object, so
an uninitialized field wouldn't be reported multiple times.
This however introduced false negatives when the two constructors were in no
relation to one another -- see the test file for a neat example for this with
singletons.
This patch aims so fix this issue.
Repository:
rC Clang
https://reviews.llvm.org/D48436
Files:
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
test/Analysis/cxx-uninitialized-object.cpp
Index: test/Analysis/cxx-uninitialized-object.cpp
===
--- test/Analysis/cxx-uninitialized-object.cpp
+++ test/Analysis/cxx-uninitialized-object.cpp
@@ -1035,13 +1035,12 @@
// While a singleton would make more sense as a static variable, that would
zero
// initialize all of its fields, hence the not too practical implementation.
struct Singleton {
- // TODO: we'd expect the note: {{uninitialized field 'this->i'}}
- int i; // no-note
+ int i; // expected-note{{uninitialized field 'this->i'}}
+ int dontGetFilteredByNonPedanticMode = 0;
Singleton() {
assert(!isInstantiated);
-// TODO: we'd expect the warning: {{1 uninitialized field}}
-isInstantiated = true; // no-warning
+isInstantiated = true; // expected-warning{{1 uninitialized field}}
}
~Singleton() {
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -212,9 +212,11 @@
Optional
getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext );
-/// Checks whether the constructor under checking is called by another
-/// constructor.
-bool isCalledByConstructor(const CheckerContext );
+/// Checks whether the object constructed by \p Ctor will be analyzed later
+/// (e.g. if the object is a field of another object, in which case we'd check
+/// it multiple times).
+bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
+ CheckerContext );
/// Returns whether FD can be (transitively) dereferenced to a void pointer
type
/// (void*, void**, ...). The type of the region behind a void pointer isn't
@@ -255,7 +257,7 @@
return;
// This avoids essentially the same error being reported multiple times.
- if (isCalledByConstructor(Context))
+ if (willObjectBeAnalyzedLater(CtorDecl, Context))
return;
Optional Object = getObjectVal(CtorDecl, Context);
@@ -419,8 +421,8 @@
}
// Checking bases.
- // FIXME: As of now, because of `isCalledByConstructor`, objects whose type
- // is a descendant of another type will emit warnings for uninitalized
+ // FIXME: As of now, because of `willObjectBeAnalyzedLater`, objects whose
+ // type is a descendant of another type will emit warnings for uninitalized
// inherited members.
// This is not the only way to analyze bases of an object -- if we didn't
// filter them out, and didn't analyze the bases, this checker would run for
@@ -661,15 +663,29 @@
return Object.getAs();
}
-// TODO: We should also check that if the constructor was called by another
-// constructor, whether those two are in any relation to one another. In it's
-// current state, this introduces some false negatives.
-bool isCalledByConstructor(const CheckerContext ) {
+bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
+ CheckerContext ) {
const LocationContext *LC = Context.getLocationContext()->getParent();
+ Optional CurrentObject =
+ getObjectVal(Ctor, Context);
+ if (!CurrentObject)
+return false;
+
while (LC) {
-if (isa(LC->getDecl()))
- return true;
+// If this constructor was called by another constructor.
+if (const auto *OtherCtor = dyn_cast(LC->getDecl())) {
+
+ Optional OtherObject =
+ getObjectVal(OtherCtor, Context);
+ if (!OtherObject)
+continue;
+
+ // If the CurrentObject is a field of OtherObject, it will be analyzed
+ // during the analysis of OtherObject.
+ if (CurrentObject->getRegion()->isSubRegionOf(OtherObject->getRegion()))
+return true;
+}
LC = LC->getParent();
}
Index: test/Analysis/cxx-uninitialized-object.cpp
===
--- test/Analysis/cxx-uninitialized-object.cpp
+++ test/Analysis/cxx-uninitialized-object.cpp
@@ -1035,13 +1035,12 @@
// While a singleton would make more sense as a static variable, that would zero
// initialize all of its fields, hence the not too
