NoQ created this revision.
NoQ added reviewers: zaks.anna, dcoughlin, xazax.hun, a.sidorin.
NoQ added a subscriber: cfe-commits.
When binding string literal regions to `char` arrays, `RegionStore`'s
`bindArray()` method converts the string literals to their lazy compound values
before binding. Bug https://llvm.org/bugs/show_bug.cgi?id=28449 shows that
similar behavior is required for handling compound literals (brace
initializers).
However, it seems illogical to me that `RegionStore` modifies the value it was
asked to bind - it should be handled by the external code, and `RegionStore`'s
`bind...()` interface should perhaps only bind the given value to the given
location, without improvising.
This patch conducts the necessary refactoring to avoid the issue. Now all
compound values should be correct to begin with. The patch accidentally fixes
the bug.
Additionally, this patch enables loading values from compound-literal regions -
it was explicitly disabled, but the problem due to which it was disabled seems
already resolved; made the respective tests stronger to see that it's actually
working correctly.
Additionally, this patch tweaks the dump method of compound literal regions,
addressing a FIXME there and making things prettier.
Didn't notice significant changes on large codebase runs - no new crashes, no
changes in positives.
https://reviews.llvm.org/D23963
Files:
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineC.cpp
lib/StaticAnalyzer/Core/MemRegion.cpp
lib/StaticAnalyzer/Core/RegionStore.cpp
test/Analysis/compound-literals.c
test/Analysis/region-store.c
Index: test/Analysis/region-store.c
===================================================================
--- test/Analysis/region-store.c
+++ test/Analysis/region-store.c
@@ -1,14 +1,16 @@
// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix,debug.ExprInspection -verify %s
int printf(const char *restrict,...);
+void clang_analyzer_eval();
// Testing core functionality of the region store.
// radar://10127782
int compoundLiteralTest() {
int index = 0;
for (index = 0; index < 2; index++) {
int thing = (int []){0, 1}[index];
printf("thing: %i\n", thing);
+ clang_analyzer_eval(thing == index); // expected-warning{{TRUE}}
}
return 0;
}
@@ -18,6 +20,7 @@
for (index = 0; index < 3; index++) {
int thing = (int [][3]){{0,0,0}, {1,1,1}, {2,2,2}}[index][index];
printf("thing: %i\n", thing);
+ clang_analyzer_eval(thing == index); // expected-warning{{TRUE}}
}
return 0;
}
Index: test/Analysis/compound-literals.c
===================================================================
--- /dev/null
+++ test/Analysis/compound-literals.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple=i386-apple-darwin10 -analyze -analyzer-checker=debug.ExprInspection -verify %s
+void clang_analyzer_eval(int);
+
+// pr28449: Used to crash.
+void foo(void) {
+ static const unsigned short array[] = (const unsigned short[]){0x0F00};
+ clang_analyzer_eval(array[0] == 0x0F00); // expected-warning{{TRUE}}
+}
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===================================================================
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1568,10 +1568,6 @@
SVal RegionStoreManager::getBindingForElement(RegionBindingsConstRef B,
const ElementRegion* R) {
- // We do not currently model bindings of the CompoundLiteralregion.
- if (isa<CompoundLiteralRegion>(R->getBaseRegion()))
- return UnknownVal();
-
// Check if the region has a binding.
if (const Optional<SVal> &V = B.getDirectBinding(R))
return *V;
@@ -2057,17 +2053,6 @@
if (const ConstantArrayType* CAT = dyn_cast<ConstantArrayType>(AT))
Size = CAT->getSize().getZExtValue();
- // Check if the init expr is a string literal.
- if (Optional<loc::MemRegionVal> MRV = Init.getAs<loc::MemRegionVal>()) {
- const StringRegion *S = cast<StringRegion>(MRV->getRegion());
-
- // Treat the string as a lazy compound value.
- StoreRef store(B.asStore(), *this);
- nonloc::LazyCompoundVal LCV = svalBuilder.makeLazyCompoundVal(store, S)
- .castAs<nonloc::LazyCompoundVal>();
- return bindAggregate(B, R, LCV);
- }
-
// Handle lazy compound values.
if (Init.getAs<nonloc::LazyCompoundVal>())
return bindAggregate(B, R, Init);
Index: lib/StaticAnalyzer/Core/MemRegion.cpp
===================================================================
--- lib/StaticAnalyzer/Core/MemRegion.cpp
+++ lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -431,8 +431,9 @@
}
void CompoundLiteralRegion::dumpToStream(raw_ostream &os) const {
- // FIXME: More elaborate pretty-printing.
- os << "{ " << static_cast<const void*>(CL) << " }";
+ os << "{ ";
+ CL->printPretty(os, nullptr, PrintingPolicy(getContext().getLangOpts()));
+ os << " }";
}
void CXXTempObjectRegion::dumpToStream(raw_ostream &os) const {
Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -535,14 +535,9 @@
} else {
// We bound the temp obj region to the CXXConstructExpr. Now recover
// the lazy compound value when the variable is not a reference.
- if (AMgr.getLangOpts().CPlusPlus && VD->getType()->isRecordType() &&
- !VD->getType()->isReferenceType()) {
- if (Optional<loc::MemRegionVal> M =
- InitVal.getAs<loc::MemRegionVal>()) {
- InitVal = state->getSVal(M->getRegion());
- assert(InitVal.getAs<nonloc::LazyCompoundVal>());
- }
- }
+ if (InitEx->isLValue() && !InitVal.isUnknownOrUndef() &&
+ !VD->getType()->isReferenceType())
+ InitVal = state->getSVal(InitVal.castAs<Loc>(), InitEx->getType());
// Recover some path-sensitivity if a scalar value evaluated to
// UnknownVal.
@@ -663,10 +658,50 @@
return;
}
- for (InitListExpr::const_reverse_iterator it = IE->rbegin(),
- ei = IE->rend(); it != ei; ++it) {
- SVal V = state->getSVal(cast<Expr>(*it), LCtx);
- vals = getBasicVals().consVals(V, vals);
+ // We need to recover the rvalue of the initializer list's element in case
+ // the value it's going to be bound to is not a reference.
+ // The AST represents it by making the initializer list have the correct
+ // type, as any expression. We need to read this type and make the
+ // necessary conversions.
+ if (const RecordType *RT =
+ IE->getType().getCanonicalType()->getAs<RecordType>()) {
+ // For structures, check if the respective field is a reference.
+ // FIXME: What if fields mismatch?
+ const RecordDecl *RD = RT->getDecl();
+ assert(RD);
+ auto FieldI = RD->field_begin(), FieldE = RD->field_end();
+ for (auto InitI = IE->rbegin(), InitE = IE->rend();
+ InitI != InitE; ++InitI) {
+ if (FieldI == FieldE)
+ break;
+ const Expr *E = cast<Expr>(*InitI);
+ SVal V = state->getSVal(E, LCtx);
+ if (E->isLValue() && !V.isUnknownOrUndef() &&
+ !FieldI->getType()->isReferenceType())
+ V = state->getSVal(V.castAs<Loc>(), FieldI->getType());
+ vals = getBasicVals().consVals(V, vals);
+ ++FieldI;
+ }
+ } else if (const ArrayType *AT = getContext().getAsArrayType(
+ IE->getType().getCanonicalType())) {
+ // There cannot be arrays of references. Every lvalue expression
+ // deserves to be dereferenced.
+ QualType ET = AT->getElementType();
+ for (auto InitI = IE->rbegin(), InitE = IE->rend();
+ InitI != InitE; ++InitI) {
+ const Expr *E = cast<Expr>(*InitI);
+ SVal V = state->getSVal(E, LCtx);
+ if (E->isLValue() && !V.isUnknownOrUndef())
+ V = state->getSVal(V.castAs<Loc>(), ET);
+ vals = getBasicVals().consVals(V, vals);
+ }
+ } else {
+ // Vector values, complex numbers, etc: No lvalues to expect.
+ for (auto InitI = IE->rbegin(), InitE = IE->rend();
+ InitI != InitE; ++InitI) {
+ SVal V = state->getSVal(cast<Expr>(*InitI), LCtx);
+ vals = getBasicVals().consVals(V, vals);
+ }
}
B.generateNode(IE, Pred,
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -512,6 +512,20 @@
}
} else {
InitVal = State->getSVal(BMI->getInit(), stackFrame);
+
+ // If the initializer is an lvalue, and the field is not a reference,
+ // then the initializer's rvalue is actually being copied,
+ // and therefore we need to do an extra getSVal()-dereference.
+ const Expr *ExprForReasoningIfLValue = BMI->getInit();
+ if (const CXXDefaultInitExpr *DIE =
+ dyn_cast<CXXDefaultInitExpr>(ExprForReasoningIfLValue))
+ ExprForReasoningIfLValue = DIE->getExpr();
+
+ if (ExprForReasoningIfLValue->isLValue() &&
+ !InitVal.isUnknownOrUndef() &&
+ !Field->getType()->isReferenceType()) {
+ InitVal = State->getSVal(InitVal.castAs<Loc>(), Field->getType());
+ }
}
assert(Tmp.size() == 1 && "have not generated any new nodes yet");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
