[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
This revision was automatically updated to reflect the committed changes.
Closed by commit rL342217: [analyzer][UninitializedObjectChecker] Correct
dynamic type is acquired for… (authored by Szelethus, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D50892?vs=163479=165447#toc
Repository:
rL LLVM
https://reviews.llvm.org/D50892
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
cfe/trunk/test/Analysis/cxx-uninitialized-object-inheritance.cpp
Index: cfe/trunk/test/Analysis/cxx-uninitialized-object-inheritance.cpp
===
--- cfe/trunk/test/Analysis/cxx-uninitialized-object-inheritance.cpp
+++ cfe/trunk/test/Analysis/cxx-uninitialized-object-inheritance.cpp
@@ -781,21 +781,53 @@
// Dynamic type test.
//===--===//
-struct DynTBase {};
-struct DynTDerived : DynTBase {
- // TODO: we'd expect the note: {{uninitialized field 'this->x'}}
- int x; // no-note
+struct DynTBase1 {};
+struct DynTDerived1 : DynTBase1 {
+ int y; // expected-note{{uninitialized field 'static_cast(this->bptr)->y'}}
};
-struct DynamicTypeTest {
- DynTBase *bptr;
+struct DynamicTypeTest1 {
+ DynTBase1 *bptr;
int i = 0;
- // TODO: we'd expect the warning: {{1 uninitialized field}}
- DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
+ DynamicTypeTest1(DynTBase1 *bptr) : bptr(bptr) {} // expected-warning{{1
uninitialized field}}
};
-void f() {
- DynTDerived d;
- DynamicTypeTest t();
+void fDynamicTypeTest1() {
+ DynTDerived1 d;
+ DynamicTypeTest1 t();
};
+
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'static_cast(this->bptr)->DynTBase2::x'}}
+};
+struct DynTDerived2 : DynTBase2 {
+ int y; // expected-note{{uninitialized field 'static_cast(this->bptr)->y'}}
+};
+
+struct DynamicTypeTest2 {
+ DynTBase2 *bptr;
+ int i = 0;
+
+ DynamicTypeTest2(DynTBase2 *bptr) : bptr(bptr) {} // expected-warning{{2
uninitialized fields}}
+};
+
+void fDynamicTypeTest2() {
+ DynTDerived2 d;
+ DynamicTypeTest2 t();
+}
+
+struct SymbolicSuperRegionBase {
+ SymbolicSuperRegionBase() {}
+};
+
+struct SymbolicSuperRegionDerived : SymbolicSuperRegionBase {
+ SymbolicSuperRegionBase *bptr; // no-crash
+ SymbolicSuperRegionDerived(SymbolicSuperRegionBase *bptr) : bptr(bptr) {}
+};
+
+SymbolicSuperRegionDerived *getSymbolicRegion();
+
+void fSymbolicSuperRegionTest() {
+ SymbolicSuperRegionDerived test(getSymbolicRegion());
+}
Index:
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
===
---
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
+++
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
@@ -234,5 +234,13 @@
break;
}
+ while (R->getAs()) {
+NeedsCastBack = true;
+
+if (!isa(R->getSuperRegion()))
+ break;
+R = R->getSuperRegion()->getAs();
+ }
+
return std::make_pair(R, NeedsCastBack);
}
Index: cfe/trunk/test/Analysis/cxx-uninitialized-object-inheritance.cpp
===
--- cfe/trunk/test/Analysis/cxx-uninitialized-object-inheritance.cpp
+++ cfe/trunk/test/Analysis/cxx-uninitialized-object-inheritance.cpp
@@ -781,21 +781,53 @@
// Dynamic type test.
//===--===//
-struct DynTBase {};
-struct DynTDerived : DynTBase {
- // TODO: we'd expect the note: {{uninitialized field 'this->x'}}
- int x; // no-note
+struct DynTBase1 {};
+struct DynTDerived1 : DynTBase1 {
+ int y; // expected-note{{uninitialized field 'static_cast(this->bptr)->y'}}
};
-struct DynamicTypeTest {
- DynTBase *bptr;
+struct DynamicTypeTest1 {
+ DynTBase1 *bptr;
int i = 0;
- // TODO: we'd expect the warning: {{1 uninitialized field}}
- DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
+ DynamicTypeTest1(DynTBase1 *bptr) : bptr(bptr) {} // expected-warning{{1 uninitialized field}}
};
-void f() {
- DynTDerived d;
- DynamicTypeTest t();
+void fDynamicTypeTest1() {
+ DynTDerived1 d;
+ DynamicTypeTest1 t();
};
+
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'static_cast(this->bptr)->DynTBase2::x'}}
+};
+struct DynTDerived2 : DynTBase2 {
+ int y; // expected-note{{uninitialized field 'static_cast(this->bptr)->y'}}
+};
+
+struct DynamicTypeTest2 {
+ DynTBase2 *bptr;
+ int i = 0;
+
+ DynamicTypeTest2(DynTBase2 *bptr) : bptr(bptr) {} // expected-warning{{2 uninitialized fields}}
+};
+
+void fDynamicTypeTest2() {
+ DynTDerived2 d;
+ DynamicTypeTest2 t();
+}
+
+struct SymbolicSuperRegionBase {
+ SymbolicSuperRegionBase() {}
+};
+
+struct SymbolicSuperRegionDerived : SymbolicSuperRegionBase {
+ SymbolicSuperRegionBase
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
Szelethus marked an inline comment as done.
Szelethus added inline comments.
Comment at: test/Analysis/cxx-uninitialized-object-inheritance.cpp:802
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'static_cast(this->bptr)->DynTBase2::x'}}
+};
Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > Mmm, what's the value of casting to derived type and then specifying
> > > > that we access the field of the base type anyway? Isn't `this->bptr->x`
> > > > exactly what the user needs to know(?)
> > > True, but it's a one tough job to write `this->bptr->x` here and also a
> > > correct note message for...
> > I guess don't try too hard, eg. say if it requires something of non-linear
> > complexity it's probably not worth it (not because it'd be slow but because
> > it'd be an indication that it might be not worth the effort).
> I actually invested some effort into this and I'm fairly certain I could pull
> it off with O(n) complexity, but I'll probably just place a TODO in the code
> for now, as I have other things I really want to get fixed first.
I think I'll leave this as is, it could be done, but it wouldn't be nice, and
it wouldn't make anyone's life significantly easier. At least this report tells
you the dynamic type of `this->bptr`, so I guess that's something.
https://reviews.llvm.org/D50892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
Szelethus updated this revision to Diff 163479.
Szelethus added a comment.
Fixed a crash, where the super region was symbolic.
In https://reviews.llvm.org/D50892#1218060, @NoQ wrote:
> Let's commit then?
I'd be much more comfortable landing https://reviews.llvm.org/D51057 before
modifying dereferencing any further.
https://reviews.llvm.org/D50892
Files:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
test/Analysis/cxx-uninitialized-object-inheritance.cpp
Index: test/Analysis/cxx-uninitialized-object-inheritance.cpp
===
--- test/Analysis/cxx-uninitialized-object-inheritance.cpp
+++ test/Analysis/cxx-uninitialized-object-inheritance.cpp
@@ -781,21 +781,53 @@
// Dynamic type test.
//===--===//
-struct DynTBase {};
-struct DynTDerived : DynTBase {
- // TODO: we'd expect the note: {{uninitialized field 'this->x'}}
- int x; // no-note
+struct DynTBase1 {};
+struct DynTDerived1 : DynTBase1 {
+ int y; // expected-note{{uninitialized field 'static_cast(this->bptr)->y'}}
};
-struct DynamicTypeTest {
- DynTBase *bptr;
+struct DynamicTypeTest1 {
+ DynTBase1 *bptr;
int i = 0;
- // TODO: we'd expect the warning: {{1 uninitialized field}}
- DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
+ DynamicTypeTest1(DynTBase1 *bptr) : bptr(bptr) {} // expected-warning{{1
uninitialized field}}
};
-void f() {
- DynTDerived d;
- DynamicTypeTest t();
+void fDynamicTypeTest1() {
+ DynTDerived1 d;
+ DynamicTypeTest1 t();
};
+
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'static_cast(this->bptr)->DynTBase2::x'}}
+};
+struct DynTDerived2 : DynTBase2 {
+ int y; // expected-note{{uninitialized field 'static_cast(this->bptr)->y'}}
+};
+
+struct DynamicTypeTest2 {
+ DynTBase2 *bptr;
+ int i = 0;
+
+ DynamicTypeTest2(DynTBase2 *bptr) : bptr(bptr) {} // expected-warning{{2
uninitialized fields}}
+};
+
+void fDynamicTypeTest2() {
+ DynTDerived2 d;
+ DynamicTypeTest2 t();
+}
+
+struct SymbolicSuperRegionBase {
+ SymbolicSuperRegionBase() {}
+};
+
+struct SymbolicSuperRegionDerived : SymbolicSuperRegionBase {
+ SymbolicSuperRegionBase *bptr; // no-crash
+ SymbolicSuperRegionDerived(SymbolicSuperRegionBase *bptr) : bptr(bptr) {}
+};
+
+SymbolicSuperRegionDerived *getSymbolicRegion();
+
+void fSymbolicSuperRegionTest() {
+ SymbolicSuperRegionDerived test(getSymbolicRegion());
+}
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
@@ -240,5 +240,13 @@
break;
}
+ while (R->getAs()) {
+NeedsCastBack = true;
+
+if (!isa(R->getSuperRegion()))
+ break;
+R = R->getSuperRegion()->getAs();
+ }
+
return std::make_pair(R, NeedsCastBack);
}
Index: test/Analysis/cxx-uninitialized-object-inheritance.cpp
===
--- test/Analysis/cxx-uninitialized-object-inheritance.cpp
+++ test/Analysis/cxx-uninitialized-object-inheritance.cpp
@@ -781,21 +781,53 @@
// Dynamic type test.
//===--===//
-struct DynTBase {};
-struct DynTDerived : DynTBase {
- // TODO: we'd expect the note: {{uninitialized field 'this->x'}}
- int x; // no-note
+struct DynTBase1 {};
+struct DynTDerived1 : DynTBase1 {
+ int y; // expected-note{{uninitialized field 'static_cast(this->bptr)->y'}}
};
-struct DynamicTypeTest {
- DynTBase *bptr;
+struct DynamicTypeTest1 {
+ DynTBase1 *bptr;
int i = 0;
- // TODO: we'd expect the warning: {{1 uninitialized field}}
- DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
+ DynamicTypeTest1(DynTBase1 *bptr) : bptr(bptr) {} // expected-warning{{1 uninitialized field}}
};
-void f() {
- DynTDerived d;
- DynamicTypeTest t();
+void fDynamicTypeTest1() {
+ DynTDerived1 d;
+ DynamicTypeTest1 t();
};
+
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'static_cast(this->bptr)->DynTBase2::x'}}
+};
+struct DynTDerived2 : DynTBase2 {
+ int y; // expected-note{{uninitialized field 'static_cast(this->bptr)->y'}}
+};
+
+struct DynamicTypeTest2 {
+ DynTBase2 *bptr;
+ int i = 0;
+
+ DynamicTypeTest2(DynTBase2 *bptr) : bptr(bptr) {} // expected-warning{{2 uninitialized fields}}
+};
+
+void fDynamicTypeTest2() {
+ DynTDerived2 d;
+ DynamicTypeTest2 t();
+}
+
+struct SymbolicSuperRegionBase {
+ SymbolicSuperRegionBase() {}
+};
+
+struct SymbolicSuperRegionDerived : SymbolicSuperRegionBase {
+ SymbolicSuperRegionBase *bptr; // no-crash
+ SymbolicSuperRegionDerived(SymbolicSuperRegionBase *bptr) : bptr(bptr) {}
+};
+
+SymbolicSuperRegionDerived *getSymbolicRegion();
+
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
NoQ accepted this revision. NoQ added a comment. Let's commit then? https://reviews.llvm.org/D50892 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
Szelethus added inline comments.
Comment at: test/Analysis/cxx-uninitialized-object-inheritance.cpp:802
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'static_cast(this->bptr)->DynTBase2::x'}}
+};
NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Mmm, what's the value of casting to derived type and then specifying that
> > > we access the field of the base type anyway? Isn't `this->bptr->x`
> > > exactly what the user needs to know(?)
> > True, but it's a one tough job to write `this->bptr->x` here and also a
> > correct note message for...
> I guess don't try too hard, eg. say if it requires something of non-linear
> complexity it's probably not worth it (not because it'd be slow but because
> it'd be an indication that it might be not worth the effort).
I actually invested some effort into this and I'm fairly certain I could pull
it off with O(n) complexity, but I'll probably just place a TODO in the code
for now, as I have other things I really want to get fixed first.
https://reviews.llvm.org/D50892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
NoQ added inline comments.
Comment at: test/Analysis/cxx-uninitialized-object-inheritance.cpp:802
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'static_cast(this->bptr)->DynTBase2::x'}}
+};
Szelethus wrote:
> NoQ wrote:
> > Mmm, what's the value of casting to derived type and then specifying that
> > we access the field of the base type anyway? Isn't `this->bptr->x` exactly
> > what the user needs to know(?)
> True, but it's a one tough job to write `this->bptr->x` here and also a
> correct note message for...
I guess don't try too hard, eg. say if it requires something of non-linear
complexity it's probably not worth it (not because it'd be slow but because
it'd be an indication that it might be not worth the effort).
https://reviews.llvm.org/D50892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
Szelethus added inline comments.
Comment at: test/Analysis/cxx-uninitialized-object-inheritance.cpp:802
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'static_cast(this->bptr)->DynTBase2::x'}}
+};
NoQ wrote:
> Mmm, what's the value of casting to derived type and then specifying that we
> access the field of the base type anyway? Isn't `this->bptr->x` exactly what
> the user needs to know(?)
True, but it's a one tough job to write `this->bptr->x` here and also a correct
note message for...
Comment at: test/Analysis/cxx-uninitialized-object-inheritance.cpp:805
+struct DynTDerived2 : DynTBase2 {
+ int y; // expected-note{{uninitialized field 'static_cast(this->bptr)->y'}}
+};
...this one. I'll see how I could achieve it without disrupting `FieldNode`'s
interface too much.
https://reviews.llvm.org/D50892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
NoQ added inline comments.
Comment at: test/Analysis/cxx-uninitialized-object-inheritance.cpp:802
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'static_cast(this->bptr)->DynTBase2::x'}}
+};
Mmm, what's the value of casting to derived type and then specifying that we
access the field of the base type anyway? Isn't `this->bptr->x` exactly what
the user needs to know(?)
https://reviews.llvm.org/D50892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
Szelethus updated this revision to Diff 162658.
Szelethus marked 5 inline comments as done.
Szelethus added a comment.
Actually, the note messages were incorrect as in this case the static and
dynamic type of the object differs.
https://reviews.llvm.org/D50892
Files:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
test/Analysis/cxx-uninitialized-object-inheritance.cpp
Index: test/Analysis/cxx-uninitialized-object-inheritance.cpp
===
--- test/Analysis/cxx-uninitialized-object-inheritance.cpp
+++ test/Analysis/cxx-uninitialized-object-inheritance.cpp
@@ -781,21 +781,38 @@
// Dynamic type test.
//===--===//
-struct DynTBase {};
-struct DynTDerived : DynTBase {
- // TODO: we'd expect the note: {{uninitialized field 'this->x'}}
- int x; // no-note
+struct DynTBase1 {};
+struct DynTDerived1 : DynTBase1 {
+ int y; // expected-note{{uninitialized field 'static_cast(this->bptr)->y'}}
};
-struct DynamicTypeTest {
- DynTBase *bptr;
+struct DynamicTypeTest1 {
+ DynTBase1 *bptr;
int i = 0;
- // TODO: we'd expect the warning: {{1 uninitialized field}}
- DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
+ DynamicTypeTest1(DynTBase1 *bptr) : bptr(bptr) {} // expected-warning{{1
uninitialized field}}
};
-void f() {
- DynTDerived d;
- DynamicTypeTest t();
+void fDynamicTypeTest1() {
+ DynTDerived1 d;
+ DynamicTypeTest1 t();
};
+
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'static_cast(this->bptr)->DynTBase2::x'}}
+};
+struct DynTDerived2 : DynTBase2 {
+ int y; // expected-note{{uninitialized field 'static_cast(this->bptr)->y'}}
+};
+
+struct DynamicTypeTest2 {
+ DynTBase2 *bptr;
+ int i = 0;
+
+ DynamicTypeTest2(DynTBase2 *bptr) : bptr(bptr) {} // expected-warning{{2
uninitialized fields}}
+};
+
+void fDynamicTypeTest2() {
+ DynTDerived2 d;
+ DynamicTypeTest2 t();
+}
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
@@ -240,5 +240,10 @@
break;
}
+ while (R->getAs()) {
+NeedsCastBack = true;
+R = R->getSuperRegion()->getAs();
+ }
+
return std::make_pair(R, NeedsCastBack);
}
Index: test/Analysis/cxx-uninitialized-object-inheritance.cpp
===
--- test/Analysis/cxx-uninitialized-object-inheritance.cpp
+++ test/Analysis/cxx-uninitialized-object-inheritance.cpp
@@ -781,21 +781,38 @@
// Dynamic type test.
//===--===//
-struct DynTBase {};
-struct DynTDerived : DynTBase {
- // TODO: we'd expect the note: {{uninitialized field 'this->x'}}
- int x; // no-note
+struct DynTBase1 {};
+struct DynTDerived1 : DynTBase1 {
+ int y; // expected-note{{uninitialized field 'static_cast(this->bptr)->y'}}
};
-struct DynamicTypeTest {
- DynTBase *bptr;
+struct DynamicTypeTest1 {
+ DynTBase1 *bptr;
int i = 0;
- // TODO: we'd expect the warning: {{1 uninitialized field}}
- DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
+ DynamicTypeTest1(DynTBase1 *bptr) : bptr(bptr) {} // expected-warning{{1 uninitialized field}}
};
-void f() {
- DynTDerived d;
- DynamicTypeTest t();
+void fDynamicTypeTest1() {
+ DynTDerived1 d;
+ DynamicTypeTest1 t();
};
+
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'static_cast(this->bptr)->DynTBase2::x'}}
+};
+struct DynTDerived2 : DynTBase2 {
+ int y; // expected-note{{uninitialized field 'static_cast(this->bptr)->y'}}
+};
+
+struct DynamicTypeTest2 {
+ DynTBase2 *bptr;
+ int i = 0;
+
+ DynamicTypeTest2(DynTBase2 *bptr) : bptr(bptr) {} // expected-warning{{2 uninitialized fields}}
+};
+
+void fDynamicTypeTest2() {
+ DynTDerived2 d;
+ DynamicTypeTest2 t();
+}
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
@@ -240,5 +240,10 @@
break;
}
+ while (R->getAs()) {
+NeedsCastBack = true;
+R = R->getSuperRegion()->getAs();
+ }
+
return std::make_pair(R, NeedsCastBack);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
NoQ accepted this revision.
NoQ added inline comments.
This revision is now accepted and ready to land.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:187-191
// If FR is a pointer pointing to a non-primitive type.
if (Optional RecordV =
DerefdV.getAs()) {
const TypedValueRegion *R = RecordV->getRegion();
Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > This looks like one more situation where we dereference a location to
> > > > get a value and then struggle to get back to the location that we've
> > > > dereferenced by looking at the value. Can we just use `V`?
> > > I've struggled with derefencing for months now -- I'm afraid I just don't
> > > really get what you'd like to see here.
> > >
> > > Here's what I attempted to implement:
> > > I'd like to obtain the pointee's region of a `Loc` region, even if it has
> > > to be casted to another type, like through void pointers and
> > > `nonloc::LocAsInteger`, and continue analysis on said region as usual.
> > >
> > > The trickiest part I can't seem to get right is the acquisition of the
> > > pointee region. For the problem this patch attempts to solve, even though
> > > `DynT` correctly says that the dynamic type is `DynTDerived2 *`,
> > > `DerefdV` contains a region for `DynTBase`.
> > >
> > > I uploaded a new patch, D51057, which hopefully settles derefence related
> > > issues. Please note that it **does not **replace this diff, as the
> > > acquired region is still of type `DynTBase`.
> > >
> > > I find understanding these intricate details of the analyzer very
> > > difficult, as I found very little documentation about how this works,
> > > which often left me guessing what the proper way to do this is. Can you
> > > recommend some literature for me on this field?
> > > Can you recommend some literature for me on this field?
> >
> > This is pretty specific to our analyzer. `SVal`/`SymExpr`/`MemRegion`
> > hierarchy is tightly coupled to implementation details of the
> > `RegionStore`, which is our memory model. There's a paper on it [1]. We
> > have some in-tree documentation of the `RegionStore` [2] (other docs there
> > are also interesting to read). And there's my old workbook [3]. And i guess
> > that's it.
> >
> > [1] Xu, Zhongxing & Kremenek, Ted & Zhang, Jian. (2010). A Memory Model for
> > Static Analysis of C Programs. 535-548. 10.1007/978-3-642-16558-0_44.
> > [2]
> > https://github.com/llvm-mirror/clang/blob/master/docs/analyzer/RegionStore.txt
> > [3]
> > https://github.com/haoNoQ/clang-analyzer-guide/releases/download/v0.1/clang-analyzer-guide-v0.1.pdf
> Thank you! I'm aware of these works, and have read them already.
>
> The actual implementation of the analyzer is most described in your book, and
> I've often got the best ideas from that. However, some very well described
> things in there have absolutely no documentation in the actual code -- for
> example, it isn't obvious at all to me what `CompundValue` is, and I was
> surprised to learn what it does from your guide. Other examples are the
> difference between `TypedValueRegion`s `getLocationType` and `getValueType`,
> `SymExpr` in general, and so on.
>
> I think it would also be very valuable to have a link in `docs/analyzer` to
> your book. On another note, would you mind if I were to put some of the
> information from there into the actual code?
I guess a link on the website would be good. We should also convert a lot of
our comments to doxygen comments, because some people do read doxygen
(including myself).
I don't advice copy-paste-ing texts from the workbook to the code because i
picked a weird licensing scheme (no idea why i picked a -BY license) and wasn't
straight enough about authorship (so i don't feel i can change it anymore). But
actually documenting things that are currently not documented is something i'd
greatly appreciate :)
https://reviews.llvm.org/D50892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
Szelethus added inline comments.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:187-191
// If FR is a pointer pointing to a non-primitive type.
if (Optional RecordV =
DerefdV.getAs()) {
const TypedValueRegion *R = RecordV->getRegion();
NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > This looks like one more situation where we dereference a location to get
> > > a value and then struggle to get back to the location that we've
> > > dereferenced by looking at the value. Can we just use `V`?
> > I've struggled with derefencing for months now -- I'm afraid I just don't
> > really get what you'd like to see here.
> >
> > Here's what I attempted to implement:
> > I'd like to obtain the pointee's region of a `Loc` region, even if it has
> > to be casted to another type, like through void pointers and
> > `nonloc::LocAsInteger`, and continue analysis on said region as usual.
> >
> > The trickiest part I can't seem to get right is the acquisition of the
> > pointee region. For the problem this patch attempts to solve, even though
> > `DynT` correctly says that the dynamic type is `DynTDerived2 *`, `DerefdV`
> > contains a region for `DynTBase`.
> >
> > I uploaded a new patch, D51057, which hopefully settles derefence related
> > issues. Please note that it **does not **replace this diff, as the acquired
> > region is still of type `DynTBase`.
> >
> > I find understanding these intricate details of the analyzer very
> > difficult, as I found very little documentation about how this works, which
> > often left me guessing what the proper way to do this is. Can you recommend
> > some literature for me on this field?
> > Can you recommend some literature for me on this field?
>
> This is pretty specific to our analyzer. `SVal`/`SymExpr`/`MemRegion`
> hierarchy is tightly coupled to implementation details of the `RegionStore`,
> which is our memory model. There's a paper on it [1]. We have some in-tree
> documentation of the `RegionStore` [2] (other docs there are also interesting
> to read). And there's my old workbook [3]. And i guess that's it.
>
> [1] Xu, Zhongxing & Kremenek, Ted & Zhang, Jian. (2010). A Memory Model for
> Static Analysis of C Programs. 535-548. 10.1007/978-3-642-16558-0_44.
> [2]
> https://github.com/llvm-mirror/clang/blob/master/docs/analyzer/RegionStore.txt
> [3]
> https://github.com/haoNoQ/clang-analyzer-guide/releases/download/v0.1/clang-analyzer-guide-v0.1.pdf
Thank you! I'm aware of these works, and have read them already.
The actual implementation of the analyzer is most described in your book, and
I've often got the best ideas from that. However, some very well described
things in there have absolutely no documentation in the actual code -- for
example, it isn't obvious at all to me what `CompundValue` is, and I was
surprised to learn what it does from your guide. Other examples are the
difference between `TypedValueRegion`s `getLocationType` and `getValueType`,
`SymExpr` in general, and so on.
I think it would also be very valuable to have a link in `docs/analyzer` to
your book. On another note, would you mind if I were to put some of the
information from there into the actual code?
Comment at: test/Analysis/cxx-uninitialized-object-inheritance.cpp:787
// TODO: we'd expect the note: {{uninitialized field 'this->x'}}
int x; // no-note
};
NoQ wrote:
> Szelethus wrote:
> > The checker should be able to catch this one -- for some reason it is
> > regarded as an unknown region. Odd, as the test case right after this one
> > works perfectly.
> There's a variety of problems we have with empty base classes, might be one
> of those, and they are usually easy to fix because, well, yes it's a special
> case, but it's also an extremely simple case.
>
> I encourage you to open up the Exploded Graph and study it carefully to see
> what and where goes wrong (not for this revision).
I'd love to learn about other parts of the analyzer (besides chasing pointers),
but for now, this seems to have fixed itself ;)
https://reviews.llvm.org/D50892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
Szelethus updated this revision to Diff 162205.
Szelethus added a comment.
Rebased to https://reviews.llvm.org/D51057.
https://reviews.llvm.org/D50892
Files:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
test/Analysis/cxx-uninitialized-object-inheritance.cpp
Index: test/Analysis/cxx-uninitialized-object-inheritance.cpp
===
--- test/Analysis/cxx-uninitialized-object-inheritance.cpp
+++ test/Analysis/cxx-uninitialized-object-inheritance.cpp
@@ -781,21 +781,38 @@
// Dynamic type test.
//===--===//
-struct DynTBase {};
-struct DynTDerived : DynTBase {
- // TODO: we'd expect the note: {{uninitialized field 'this->x'}}
- int x; // no-note
+struct DynTBase1 {};
+struct DynTDerived1 : DynTBase1 {
+ int y; // expected-note{{uninitialized field 'this->bptr->y'}}
};
-struct DynamicTypeTest {
- DynTBase *bptr;
+struct DynamicTypeTest1 {
+ DynTBase1 *bptr;
int i = 0;
- // TODO: we'd expect the warning: {{1 uninitialized field}}
- DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
+ DynamicTypeTest1(DynTBase1 *bptr) : bptr(bptr) {} // expected-warning{{1
uninitialized field}}
};
-void f() {
- DynTDerived d;
- DynamicTypeTest t();
+void fDynamicTypeTest1() {
+ DynTDerived1 d;
+ DynamicTypeTest1 t();
};
+
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'this->bptr->DynTBase2::x'}}
+};
+struct DynTDerived2 : DynTBase2 {
+ int y; // expected-note{{uninitialized field 'this->bptr->y'}}
+};
+
+struct DynamicTypeTest2 {
+ DynTBase2 *bptr;
+ int i = 0;
+
+ DynamicTypeTest2(DynTBase2 *bptr) : bptr(bptr) {} // expected-warning{{2
uninitialized fields}}
+};
+
+void fDynamicTypeTest2() {
+ DynTDerived2 d;
+ DynamicTypeTest2 t();
+}
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
@@ -240,5 +240,9 @@
break;
}
+ while (R->getAs()) {
+R = R->getSuperRegion()->getAs();
+ }
+
return std::make_pair(R, NeedsCastBack);
}
Index: test/Analysis/cxx-uninitialized-object-inheritance.cpp
===
--- test/Analysis/cxx-uninitialized-object-inheritance.cpp
+++ test/Analysis/cxx-uninitialized-object-inheritance.cpp
@@ -781,21 +781,38 @@
// Dynamic type test.
//===--===//
-struct DynTBase {};
-struct DynTDerived : DynTBase {
- // TODO: we'd expect the note: {{uninitialized field 'this->x'}}
- int x; // no-note
+struct DynTBase1 {};
+struct DynTDerived1 : DynTBase1 {
+ int y; // expected-note{{uninitialized field 'this->bptr->y'}}
};
-struct DynamicTypeTest {
- DynTBase *bptr;
+struct DynamicTypeTest1 {
+ DynTBase1 *bptr;
int i = 0;
- // TODO: we'd expect the warning: {{1 uninitialized field}}
- DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
+ DynamicTypeTest1(DynTBase1 *bptr) : bptr(bptr) {} // expected-warning{{1 uninitialized field}}
};
-void f() {
- DynTDerived d;
- DynamicTypeTest t();
+void fDynamicTypeTest1() {
+ DynTDerived1 d;
+ DynamicTypeTest1 t();
};
+
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'this->bptr->DynTBase2::x'}}
+};
+struct DynTDerived2 : DynTBase2 {
+ int y; // expected-note{{uninitialized field 'this->bptr->y'}}
+};
+
+struct DynamicTypeTest2 {
+ DynTBase2 *bptr;
+ int i = 0;
+
+ DynamicTypeTest2(DynTBase2 *bptr) : bptr(bptr) {} // expected-warning{{2 uninitialized fields}}
+};
+
+void fDynamicTypeTest2() {
+ DynTDerived2 d;
+ DynamicTypeTest2 t();
+}
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
@@ -240,5 +240,9 @@
break;
}
+ while (R->getAs()) {
+R = R->getSuperRegion()->getAs();
+ }
+
return std::make_pair(R, NeedsCastBack);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
NoQ added inline comments.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:187-191
// If FR is a pointer pointing to a non-primitive type.
if (Optional RecordV =
DerefdV.getAs()) {
const TypedValueRegion *R = RecordV->getRegion();
Szelethus wrote:
> NoQ wrote:
> > This looks like one more situation where we dereference a location to get a
> > value and then struggle to get back to the location that we've dereferenced
> > by looking at the value. Can we just use `V`?
> I've struggled with derefencing for months now -- I'm afraid I just don't
> really get what you'd like to see here.
>
> Here's what I attempted to implement:
> I'd like to obtain the pointee's region of a `Loc` region, even if it has to
> be casted to another type, like through void pointers and
> `nonloc::LocAsInteger`, and continue analysis on said region as usual.
>
> The trickiest part I can't seem to get right is the acquisition of the
> pointee region. For the problem this patch attempts to solve, even though
> `DynT` correctly says that the dynamic type is `DynTDerived2 *`, `DerefdV`
> contains a region for `DynTBase`.
>
> I uploaded a new patch, D51057, which hopefully settles derefence related
> issues. Please note that it **does not **replace this diff, as the acquired
> region is still of type `DynTBase`.
>
> I find understanding these intricate details of the analyzer very difficult,
> as I found very little documentation about how this works, which often left
> me guessing what the proper way to do this is. Can you recommend some
> literature for me on this field?
> Can you recommend some literature for me on this field?
This is pretty specific to our analyzer. `SVal`/`SymExpr`/`MemRegion` hierarchy
is tightly coupled to implementation details of the `RegionStore`, which is our
memory model. There's a paper on it [1]. We have some in-tree documentation of
the `RegionStore` [2] (other docs there are also interesting to read). And
there's my old workbook [3]. And i guess that's it.
[1] Xu, Zhongxing & Kremenek, Ted & Zhang, Jian. (2010). A Memory Model for
Static Analysis of C Programs. 535-548. 10.1007/978-3-642-16558-0_44.
[2]
https://github.com/llvm-mirror/clang/blob/master/docs/analyzer/RegionStore.txt
[3]
https://github.com/haoNoQ/clang-analyzer-guide/releases/download/v0.1/clang-analyzer-guide-v0.1.pdf
Repository:
rC Clang
https://reviews.llvm.org/D50892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
Szelethus added inline comments.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:187-191
// If FR is a pointer pointing to a non-primitive type.
if (Optional RecordV =
DerefdV.getAs()) {
const TypedValueRegion *R = RecordV->getRegion();
NoQ wrote:
> This looks like one more situation where we dereference a location to get a
> value and then struggle to get back to the location that we've dereferenced
> by looking at the value. Can we just use `V`?
I've struggled with derefencing for months now -- I'm afraid I just don't
really get what you'd like to see here.
Here's what I attempted to implement:
I'd like to obtain the pointee's region of a `Loc` region, even if it has to be
casted to another type, like through void pointers and `nonloc::LocAsInteger`,
and continue analysis on said region as usual.
The trickiest part I can't seem to get right is the acquisition of the pointee
region. For the problem this patch attempts to solve, even though `DynT`
correctly says that the dynamic type is `DynTDerived2 *`, `DerefdV` contains a
region for `DynTBase`.
I uploaded a new patch, D51057, which hopefully settles derefence related
issues. Please note that it **does not **replace this diff, as the acquired
region is still of type `DynTBase`.
I find understanding these intricate details of the analyzer very difficult, as
I found very little documentation about how this works, which often left me
guessing what the proper way to do this is. Can you recommend some literature
for me on this field?
Repository:
rC Clang
https://reviews.llvm.org/D50892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
NoQ added inline comments.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:187-191
// If FR is a pointer pointing to a non-primitive type.
if (Optional RecordV =
DerefdV.getAs()) {
const TypedValueRegion *R = RecordV->getRegion();
This looks like one more situation where we dereference a location to get a
value and then struggle to get back to the location that we've dereferenced by
looking at the value. Can we just use `V`?
Comment at: test/Analysis/cxx-uninitialized-object-inheritance.cpp:787
// TODO: we'd expect the note: {{uninitialized field 'this->x'}}
int x; // no-note
};
Szelethus wrote:
> The checker should be able to catch this one -- for some reason it is
> regarded as an unknown region. Odd, as the test case right after this one
> works perfectly.
There's a variety of problems we have with empty base classes, might be one of
those, and they are usually easy to fix because, well, yes it's a special case,
but it's also an extremely simple case.
I encourage you to open up the Exploded Graph and study it carefully to see
what and where goes wrong (not for this revision).
Repository:
rC Clang
https://reviews.llvm.org/D50892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
Szelethus added inline comments.
Comment at: test/Analysis/cxx-uninitialized-object-inheritance.cpp:787
// TODO: we'd expect the note: {{uninitialized field 'this->x'}}
int x; // no-note
};
The checker should be able to catch this one -- for some reason it is regarded
as an unknown region. Odd, as the test case right after this one works
perfectly.
Repository:
rC Clang
https://reviews.llvm.org/D50892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
Szelethus created this revision.
Szelethus added reviewers: george.karpenkov, NoQ, xazax.hun, rnkovacs.
Herald added subscribers: cfe-commits, mikhail.ramalho, a.sidorin, szepet,
whisperity.
Repository:
rC Clang
https://reviews.llvm.org/D50892
Files:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
test/Analysis/cxx-uninitialized-object-inheritance.cpp
Index: test/Analysis/cxx-uninitialized-object-inheritance.cpp
===
--- test/Analysis/cxx-uninitialized-object-inheritance.cpp
+++ test/Analysis/cxx-uninitialized-object-inheritance.cpp
@@ -781,21 +781,40 @@
// Dynamic type test.
//===--===//
-struct DynTBase {};
-struct DynTDerived : DynTBase {
+struct DynTBase1 {};
+struct DynTDerived1 : DynTBase1 {
// TODO: we'd expect the note: {{uninitialized field 'this->x'}}
int x; // no-note
};
-struct DynamicTypeTest {
- DynTBase *bptr;
+struct DynamicTypeTest1 {
+ DynTBase1 *bptr;
int i = 0;
// TODO: we'd expect the warning: {{1 uninitialized field}}
- DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
+ DynamicTypeTest1(DynTBase1 *bptr) : bptr(bptr) {} // no-warning
};
-void f() {
- DynTDerived d;
- DynamicTypeTest t();
+void fDynamicTypeTest1() {
+ DynTDerived1 d;
+ DynamicTypeTest1 t();
};
+
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'this->bptr->x'}}
+};
+struct DynTDerived2 : DynTBase2 {
+ int y; // expected-note{{uninitialized field 'this->bptr->y'}}
+};
+
+struct DynamicTypeTest2 {
+ DynTBase2 *bptr;
+ int i = 0;
+
+ DynamicTypeTest2(DynTBase2 *bptr) : bptr(bptr) {} // expected-warning{{2
uninitialized fields}}
+};
+
+void fDynamicTypeTest2() {
+ DynTDerived2 d;
+ DynamicTypeTest2 t();
+}
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
@@ -190,6 +190,10 @@
const TypedValueRegion *R = RecordV->getRegion();
+while (R->getAs()) {
+ R = R->getSuperRegion()->getAs();
+}
+
if (DynT->getPointeeType()->isStructureOrClassType()) {
if (NeedsCastBack)
return isNonUnionUninit(R, LocalChain.add(NeedsCastLocField(FR,
DynT)));
Index: test/Analysis/cxx-uninitialized-object-inheritance.cpp
===
--- test/Analysis/cxx-uninitialized-object-inheritance.cpp
+++ test/Analysis/cxx-uninitialized-object-inheritance.cpp
@@ -781,21 +781,40 @@
// Dynamic type test.
//===--===//
-struct DynTBase {};
-struct DynTDerived : DynTBase {
+struct DynTBase1 {};
+struct DynTDerived1 : DynTBase1 {
// TODO: we'd expect the note: {{uninitialized field 'this->x'}}
int x; // no-note
};
-struct DynamicTypeTest {
- DynTBase *bptr;
+struct DynamicTypeTest1 {
+ DynTBase1 *bptr;
int i = 0;
// TODO: we'd expect the warning: {{1 uninitialized field}}
- DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
+ DynamicTypeTest1(DynTBase1 *bptr) : bptr(bptr) {} // no-warning
};
-void f() {
- DynTDerived d;
- DynamicTypeTest t();
+void fDynamicTypeTest1() {
+ DynTDerived1 d;
+ DynamicTypeTest1 t();
};
+
+struct DynTBase2 {
+ int x; // expected-note{{uninitialized field 'this->bptr->x'}}
+};
+struct DynTDerived2 : DynTBase2 {
+ int y; // expected-note{{uninitialized field 'this->bptr->y'}}
+};
+
+struct DynamicTypeTest2 {
+ DynTBase2 *bptr;
+ int i = 0;
+
+ DynamicTypeTest2(DynTBase2 *bptr) : bptr(bptr) {} // expected-warning{{2 uninitialized fields}}
+};
+
+void fDynamicTypeTest2() {
+ DynTDerived2 d;
+ DynamicTypeTest2 t();
+}
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
@@ -190,6 +190,10 @@
const TypedValueRegion *R = RecordV->getRegion();
+while (R->getAs()) {
+ R = R->getSuperRegion()->getAs();
+}
+
if (DynT->getPointeeType()->isStructureOrClassType()) {
if (NeedsCastBack)
return isNonUnionUninit(R, LocalChain.add(NeedsCastLocField(FR, DynT)));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
