[PATCH] D42219: [analyzer] Enable c++-allocator-inlining by default?

[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.

Once https://reviews.llvm.org/D42192 lands, i believe it should be safe to flip the default value of `-analyzer-config c++-allocator-inlining` to `true` by default.

My evaluation (on a large mostly internal codebase, including WebKit, more than a day of real-world time of continuous analysis) shows that around twice as much false positives are getting fixed (~35) as new false positives added (~20). Additionally, the reason for newly added false positives is our increased precision and coverage due to more aggressive inlining, which exposes other, unrelated problems in the analyzer, which is an expected outcome of enabling more coverage on codebases that already have high false positive rates (such as many C++ codebases). Gradually improving C++ support should be mitigating those. One true positive was lost due to analyzer reaching the `max-nodes` limit due to more aggressive inlining. I didn't immediately find any new positives that were definitely true, but some looked fairly suspicious.

The old mode would be available for a while, so it's possible, if necessary, to turn the new mode off with `-analyzer-config c++-allocator-inlining=false`.

All changes in tests in the patch are FIXMEs that were resolved by enabling the new mode.


Repository:
  rC Clang

https://reviews.llvm.org/D42219

Files:
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  test/Analysis/NewDelete-custom.cpp
  test/Analysis/ctor.mm
  test/Analysis/new.cpp
  test/Analysis/virtualcall.cpp


Index: test/Analysis/virtualcall.cpp
===================================================================
--- test/Analysis/virtualcall.cpp
+++ test/Analysis/virtualcall.cpp
@@ -262,6 +262,9 @@
 	//expected-note-re at -2 {{{{^}}Calling default constructor for 'M'}}
 #endif
   Y *y = new Y;
+#if !PUREONLY
+  //expected-note-re at -2 {{{{^}}Calling default constructor for 'Y'}}
+#endif
   delete y;
   header::Z z;
 #if !PUREONLY
Index: test/Analysis/new.cpp
===================================================================
--- test/Analysis/new.cpp
+++ test/Analysis/new.cpp
@@ -34,7 +34,7 @@
 
   void *y = new (x) int;
   clang_analyzer_eval(x == y); // expected-warning{{TRUE}};
-  clang_analyzer_eval(*x == 1); // expected-warning{{UNKNOWN}};
+  clang_analyzer_eval(*x == 1); // expected-warning{{TRUE}};
 
   return y;
 }
@@ -200,8 +200,7 @@
   int n;
   new (&n) int;
 
-  // Should warn that n is uninitialized.
-  if (n) { // no-warning
+  if (n) { // expected-warning{{Branch condition evaluates to a garbage value}}
     return 0;
   }
   return 1;
Index: test/Analysis/ctor.mm
===================================================================
--- test/Analysis/ctor.mm
+++ test/Analysis/ctor.mm
@@ -572,10 +572,9 @@
   }
 
   void testNew() {
-    // FIXME: Pending proper implementation of constructors for 'new'.
     raw_pair *pp = new raw_pair();
-    clang_analyzer_eval(pp->p1 == 0); // expected-warning{{UNKNOWN}}
-    clang_analyzer_eval(pp->p2 == 0); // expected-warning{{UNKNOWN}}
+    clang_analyzer_eval(pp->p1 == 0); // expected-warning{{TRUE}}
+    clang_analyzer_eval(pp->p2 == 0); // expected-warning{{TRUE}}
   }
 
   void testArrayNew() {
@@ -679,8 +678,7 @@
 
   void testDynamic() {
     List *list = new List{1, 2};
-    // FIXME: When we handle constructors with 'new', this will be TRUE.
-    clang_analyzer_eval(list->usedInitializerList); // expected-warning{{UNKNOWN}}
+    clang_analyzer_eval(list->usedInitializerList); // expected-warning{{TRUE}}
   }
 }
 
Index: test/Analysis/NewDelete-custom.cpp
===================================================================
--- test/Analysis/NewDelete-custom.cpp
+++ test/Analysis/NewDelete-custom.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,unix.Malloc -std=c++11 -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -DLEAKS=1 -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,unix.Malloc -std=c++11 -analyzer-config c++-allocator-inlining=true -DALLOCATOR_INLINING=1 -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -analyzer-config c++-allocator-inlining=true -DLEAKS=1 -DALLOCATOR_INLINING=1 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,unix.Malloc -std=c++11 -analyzer-config c++-allocator-inlining=false -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -analyzer-config c++-allocator-inlining=false -DLEAKS=1 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,unix.Malloc -std=c++11 -DALLOCATOR_INLINING=1 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -DLEAKS=1 -DALLOCATOR_INLINING=1 -fblocks -verify %s
 #include "Inputs/system-header-simulator-cxx.h"
 
 #if !(LEAKS && !ALLOCATOR_INLINING)
Index: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
===================================================================
--- lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
+++ lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
@@ -203,7 +203,7 @@
 bool AnalyzerOptions::mayInlineCXXAllocator() {
   return getBooleanOption(InlineCXXAllocator,
                           "c++-allocator-inlining",
-                          /*Default=*/false);
+                          /*Default=*/true);
 }
 
 bool AnalyzerOptions::mayInlineCXXContainerMethods() {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D42219.130326.patch
Type: text/x-patch
Size: 4153 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180118/c1127932/attachment.bin>