Skip to content

Commit

Permalink
[analyzer] Invalidate union regions properly. Don't hesitate to load …
Browse files Browse the repository at this point in the history
…later.

We weren't invalidating our unions correctly. The previous behavior in
invalidateRegionsWorker::VisitCluster() was to direct-bind an UnknownVal
to the union (at offset 0).

For that reason we were never actually loading default bindings from our unions,
because there never was any default binding to load, and the value
that is presumed when there's no default binding to load
is usually completely incorrect (eg. UndefinedVal for stack unions).

The new behavior is to default-bind a conjured symbol (of irrelevant type)
to the union that's being invalidated, similarly to what we do for structures
and classes. Then it becomes safe to load the value properly.

Differential Revision: https://reviews.llvm.org/D45241

llvm-svn: 331563
  • Loading branch information
haoNoQ committed May 4, 2018
1 parent e603e07 commit 394588a
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 8 deletions.
7 changes: 1 addition & 6 deletions clang/lib/StaticAnalyzer/Core/RegionStore.cpp
Expand Up @@ -230,11 +230,6 @@ Optional<SVal> RegionBindingsRef::getDirectBinding(const MemRegion *R) const {
}

Optional<SVal> RegionBindingsRef::getDefaultBinding(const MemRegion *R) const {
if (R->isBoundable())
if (const TypedValueRegion *TR = dyn_cast<TypedValueRegion>(R))
if (TR->getValueType()->isUnionType())
return UnknownVal();

return Optional<SVal>::create(lookup(R, BindingKey::Default));
}

Expand Down Expand Up @@ -1099,7 +1094,7 @@ void invalidateRegionsWorker::VisitCluster(const MemRegion *baseR,
return;
}

if (T->isStructureOrClassType()) {
if (T->isRecordType()) {
// Invalidate the region by setting its default value to
// conjured symbol. The type of the symbol is irrelevant.
DefinedOrUnknownSVal V = svalBuilder.conjureSymbolVal(baseR, Ex, LCtx,
Expand Down
20 changes: 18 additions & 2 deletions clang/test/Analysis/unions.cpp
Expand Up @@ -79,8 +79,7 @@ namespace PR17596 {
IntOrString vv;
vv.i = 5;
uu = vv;
// FIXME: Should be true.
clang_analyzer_eval(uu.i == 5); // expected-warning{{UNKNOWN}}
clang_analyzer_eval(uu.i == 5); // expected-warning{{TRUE}}
}

void testInvalidation() {
Expand All @@ -106,3 +105,20 @@ namespace PR17596 {
clang_analyzer_eval(uu.s[0] == 'a'); // expected-warning{{UNKNOWN}}
}
}

namespace assume_union_contents {
union U {
int x;
};

U get();

void test() {
U u = get();
int y = 0;
if (u.x)
y = 1;
if (u.x)
y = 1 / y; // no-warning
}
} // end namespace assume_union_contents

0 comments on commit 394588a

Please sign in to comment.