[PATCH] D45241: [analyzer] Invalidate union regions properly. Don't hesitate to load the default binding later.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC331563: [analyzer] Invalidate union regions properly. 
Dont hesitate to load later. (authored by dergachev, committed by ).
Herald added a subscriber: baloghadamsoftware.

Repository:
  rC Clang

https://reviews.llvm.org/D45241

Files:
  lib/StaticAnalyzer/Core/RegionStore.cpp
  test/Analysis/unions.cpp


Index: test/Analysis/unions.cpp
===
--- test/Analysis/unions.cpp
+++ test/Analysis/unions.cpp
@@ -79,8 +79,7 @@
 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() {
@@ -106,3 +105,20 @@
 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
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -230,11 +230,6 @@
 }
 
 Optional RegionBindingsRef::getDefaultBinding(const MemRegion *R) const {
-  if (R->isBoundable())
-if (const TypedValueRegion *TR = dyn_cast(R))
-  if (TR->getValueType()->isUnionType())
-return UnknownVal();
-
   return Optional::create(lookup(R, BindingKey::Default));
 }
 
@@ -1099,7 +1094,7 @@
 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,


Index: test/Analysis/unions.cpp
===
--- test/Analysis/unions.cpp
+++ test/Analysis/unions.cpp
@@ -79,8 +79,7 @@
 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() {
@@ -106,3 +105,20 @@
 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
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -230,11 +230,6 @@
 }
 
 Optional RegionBindingsRef::getDefaultBinding(const MemRegion *R) const {
-  if (R->isBoundable())
-if (const TypedValueRegion *TR = dyn_cast(R))
-  if (TR->getValueType()->isUnionType())
-return UnknownVal();
-
   return Optional::create(lookup(R, BindingKey::Default));
 }
 
@@ -1099,7 +1094,7 @@
 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,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45241: [analyzer] Invalidate union regions properly. Don't hesitate to load the default binding later.

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin accepted this revision.
a.sidorin added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D45241



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45241: [analyzer] Invalidate union regions properly. Don't hesitate to load the default binding later.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: test/Analysis/unions.cpp:82
 uu = vv;
-// FIXME: Should be true.
-clang_analyzer_eval(uu.i == 5); // expected-warning{{UNKNOWN}}
+clang_analyzer_eval(uu.i == 5); // expected-warning{{TRUE}}
   }

This test got fixed because we're now loading the default-bound lazy compound 
value for `vv` from `uu` correctly.


Repository:
  rC Clang

https://reviews.llvm.org/D45241



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45241: [analyzer] Invalidate union regions properly. Don't hesitate to load the default binding later.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.

(1) 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). The proposed 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.

(2) In order to keep ourselves afloat, 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).

Fix bug (1) and then remove hack (2).


Repository:
  rC Clang

https://reviews.llvm.org/D45241

Files:
  lib/StaticAnalyzer/Core/RegionStore.cpp
  test/Analysis/unions.cpp


Index: test/Analysis/unions.cpp
===
--- test/Analysis/unions.cpp
+++ test/Analysis/unions.cpp
@@ -79,8 +79,7 @@
 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() {
@@ -106,3 +105,20 @@
 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
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -230,11 +230,6 @@
 }
 
 Optional RegionBindingsRef::getDefaultBinding(const MemRegion *R) const {
-  if (R->isBoundable())
-if (const TypedValueRegion *TR = dyn_cast(R))
-  if (TR->getValueType()->isUnionType())
-return UnknownVal();
-
   return Optional::create(lookup(R, BindingKey::Default));
 }
 
@@ -1095,7 +1090,7 @@
 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,


Index: test/Analysis/unions.cpp
===
--- test/Analysis/unions.cpp
+++ test/Analysis/unions.cpp
@@ -79,8 +79,7 @@
 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() {
@@ -106,3 +105,20 @@
 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
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -230,11 +230,6 @@
 }
 
 Optional RegionBindingsRef::getDefaultBinding(const MemRegion *R) const {
-  if (R->isBoundable())
-if (const TypedValueRegion *TR = dyn_cast(R))
-  if (TR->getValueType()->isUnionType())
-return UnknownVal();
-
   return Optional::create(lookup(R, BindingKey::Default));
 }
 
@@ -1095,7 +1090,7 @@
 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,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits