[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336994: [analyzer][UninitializedObjectChecker] Support for
MemberPointerTypes (authored by Szelethus, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D48325?vs=155356=155357#toc
Repository:
rL LLVM
https://reviews.llvm.org/D48325
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
cfe/trunk/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
Index: cfe/trunk/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
===
--- cfe/trunk/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
+++ cfe/trunk/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
@@ -416,14 +416,12 @@
#ifdef PEDANTIC
struct PointerToMemberFunctionTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- void (UsefulFunctions::*f)(void); // no-note
+ void (UsefulFunctions::*f)(void); // expected-note{{uninitialized field 'this->f'}}
PointerToMemberFunctionTest1() {}
};
void fPointerToMemberFunctionTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberFunctionTest1(); // no-warning
+ PointerToMemberFunctionTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberFunctionTest2 {
@@ -460,14 +458,12 @@
}
struct PointerToMemberDataTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- int UsefulFunctions::*d; // no-note
+ int UsefulFunctions::*d; // expected-note{{uninitialized field 'this->d'}}
PointerToMemberDataTest1() {}
};
void fPointerToMemberDataTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberDataTest1(); // no-warning
+ PointerToMemberDataTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberDataTest2 {
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -131,12 +131,10 @@
// - a non-union record
// - a pointer/reference
// - an array
- // - of a member pointer type
- // - of a primitive type, which we'll define as either a BuiltinType or
- // EnumeralType.
+ // - of a primitive type, which we'll define later in a helper function.
// * the parent of each node is the object that contains it
- // * every leaf is an array, a primitive object, a member pointer, a nullptr
- // or an undefined pointer.
+ // * every leaf is an array, a primitive object, a nullptr or an undefined
+ // pointer.
//
// Example:
//
@@ -163,8 +161,8 @@
//
// From this we'll construct a vector of fieldchains, where each fieldchain
// represents an uninitialized field. An uninitialized field may be a
- // primitive object, a member pointer, a pointer, a pointee or a union without
- // a single initialized field.
+ // primitive object, a pointer, a pointee or a union without a single
+ // initialized field.
// In the above example, for the default constructor call we'll end up with
// these fieldchains:
//
@@ -189,10 +187,6 @@
bool isPointerOrReferenceUninit(const FieldRegion *FR,
FieldChainInfo LocalChain);
- /// This method checks a region of MemberPointerType, and returns true if the
- /// the pointer is uninitialized.
- bool isMemberPointerUninit(const FieldRegion *FR, FieldChainInfo LocalChain);
-
/// This method returns true if the value of a primitive object is
/// uninitialized.
bool isPrimitiveUninit(const SVal );
@@ -225,10 +219,13 @@
/// known, and thus FD can not be analyzed.
static bool isVoidPointer(const FieldDecl *FD);
-/// Returns true if T is a primitive type. We'll call a type primitive if it's
-/// either a BuiltinType or an EnumeralType.
+/// Returns true if T is a primitive type. We defined this type so that for
+/// objects that we'd only like analyze as much as checking whether their
+/// value is undefined or not, such as ints and doubles, can be analyzed with
+/// ease. This also helps ensuring that every special field type is handled
+/// correctly.
static bool isPrimitiveType(const QualType ) {
- return T->isBuiltinType() || T->isEnumeralType();
+ return T->isBuiltinType() || T->isEnumeralType() || T->isMemberPointerType();
}
/// Constructs a note message for a given FieldChainInfo object.
@@ -392,13 +389,6 @@
continue;
}
-if (T->isMemberPointerType()) {
- if (isMemberPointerUninit(FR, LocalChain))
-ContainsUninitField = true;
- continue;
-}
-
-// If this is a pointer or reference type.
[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
Szelethus updated this revision to Diff 155356.
Szelethus added a comment.
Thank you! ^-^
Rebased to https://reviews.llvm.org/rC336901.
https://reviews.llvm.org/D48325
Files:
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
Index: test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
===
--- test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
+++ test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
@@ -416,14 +416,12 @@
#ifdef PEDANTIC
struct PointerToMemberFunctionTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- void (UsefulFunctions::*f)(void); // no-note
+ void (UsefulFunctions::*f)(void); // expected-note{{uninitialized field 'this->f'}}
PointerToMemberFunctionTest1() {}
};
void fPointerToMemberFunctionTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberFunctionTest1(); // no-warning
+ PointerToMemberFunctionTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberFunctionTest2 {
@@ -460,14 +458,12 @@
}
struct PointerToMemberDataTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- int UsefulFunctions::*d; // no-note
+ int UsefulFunctions::*d; // expected-note{{uninitialized field 'this->d'}}
PointerToMemberDataTest1() {}
};
void fPointerToMemberDataTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberDataTest1(); // no-warning
+ PointerToMemberDataTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberDataTest2 {
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -131,12 +131,10 @@
// - a non-union record
// - a pointer/reference
// - an array
- // - of a member pointer type
- // - of a primitive type, which we'll define as either a BuiltinType or
- // EnumeralType.
+ // - of a primitive type, which we'll define later in a helper function.
// * the parent of each node is the object that contains it
- // * every leaf is an array, a primitive object, a member pointer, a nullptr
- // or an undefined pointer.
+ // * every leaf is an array, a primitive object, a nullptr or an undefined
+ // pointer.
//
// Example:
//
@@ -163,8 +161,8 @@
//
// From this we'll construct a vector of fieldchains, where each fieldchain
// represents an uninitialized field. An uninitialized field may be a
- // primitive object, a member pointer, a pointer, a pointee or a union without
- // a single initialized field.
+ // primitive object, a pointer, a pointee or a union without a single
+ // initialized field.
// In the above example, for the default constructor call we'll end up with
// these fieldchains:
//
@@ -189,10 +187,6 @@
bool isPointerOrReferenceUninit(const FieldRegion *FR,
FieldChainInfo LocalChain);
- /// This method checks a region of MemberPointerType, and returns true if the
- /// the pointer is uninitialized.
- bool isMemberPointerUninit(const FieldRegion *FR, FieldChainInfo LocalChain);
-
/// This method returns true if the value of a primitive object is
/// uninitialized.
bool isPrimitiveUninit(const SVal );
@@ -225,10 +219,13 @@
/// known, and thus FD can not be analyzed.
static bool isVoidPointer(const FieldDecl *FD);
-/// Returns true if T is a primitive type. We'll call a type primitive if it's
-/// either a BuiltinType or an EnumeralType.
+/// Returns true if T is a primitive type. We defined this type so that for
+/// objects that we'd only like analyze as much as checking whether their
+/// value is undefined or not, such as ints and doubles, can be analyzed with
+/// ease. This also helps ensuring that every special field type is handled
+/// correctly.
static bool isPrimitiveType(const QualType ) {
- return T->isBuiltinType() || T->isEnumeralType();
+ return T->isBuiltinType() || T->isEnumeralType() || T->isMemberPointerType();
}
/// Constructs a note message for a given FieldChainInfo object.
@@ -392,13 +389,6 @@
continue;
}
-if (T->isMemberPointerType()) {
- if (isMemberPointerUninit(FR, LocalChain))
-ContainsUninitField = true;
- continue;
-}
-
-// If this is a pointer or reference type.
if (T->isPointerType() || T->isReferenceType()) {
if (isPointerOrReferenceUninit(FR, LocalChain))
ContainsUninitField = true;
@@ -542,14 +532,6 @@
return false;
}
-bool FindUninitializedFields::isMemberPointerUninit(const FieldRegion *FR,
-FieldChainInfo LocalChain) {
-
[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
NoQ accepted this revision. NoQ added a comment. Yay less code. https://reviews.llvm.org/D48325 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
Szelethus updated this revision to Diff 154398.
Szelethus added a comment.
MemberPointerTypes are regarded as primitive types from now. I agree with @NoQ,
it makes so much more sense this way :)
https://reviews.llvm.org/D48325
Files:
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
Index: test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
===
--- test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
+++ test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
@@ -416,14 +416,12 @@
#ifdef PEDANTIC
struct PointerToMemberFunctionTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- void (UsefulFunctions::*f)(void); // no-note
+ void (UsefulFunctions::*f)(void); // expected-note{{uninitialized field 'this->f'}}
PointerToMemberFunctionTest1() {}
};
void fPointerToMemberFunctionTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberFunctionTest1(); // no-warning
+ PointerToMemberFunctionTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberFunctionTest2 {
@@ -460,14 +458,12 @@
}
struct PointerToMemberDataTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- int UsefulFunctions::*d; // no-note
+ int UsefulFunctions::*d; // expected-note{{uninitialized field 'this->d'}}
PointerToMemberDataTest1() {}
};
void fPointerToMemberDataTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberDataTest1(); // no-warning
+ PointerToMemberDataTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberDataTest2 {
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -133,12 +133,10 @@
// - a non-union record
// - a pointer/reference
// - an array
- // - of a member pointer type
- // - of a primitive type, which we'll define as either a BuiltinType or
- // EnumeralType.
+ // - of a primitive type, which we'll define later in a helper function.
// * the parent of each node is the object that contains it
- // * every leaf is an array, a primitive object, a member pointer, a nullptr
- // or an undefined pointer.
+ // * every leaf is an array, a primitive object, a nullptr or an undefined
+ // pointer.
//
// Example:
//
@@ -165,8 +163,8 @@
//
// From this we'll construct a vector of fieldchains, where each fieldchain
// represents an uninitialized field. An uninitialized field may be a
- // primitive object, a member pointer, a pointer, a pointee or a union without
- // a single initialized field.
+ // primitive object, a pointer, a pointee or a union without a single
+ // initialized field.
// In the above example, for the default constructor call we'll end up with
// these fieldchains:
//
@@ -191,10 +189,6 @@
bool isPointerOrReferenceUninit(const FieldRegion *FR,
FieldChainInfo LocalChain);
- /// This method checks a region of MemberPointerType, and returns true if the
- /// the pointer is uninitialized.
- bool isMemberPointerUninit(const FieldRegion *FR, FieldChainInfo LocalChain);
-
/// This method returns true if the value of a primitive object is
/// uninitialized.
bool isPrimitiveUninit(const SVal );
@@ -221,10 +215,13 @@
/// known, and thus FD can not be analyzed.
bool isVoidPointer(const FieldDecl *FD);
-/// Returns true if T is a primitive type. We'll call a type primitive if it's
-/// either a BuiltinType or an EnumeralType.
+/// Returns true if T is a primitive type. We defined this type so that for
+/// objects that we'd only like analyze as much as checking whether their
+/// value is undefined or not, such as ints and doubles, can be analyzed with
+/// ease. This also helps ensuring that every special field type is handled
+/// correctly.
bool isPrimitiveType(const QualType ) {
- return T->isBuiltinType() || T->isEnumeralType();
+ return T->isBuiltinType() || T->isEnumeralType() || T->isMemberPointerType();
}
/// Constructs a note message for a given FieldChainInfo object.
@@ -389,13 +386,6 @@
continue;
}
-if (T->isMemberPointerType()) {
- if (isMemberPointerUninit(FR, LocalChain))
-ContainsUninitField = true;
- continue;
-}
-
-// If this is a pointer or reference type.
if (T->isPointerType() || T->isReferenceType()) {
if (isPointerOrReferenceUninit(FR, LocalChain))
ContainsUninitField = true;
@@ -539,14 +529,6 @@
return false;
}
-bool FindUninitializedFields::isMemberPointerUninit(const FieldRegion *FR,
-FieldChainInfo
[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:392
-if (T->isMemberPointerType()) {
- if (isMemberPointerUninit(FR, LocalChain))
+if (T->isPointerType() || T->isReferenceType()) {
+ if (isPointerOrReferenceUninit(FR, LocalChain))
Szelethus wrote:
> NoQ wrote:
> > george.karpenkov wrote:
> > > I am confused here, how can references be uninitialized?
> > Hmm, that's actually a good question. I guess we technically initialize a
> > reference with an undefined pointer, but that should be caught by another
> > checker early because it's an immediate UB.
> >
> > In any case, there don't seem to be tests for this.
> I guess you could say that naming could be improved upon. I didn't mean that
> the reference object itself, but rather it's pointee is uninitialized.
>
> ```
> int a; // say that this isn't zero initialized
> int = a;
> ```
>
> There are numerous tests for this both for left value and right value
> references in `cxx-uninitialized-object-ptr-ref.cpp`.
Aha, oh, i see, you just reordered the `if`s and it seemed as if you're
suddenly doing new stuff with references, sorry><
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:488-491
// TODO: Dereferencing should be done according to the dynamic type.
while (Optional L = DerefdV.getAs()) {
DerefdV = State->getSVal(*L);
}
Szelethus wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Ok, this isn't related to that review, but i still don't understand why
> > > this loop terminates. We might skip `void *` above, but we don't skip
> > > `void *`, right? And if we go on dereferencing that, we'll eventually
> > > get to a `void *` that can point to anything.
> > >
> > > A bit more of an intuition dump: I strongly believe that in `getSVal(Loc,
> > > T)` the optional argument `T` should be mandatory. Omitting it almost
> > > always leads to errors, because there are no types in machine memory, and
> > > our `RegionStore` tries to work exactly like machine memory, and
> > > `getSVal(Loc)` makes a best-effort guess on what the type of the region
> > > is, which in some cases may work reliably (these are exactly the cases
> > > where you don't mind passing the type directly), but is usually
> > > error-prone in other cases. So whenever i see a `getSVal(Loc)` without a
> > > type, i imagine that the code essentially reads raw bytes from the
> > > symbolic memory and makes random guesses on what they mean.
> > >
> > > So the point is "oh we can do better with dynamic type info", it's "we're
> > > doing something completely unreliable here". It's not just about void
> > > pointers, it's about the fact that the underlying storage simply has no
> > > types at all.
> > From the code a couple lines back:
> >
> > if (isVoidPointer(FD)) {
> > IsAnyFieldInitialized = true;
> > return false;
> > }
> >
> > Note that isn't `Type::isVoidPointer`, I'm calling a statically defined
> > function to filter out all void pointer cases.
> > ```
> > /// 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
> > /// known, and thus FD can not be analyzed.
> > bool isVoidPointer(const FieldDecl *FD);
> > ```
> >
> > I see your point, this is a bit dodgy. I'm already working on a fix, and
> > hope to upload it along with other important fixes during the next week.
> > It's high on my priority list :)
> Also note that there are tests in `cxx-uninitialized-object-ptr-ref.cpp` for
> `void*`, `void*&`, `void**`, and `void**&&`.
Aha, yeah, i see, got it, yeah.
I think the way this should work is you take the type of the field `FR`, unwrap
it to the pointee type as long as it is a pointer type, and dereference the
pointer with that unwrapped type every time you unwrap it.
This would terminate because you'll eventually get to the leaf type. Values
loaded from the store would also match types that the program would see if it
tried to dereference the pointer that many times.
https://reviews.llvm.org/D48325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
Szelethus added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:488-491
// TODO: Dereferencing should be done according to the dynamic type.
while (Optional L = DerefdV.getAs()) {
DerefdV = State->getSVal(*L);
}
Szelethus wrote:
> NoQ wrote:
> > Ok, this isn't related to that review, but i still don't understand why
> > this loop terminates. We might skip `void *` above, but we don't skip `void
> > *`, right? And if we go on dereferencing that, we'll eventually get to
> > a `void *` that can point to anything.
> >
> > A bit more of an intuition dump: I strongly believe that in `getSVal(Loc,
> > T)` the optional argument `T` should be mandatory. Omitting it almost
> > always leads to errors, because there are no types in machine memory, and
> > our `RegionStore` tries to work exactly like machine memory, and
> > `getSVal(Loc)` makes a best-effort guess on what the type of the region is,
> > which in some cases may work reliably (these are exactly the cases where
> > you don't mind passing the type directly), but is usually error-prone in
> > other cases. So whenever i see a `getSVal(Loc)` without a type, i imagine
> > that the code essentially reads raw bytes from the symbolic memory and
> > makes random guesses on what they mean.
> >
> > So the point is "oh we can do better with dynamic type info", it's "we're
> > doing something completely unreliable here". It's not just about void
> > pointers, it's about the fact that the underlying storage simply has no
> > types at all.
> From the code a couple lines back:
>
> if (isVoidPointer(FD)) {
> IsAnyFieldInitialized = true;
> return false;
> }
>
> Note that isn't `Type::isVoidPointer`, I'm calling a statically defined
> function to filter out all void pointer cases.
> ```
> /// 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
> /// known, and thus FD can not be analyzed.
> bool isVoidPointer(const FieldDecl *FD);
> ```
>
> I see your point, this is a bit dodgy. I'm already working on a fix, and hope
> to upload it along with other important fixes during the next week. It's high
> on my priority list :)
Also note that there are tests in `cxx-uninitialized-object-ptr-ref.cpp` for
`void*`, `void*&`, `void**`, and `void**&&`.
https://reviews.llvm.org/D48325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
Szelethus added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:392
-if (T->isMemberPointerType()) {
- if (isMemberPointerUninit(FR, LocalChain))
+if (T->isPointerType() || T->isReferenceType()) {
+ if (isPointerOrReferenceUninit(FR, LocalChain))
NoQ wrote:
> george.karpenkov wrote:
> > I am confused here, how can references be uninitialized?
> Hmm, that's actually a good question. I guess we technically initialize a
> reference with an undefined pointer, but that should be caught by another
> checker early because it's an immediate UB.
>
> In any case, there don't seem to be tests for this.
I guess you could say that naming could be improved upon. I didn't mean that
the reference object itself, but rather it's pointee is uninitialized.
```
int a; // say that this isn't zero initialized
int = a;
```
There are numerous tests for this both for left value and right value
references in `cxx-uninitialized-object-ptr-ref.cpp`.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:488-491
// TODO: Dereferencing should be done according to the dynamic type.
while (Optional L = DerefdV.getAs()) {
DerefdV = State->getSVal(*L);
}
NoQ wrote:
> Ok, this isn't related to that review, but i still don't understand why this
> loop terminates. We might skip `void *` above, but we don't skip `void
> *`, right? And if we go on dereferencing that, we'll eventually get to a
> `void *` that can point to anything.
>
> A bit more of an intuition dump: I strongly believe that in `getSVal(Loc, T)`
> the optional argument `T` should be mandatory. Omitting it almost always
> leads to errors, because there are no types in machine memory, and our
> `RegionStore` tries to work exactly like machine memory, and `getSVal(Loc)`
> makes a best-effort guess on what the type of the region is, which in some
> cases may work reliably (these are exactly the cases where you don't mind
> passing the type directly), but is usually error-prone in other cases. So
> whenever i see a `getSVal(Loc)` without a type, i imagine that the code
> essentially reads raw bytes from the symbolic memory and makes random guesses
> on what they mean.
>
> So the point is "oh we can do better with dynamic type info", it's "we're
> doing something completely unreliable here". It's not just about void
> pointers, it's about the fact that the underlying storage simply has no types
> at all.
From the code a couple lines back:
if (isVoidPointer(FD)) {
IsAnyFieldInitialized = true;
return false;
}
Note that isn't `Type::isVoidPointer`, I'm calling a statically defined
function to filter out all void pointer cases.
```
/// 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
/// known, and thus FD can not be analyzed.
bool isVoidPointer(const FieldDecl *FD);
```
I see your point, this is a bit dodgy. I'm already working on a fix, and hope
to upload it along with other important fixes during the next week. It's high
on my priority list :)
https://reviews.llvm.org/D48325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:392
-if (T->isMemberPointerType()) {
- if (isMemberPointerUninit(FR, LocalChain))
+if (T->isPointerType() || T->isReferenceType()) {
+ if (isPointerOrReferenceUninit(FR, LocalChain))
george.karpenkov wrote:
> I am confused here, how can references be uninitialized?
Hmm, that's actually a good question. I guess we technically initialize a
reference with an undefined pointer, but that should be caught by another
checker early because it's an immediate UB.
In any case, there don't seem to be tests for this.
https://reviews.llvm.org/D48325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
NoQ accepted this revision.
NoQ added a comment.
Looks good! One minor comment.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:488-491
// TODO: Dereferencing should be done according to the dynamic type.
while (Optional L = DerefdV.getAs()) {
DerefdV = State->getSVal(*L);
}
Ok, this isn't related to that review, but i still don't understand why this
loop terminates. We might skip `void *` above, but we don't skip `void *`,
right? And if we go on dereferencing that, we'll eventually get to a `void *`
that can point to anything.
A bit more of an intuition dump: I strongly believe that in `getSVal(Loc, T)`
the optional argument `T` should be mandatory. Omitting it almost always leads
to errors, because there are no types in machine memory, and our `RegionStore`
tries to work exactly like machine memory, and `getSVal(Loc)` makes a
best-effort guess on what the type of the region is, which in some cases may
work reliably (these are exactly the cases where you don't mind passing the
type directly), but is usually error-prone in other cases. So whenever i see a
`getSVal(Loc)` without a type, i imagine that the code essentially reads raw
bytes from the symbolic memory and makes random guesses on what they mean.
So the point is "oh we can do better with dynamic type info", it's "we're doing
something completely unreliable here". It's not just about void pointers, it's
about the fact that the underlying storage simply has no types at all.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:524
+if (T->isMemberPointerType()) {
+ return isMemberPointerUninit(FR, LocalChain);
+}
Why not just `isPrimitiveUninit()`? I believe that there shouldn't really be
any difference, because member pointer types are kinda primitive. You can't
really "dereference" a member pointer, at least not on its own, so i think
there's no need to make an extra function here.
https://reviews.llvm.org/D48325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.
Requesting changes or clarification on uninitialized references.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:392
-if (T->isMemberPointerType()) {
- if (isMemberPointerUninit(FR, LocalChain))
+if (T->isPointerType() || T->isReferenceType()) {
+ if (isPointerOrReferenceUninit(FR, LocalChain))
I am confused here, how can references be uninitialized?
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:393
+if (T->isPointerType() || T->isReferenceType()) {
+ if (isPointerOrReferenceUninit(FR, LocalChain))
ContainsUninitField = true;
Same question here (though that's for a different commit) -- I'm confused on
how a reference can be uninitialized after the constructor call has finished.
https://reviews.llvm.org/D48325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
Szelethus added a comment. Polite ping :) https://reviews.llvm.org/D48325 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
Szelethus updated this revision to Diff 151943.
Szelethus added a comment.
Accidently added the pointer `MemberPointer` objects twice, fixed that.
https://reviews.llvm.org/D48325
Files:
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
Index: test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
===
--- test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
+++ test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
@@ -416,14 +416,12 @@
#ifdef PEDANTIC
struct PointerToMemberFunctionTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- void (UsefulFunctions::*f)(void); // no-note
+ void (UsefulFunctions::*f)(void); // expected-note{{uninitialized field
'this->f'}}
PointerToMemberFunctionTest1() {}
};
void fPointerToMemberFunctionTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberFunctionTest1(); // no-warning
+ PointerToMemberFunctionTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberFunctionTest2 {
@@ -460,14 +458,12 @@
}
struct PointerToMemberDataTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- int UsefulFunctions::*d; // no-note
+ int UsefulFunctions::*d; // expected-note{{uninitialized field 'this->d'}}
PointerToMemberDataTest1() {}
};
void fPointerToMemberDataTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberDataTest1(); // no-warning
+ PointerToMemberDataTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberDataTest2 {
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -389,15 +389,14 @@
continue;
}
-if (T->isMemberPointerType()) {
- if (isMemberPointerUninit(FR, LocalChain))
+if (T->isPointerType() || T->isReferenceType()) {
+ if (isPointerOrReferenceUninit(FR, LocalChain))
ContainsUninitField = true;
continue;
}
-// If this is a pointer or reference type.
-if (T->isPointerType() || T->isReferenceType()) {
- if (isPointerOrReferenceUninit(FR, LocalChain))
+if (T->isMemberPointerType()) {
+ if (isMemberPointerUninit(FR, LocalChain))
ContainsUninitField = true;
continue;
}
@@ -521,6 +520,10 @@
}
}
+if (T->isMemberPointerType()) {
+ return isMemberPointerUninit(FR, LocalChain);
+}
+
if (T->isArrayType()) {
IsAnyFieldInitialized = true;
return false;
@@ -543,7 +546,12 @@
FieldChainInfo LocalChain)
{
assert(FR->getDecl()->getType()->isMemberPointerType() &&
"This function only checks regions that hold MemberPointerTypes!");
- // TODO: Implement support for MemberPointerTypes.
+
+ SVal V = State->getSVal(FR);
+ if (V.isUndef())
+return addFieldToUninits({LocalChain, FR});
+
+ IsAnyFieldInitialized = true;
return false;
}
Index: test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
===
--- test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
+++ test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
@@ -416,14 +416,12 @@
#ifdef PEDANTIC
struct PointerToMemberFunctionTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- void (UsefulFunctions::*f)(void); // no-note
+ void (UsefulFunctions::*f)(void); // expected-note{{uninitialized field 'this->f'}}
PointerToMemberFunctionTest1() {}
};
void fPointerToMemberFunctionTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberFunctionTest1(); // no-warning
+ PointerToMemberFunctionTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberFunctionTest2 {
@@ -460,14 +458,12 @@
}
struct PointerToMemberDataTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- int UsefulFunctions::*d; // no-note
+ int UsefulFunctions::*d; // expected-note{{uninitialized field 'this->d'}}
PointerToMemberDataTest1() {}
};
void fPointerToMemberDataTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberDataTest1(); // no-warning
+ PointerToMemberDataTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberDataTest2 {
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -389,15 +389,14 @@
continue;
}
-if (T->isMemberPointerType()) {
- if
[PATCH] D48325: [analyzer][UninitializedObjectChecker] Support for MemberPointerTypes
Szelethus created this revision.
Szelethus added reviewers: NoQ, george.karpenkov, xazax.hun, rnkovacs.
Herald added subscribers: cfe-commits, mikhail.ramalho, a.sidorin, szepet,
whisperity.
Repository:
rC Clang
https://reviews.llvm.org/D48325
Files:
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
Index: test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
===
--- test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
+++ test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
@@ -416,14 +416,12 @@
#ifdef PEDANTIC
struct PointerToMemberFunctionTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- void (UsefulFunctions::*f)(void); // no-note
+ void (UsefulFunctions::*f)(void); // expected-note{{uninitialized field
'this->f'}}
PointerToMemberFunctionTest1() {}
};
void fPointerToMemberFunctionTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberFunctionTest1(); // no-warning
+ PointerToMemberFunctionTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberFunctionTest2 {
@@ -460,14 +458,12 @@
}
struct PointerToMemberDataTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- int UsefulFunctions::*d; // no-note
+ int UsefulFunctions::*d; // expected-note{{uninitialized field 'this->d'}}
PointerToMemberDataTest1() {}
};
void fPointerToMemberDataTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberDataTest1(); // no-warning
+ PointerToMemberDataTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberDataTest2 {
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -389,15 +389,14 @@
continue;
}
-if (T->isMemberPointerType()) {
- if (isMemberPointerUninit(FR, LocalChain))
+if (T->isPointerType() || T->isReferenceType()) {
+ if (isPointerOrReferenceUninit(FR, LocalChain))
ContainsUninitField = true;
continue;
}
-// If this is a pointer or reference type.
-if (T->isPointerType() || T->isReferenceType()) {
- if (isPointerOrReferenceUninit(FR, LocalChain))
+if (T->isMemberPointerType()) {
+ if (isMemberPointerUninit(FR, LocalChain))
ContainsUninitField = true;
continue;
}
@@ -521,6 +520,12 @@
}
}
+if (T->isMemberPointerType()) {
+ if (isMemberPointerUninit(FR, LocalChain))
+return addFieldToUninits({LocalChain, FR, /*IsDereferenced*/ true});
+ return false;
+}
+
if (T->isArrayType()) {
IsAnyFieldInitialized = true;
return false;
@@ -543,7 +548,12 @@
FieldChainInfo LocalChain)
{
assert(FR->getDecl()->getType()->isMemberPointerType() &&
"This function only checks regions that hold MemberPointerTypes!");
- // TODO: Implement support for MemberPointerTypes.
+
+ SVal V = State->getSVal(FR);
+ if (V.isUndef())
+return addFieldToUninits({LocalChain, FR});
+
+ IsAnyFieldInitialized = true;
return false;
}
Index: test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
===
--- test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
+++ test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
@@ -416,14 +416,12 @@
#ifdef PEDANTIC
struct PointerToMemberFunctionTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- void (UsefulFunctions::*f)(void); // no-note
+ void (UsefulFunctions::*f)(void); // expected-note{{uninitialized field 'this->f'}}
PointerToMemberFunctionTest1() {}
};
void fPointerToMemberFunctionTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberFunctionTest1(); // no-warning
+ PointerToMemberFunctionTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberFunctionTest2 {
@@ -460,14 +458,12 @@
}
struct PointerToMemberDataTest1 {
- // TODO: we'd expect the note {{uninitialized field 'this->f'}}
- int UsefulFunctions::*d; // no-note
+ int UsefulFunctions::*d; // expected-note{{uninitialized field 'this->d'}}
PointerToMemberDataTest1() {}
};
void fPointerToMemberDataTest1() {
- // TODO: we'd expect the warning {{1 uninitialized field}}
- PointerToMemberDataTest1(); // no-warning
+ PointerToMemberDataTest1(); // expected-warning{{1 uninitialized field}}
}
struct PointerToMemberDataTest2 {
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++
