[PATCH] D41478: [analyzer] Fix zero-initialization of stack VLAs under ARC.

[Artem Dergachev via Phabricator via cfe-commits]

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

https://developer.apple.com/library/content/releasenotes/ObjectiveC/RN-TransitioningToARC/Introduction/Introduction.html#//apple_ref/doc/uid/TP40011226-CH1-SW5 :

> Using ARC, strong, weak, and autoreleasing stack variables are now implicitly initialized with nil.

This includes variable-length arrays of Objective-C object pointers. However, in the analyzer we don't zero-initialize them. We used to, but it accidentally regressed after r289618.

Under ARC, the array variable's initializer within `DeclStmt` is an `ImplicitValueInitExpr`. Environment doesn't maintain any bindings for this expression kind - instead it always knows that it's a known constant (0 in our case), so it just returns the known value by calling `SValBuilder::makeZeroVal()` (see `EnvironmentManager::getSVal()`. Commit r289618 had introduced reasonable behavior of `SValBuilder::makeZeroVal()` for the arrays, which produces a zero-length `compoundVal{}`. When such value is bound to arrays, in `RegionStoreManager::bindArray()` "remaining" items in the array are default-initialized with zero, as in `RegionStoreManager::setImplicitDefaultValue()`. The similar mechanism works when an array is initialized by an initializer list that is too short, eg. `int a[3] = { 1, 2 };` would result in `a[2]` initialized with `0`. However, in case of variable-length arrays it didn't know if any more items need to be added, because, well, the length is variable.

Add the default binding anyway, regardless of how many actually need to be added. We don't really care, because the default binding covers the whole array anyway.


Repository:
  rC Clang

https://reviews.llvm.org/D41478

Files:
  lib/StaticAnalyzer/Core/RegionStore.cpp
  test/Analysis/arc-zero-init.m


Index: test/Analysis/arc-zero-init.m
===================================================================
--- /dev/null
+++ test/Analysis/arc-zero-init.m
@@ -0,0 +1,46 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify -fobjc-arc %s
+
+#if __has_feature(objc_arc)
+// expected-no-diagnostics
+#endif
+
+ at interface SomeClass
+ at end
+
+void simpleStrongPointerValue() {
+  SomeClass *x;
+  if (x) {}
+#if !__has_feature(objc_arc)
+// expected-warning at -2{{Branch condition evaluates to a garbage value}}
+#endif
+}
+
+void simpleArray() {
+  SomeClass *vlaArray[5];
+
+  if (vlaArray[0]) {}
+#if !__has_feature(objc_arc)
+// expected-warning at -2{{Branch condition evaluates to a garbage value}}
+#endif
+}
+
+void variableLengthArray() {
+   int count = 1;
+   SomeClass * vlaArray[count];
+
+   if (vlaArray[0]) {}
+#if !__has_feature(objc_arc)
+  // expected-warning at -2{{Branch condition evaluates to a garbage value}}
+#endif
+}
+
+void variableLengthArrayWithExplicitStrongAttribute() {
+   int count = 1;
+   __attribute__((objc_ownership(strong))) SomeClass * vlaArray[count];
+
+   if (vlaArray[0]) {}
+#if !__has_feature(objc_arc)
+  // expected-warning at -2{{Branch condition evaluates to a garbage value}}
+#endif
+}
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===================================================================
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2132,9 +2132,10 @@
       NewB = bind(NewB, loc::MemRegionVal(ER), *VI);
   }
 
-  // If the init list is shorter than the array length, set the
-  // array default value.
-  if (Size.hasValue() && i < Size.getValue())
+  // If the init list is shorter than the array length (or the array has
+  // variable length), set the array default value. Values that are already set
+  // are not overwritten.
+  if (!Size.hasValue() || i < Size.getValue())
     NewB = setImplicitDefaultValue(NewB, R, ElementTy);
 
   return NewB;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D41478.127823.patch
Type: text/x-patch
Size: 2023 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171221/cc7a0a07/attachment-0001.bin>