[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
This revision was automatically updated to reflect the committed changes.
Closed by commit rL297187: [analyzer] Fix crashes in CastToStruct checker for
undefined structs (authored by danielmarjamaki).
Changed prior to commit:
https://reviews.llvm.org/D28297?vs=89507=90900#toc
Repository:
rL LLVM
https://reviews.llvm.org/D28297
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
cfe/trunk/test/Analysis/cast-to-struct.cpp
Index: cfe/trunk/test/Analysis/cast-to-struct.cpp
===
--- cfe/trunk/test/Analysis/cast-to-struct.cpp
+++ cfe/trunk/test/Analysis/cast-to-struct.cpp
@@ -65,3 +65,17 @@
void *VP = P;
Abc = (struct ABC *)VP;
}
+
+// https://llvm.org/bugs/show_bug.cgi?id=31173
+void dontCrash1(struct AB X) {
+ struct UndefS *S = (struct UndefS *)
+}
+
+struct S;
+struct T {
+ struct S *P;
+};
+extern struct S Var1, Var2;
+void dontCrash2() {
+ ((struct T *) )->P =
+}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
@@ -84,6 +84,10 @@
if (!VD || VD->getType()->isReferenceType())
return true;
+if (ToPointeeTy->isIncompleteType() ||
+OrigPointeeTy->isIncompleteType())
+ return true;
+
// Warn when there is widening cast.
unsigned ToWidth = Ctx.getTypeInfo(ToPointeeTy).Width;
unsigned OrigWidth = Ctx.getTypeInfo(OrigPointeeTy).Width;
Index: cfe/trunk/test/Analysis/cast-to-struct.cpp
===
--- cfe/trunk/test/Analysis/cast-to-struct.cpp
+++ cfe/trunk/test/Analysis/cast-to-struct.cpp
@@ -65,3 +65,17 @@
void *VP = P;
Abc = (struct ABC *)VP;
}
+
+// https://llvm.org/bugs/show_bug.cgi?id=31173
+void dontCrash1(struct AB X) {
+ struct UndefS *S = (struct UndefS *)
+}
+
+struct S;
+struct T {
+ struct S *P;
+};
+extern struct S Var1, Var2;
+void dontCrash2() {
+ ((struct T *) )->P =
+}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
@@ -84,6 +84,10 @@
if (!VD || VD->getType()->isReferenceType())
return true;
+if (ToPointeeTy->isIncompleteType() ||
+OrigPointeeTy->isIncompleteType())
+ return true;
+
// Warn when there is widening cast.
unsigned ToWidth = Ctx.getTypeInfo(ToPointeeTy).Width;
unsigned OrigWidth = Ctx.getTypeInfo(OrigPointeeTy).Width;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Thanks. looks good. https://reviews.llvm.org/D28297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
a.sidorin added a comment. Looks like a right fix. Comment at: lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp:93 // Warn when there is widening cast. unsigned ToWidth = Ctx.getTypeInfo(ToPointeeTy).Width; NoQ wrote: > I think we should move the check here then. That'd avoid double-checking if > `ToPointeeTy` is a record type (we could `cast<>` directly on this branch). Or just hoist it out of condition? https://reviews.llvm.org/D28297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
danielmarjamaki requested review of this revision. danielmarjamaki added a comment. I have updated the patch and want a new review. https://reviews.llvm.org/D28297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
danielmarjamaki updated this revision to Diff 89507.
danielmarjamaki added a comment.
It was reported in the bugzilla report that my first fix did not fix all
crashes. A new example code was provided that triggered a new crash. I have
updated the patch so both crashes are fixed.
https://reviews.llvm.org/D28297
Files:
lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
test/Analysis/cast-to-struct.cpp
Index: test/Analysis/cast-to-struct.cpp
===
--- test/Analysis/cast-to-struct.cpp
+++ test/Analysis/cast-to-struct.cpp
@@ -65,3 +65,17 @@
void *VP = P;
Abc = (struct ABC *)VP;
}
+
+// https://llvm.org/bugs/show_bug.cgi?id=31173
+void dontCrash1(struct AB X) {
+ struct UndefS *S = (struct UndefS *)
+}
+
+struct S;
+struct T {
+ struct S *P;
+};
+extern struct S Var1, Var2;
+void dontCrash2() {
+ ((struct T *) )->P =
+}
Index: lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
@@ -84,6 +84,10 @@
if (!VD || VD->getType()->isReferenceType())
return true;
+if (ToPointeeTy->isIncompleteType() ||
+OrigPointeeTy->isIncompleteType())
+ return true;
+
// Warn when there is widening cast.
unsigned ToWidth = Ctx.getTypeInfo(ToPointeeTy).Width;
unsigned OrigWidth = Ctx.getTypeInfo(OrigPointeeTy).Width;
Index: test/Analysis/cast-to-struct.cpp
===
--- test/Analysis/cast-to-struct.cpp
+++ test/Analysis/cast-to-struct.cpp
@@ -65,3 +65,17 @@
void *VP = P;
Abc = (struct ABC *)VP;
}
+
+// https://llvm.org/bugs/show_bug.cgi?id=31173
+void dontCrash1(struct AB X) {
+ struct UndefS *S = (struct UndefS *)
+}
+
+struct S;
+struct T {
+ struct S *P;
+};
+extern struct S Var1, Var2;
+void dontCrash2() {
+ ((struct T *) )->P =
+}
Index: lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
@@ -84,6 +84,10 @@
if (!VD || VD->getType()->isReferenceType())
return true;
+if (ToPointeeTy->isIncompleteType() ||
+OrigPointeeTy->isIncompleteType())
+ return true;
+
// Warn when there is widening cast.
unsigned ToWidth = Ctx.getTypeInfo(ToPointeeTy).Width;
unsigned OrigWidth = Ctx.getTypeInfo(OrigPointeeTy).Width;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
danielmarjamaki reopened this revision. danielmarjamaki added a comment. This revision is now accepted and ready to land. I reverted the change because there were buildbot failures. Repository: rL LLVM https://reviews.llvm.org/D28297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
This revision was automatically updated to reflect the committed changes.
Closed by commit rL295545: [analyzer] Fix crash in CastToStruct when there is
no record definition (authored by danielmarjamaki).
Changed prior to commit:
https://reviews.llvm.org/D28297?vs=83062=89019#toc
Repository:
rL LLVM
https://reviews.llvm.org/D28297
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
cfe/trunk/test/Analysis/cast-to-struct.cpp
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
@@ -84,6 +84,13 @@
if (!VD || VD->getType()->isReferenceType())
return true;
+// Don't warn when target type has no definition.
+if (const RecordType *RD = dyn_cast(ToPointeeTy.getTypePtr()))
{
+ if (!RD->getDecl()->getDefinition()) {
+return true;
+ }
+}
+
// Warn when there is widening cast.
unsigned ToWidth = Ctx.getTypeInfo(ToPointeeTy).Width;
unsigned OrigWidth = Ctx.getTypeInfo(OrigPointeeTy).Width;
Index: cfe/trunk/test/Analysis/cast-to-struct.cpp
===
--- cfe/trunk/test/Analysis/cast-to-struct.cpp
+++ cfe/trunk/test/Analysis/cast-to-struct.cpp
@@ -65,3 +65,8 @@
void *VP = P;
Abc = (struct ABC *)VP;
}
+
+// https://llvm.org/bugs/show_bug.cgi?id=31173
+void dontCrash(struct AB X) {
+ struct UndefS *S = (struct UndefS *)
+}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
@@ -84,6 +84,13 @@
if (!VD || VD->getType()->isReferenceType())
return true;
+// Don't warn when target type has no definition.
+if (const RecordType *RD = dyn_cast(ToPointeeTy.getTypePtr())) {
+ if (!RD->getDecl()->getDefinition()) {
+return true;
+ }
+}
+
// Warn when there is widening cast.
unsigned ToWidth = Ctx.getTypeInfo(ToPointeeTy).Width;
unsigned OrigWidth = Ctx.getTypeInfo(OrigPointeeTy).Width;
Index: cfe/trunk/test/Analysis/cast-to-struct.cpp
===
--- cfe/trunk/test/Analysis/cast-to-struct.cpp
+++ cfe/trunk/test/Analysis/cast-to-struct.cpp
@@ -65,3 +65,8 @@
void *VP = P;
Abc = (struct ABC *)VP;
}
+
+// https://llvm.org/bugs/show_bug.cgi?id=31173
+void dontCrash(struct AB X) {
+ struct UndefS *S = (struct UndefS *)
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
zaks.anna added a comment. Please, make sure future reviews go through cfg-dev list. See http://llvm.org/docs/Phabricator.html. Repository: rL LLVM https://reviews.llvm.org/D28297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. I sometimes wish ASTContext methods just didn't crash :) Oh well, let's just see if more problems show up. Comment at: lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp:93 // Warn when there is widening cast. unsigned ToWidth = Ctx.getTypeInfo(ToPointeeTy).Width; I think we should move the check here then. That'd avoid double-checking if `ToPointeeTy` is a record type (we could `cast<>` directly on this branch). Repository: rL LLVM https://reviews.llvm.org/D28297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
danielmarjamaki added a comment.
In https://reviews.llvm.org/D28297#642523, @NoQ wrote:
> Looks good. I assume the crash is in `getTypeInfo()`; do you have any idea
> what are exact prerequisites for using this method? So that there were no
> more crashes here.
Yes. The crash happens during the `getTypeInfo()` call. I don't know what
prerequisites are interesting to check.
The Type pointer returned by getTypePtr() must be nonnull and valid. The method
`clang::Type::getTypeClass()` is called using that type pointer. If that
returns `Type::Record` then the Type pointer is casted to a RecordType. And
`RecordType::getDecl()` is called. The RecordDecl that is returned by that call
is passed to `getASTRecordLayout()` shown below.
The crash occurs on the first assert in this code:
const ASTRecordLayout &
ASTContext::getASTRecordLayout(const RecordDecl *D) const {
// These asserts test different things. A record has a definition
// as soon as we begin to parse the definition. That definition is
// not a complete definition (which is what isDefinition() tests)
// until we *finish* parsing the definition.
if (D->hasExternalLexicalStorage() && !D->getDefinition())
getExternalSource()->CompleteType(const_cast(D));
D = D->getDefinition();
assert(D && "Cannot get layout of forward declarations!");
assert(!D->isInvalidDecl() && "Cannot get layout of invalid decl!");
assert(D->isCompleteDefinition() && "Cannot layout type before complete!");
I am not sure I can write testcases that prevent regressions but do you think I
should add `isInvalidDecl()` and `isCompleteDefinition()` also?
Repository:
rL LLVM
https://reviews.llvm.org/D28297
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
zaks.anna added a comment. Please, subscribe cfe-commits list on the patches as described in http://llvm.org/docs/Phabricator.html. Thanks! Anna Repository: rL LLVM https://reviews.llvm.org/D28297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
NoQ added a subscriber: zaks.anna. NoQ added a comment. Looks good. I assume the crash is in `getTypeInfo()`; do you have any idea what are exact prerequisites for using this method? So that there were no more crashes here. Repository: rL LLVM https://reviews.llvm.org/D28297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
danielmarjamaki created this revision.
danielmarjamaki added a reviewer: NoQ.
danielmarjamaki added a subscriber: cfe-commits.
danielmarjamaki set the repository for this revision to rL LLVM.
This fix the crash reported in https://llvm.org/bugs/show_bug.cgi?id=31173
Repository:
rL LLVM
https://reviews.llvm.org/D28297
Files:
lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
test/Analysis/cast-to-struct.cpp
Index: test/Analysis/cast-to-struct.cpp
===
--- test/Analysis/cast-to-struct.cpp
+++ test/Analysis/cast-to-struct.cpp
@@ -65,3 +65,8 @@
void *VP = P;
Abc = (struct ABC *)VP;
}
+
+// https://llvm.org/bugs/show_bug.cgi?id=31173
+void dontCrash(struct AB X) {
+ struct UndefS *S = (struct UndefS *)
+}
Index: lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
@@ -55,6 +55,12 @@
if (!ToPointeeTy->isStructureOrClassType())
return true;
+ if (const RecordType *RD = dyn_cast(ToPointeeTy.getTypePtr())) {
+if (!RD->getDecl()->getDefinition()) {
+ return true;
+}
+ }
+
// We allow cast from void*.
if (OrigPointeeTy->isVoidType())
return true;
Index: test/Analysis/cast-to-struct.cpp
===
--- test/Analysis/cast-to-struct.cpp
+++ test/Analysis/cast-to-struct.cpp
@@ -65,3 +65,8 @@
void *VP = P;
Abc = (struct ABC *)VP;
}
+
+// https://llvm.org/bugs/show_bug.cgi?id=31173
+void dontCrash(struct AB X) {
+ struct UndefS *S = (struct UndefS *)
+}
Index: lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
@@ -55,6 +55,12 @@
if (!ToPointeeTy->isStructureOrClassType())
return true;
+ if (const RecordType *RD = dyn_cast(ToPointeeTy.getTypePtr())) {
+if (!RD->getDecl()->getDefinition()) {
+ return true;
+}
+ }
+
// We allow cast from void*.
if (OrigPointeeTy->isVoidType())
return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
