[analyzer] Enable cfg-temporary-dtors by default?

[NoQ]

Summary

Final results are still being baked on my end, but by putting this on review i'm trying to say that i believe i managed to fix the biggest problems with this mode with the recent patches, to the point where the new mode is not worse than the old mode. There are still known issues, such as the loss of coverage due to default arguments of object or reference type, but these seem fairly minor and will hopefully be addressed incrementally later on.

The change is not very loud - it's 2-3 times more impactful than the operator new support we added earlier, which is noticeable but not definitely extreme. The new mode fixes some but definitely not all false positives i wanted it to fix - adding support for more constructors is necessary, and i hope to continue improving it in the nearest future. It also provides a fairly fair amount of neat true positives and an expected skew due to the changes in inlining.

[george.karpenkov]

Currently there's no test demonstrating a different behavior from cfg-temporary-dtors being set to true?..

[NoQ]

That's right, most analyzer-oriented tests were added explicitly by me to temporaries.cpp and lifetime-extension.cpp in previous patches, so they didn't need to be changed in this patch. There didn't seem to be many FIXME tests specific to my work before i started doing it - it seems that i covered all of them by adding a cfg-temporary-dtors=true run-line in previous patches.

[NoQ]

Don't inline temporary destructors for now (i.e. keep c++-temp-dtor-inlining=false, previously it was by default irrelevant and arbitrarily set to true). That's because D43791 wasn't enough to handle all the problems with smart pointers; some custom reference counting pointers produce large inlining stacks that we just refuse to traverse. I'd try to address it sooner rather than later and then enable this flag as well.

With c++-temp-dtor-inlining=false this change seems even more quiet (around +27/-24, on roughly the same codebase that produced +20/-35 in D42219), and much like the last time it's mostly about rearranging large chunks of false positives due to complete lack of support for temporaries into smaller groups of false positives due to lack of support for something else. This is far from a breakthrough yet - a lot more work needs to be done, but neither it is falling apart immediately, and, most importantly, it gives hope to address any single problem with a targeted fix, because fundamental issues we used to have with temporaries would be mostly gone. So i wish to encourage switching to the new mode earlier rather than later (though it's always possible to flip the flag back locally in case of any severe problems i didn't foresee).

[dcoughlin]

Yay! This looks good to me.

cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

bool AnalyzerOptions::getBooleanOption(Optional<bool> &V, StringRef Name,
   if (!V.hasValue())
     V = getBooleanOption(Name, DefaultVal, C, SearchInParents);
   return V.getValue();
 }
 
 bool AnalyzerOptions::includeTemporaryDtorsInCFG() {
   return getBooleanOption(IncludeTemporaryDtorsInCFG,
                           "cfg-temporary-dtors",
-                          /* Default = */ false);
+                          /* Default = */ true);
 }
 
 bool AnalyzerOptions::includeImplicitDtorsInCFG() {
   return getBooleanOption(IncludeImplicitDtorsInCFG,
                           "cfg-implicit-dtors",
                           /* Default = */ true);
 }
 
bool AnalyzerOptions::mayInlineCXXSharedPtrDtor() {
   return getBooleanOption(InlineCXXSharedPtrDtor,
                           "c++-shared_ptr-inlining",
                           /*Default=*/false);
 }
 
 bool AnalyzerOptions::mayInlineCXXTemporaryDtors() {
   return getBooleanOption(InlineCXXTemporaryDtors,
                           "c++-temp-dtor-inlining",
-                          /*Default=*/true);
+                          /*Default=*/false);
 }
 
 bool AnalyzerOptions::mayInlineObjCMethod() {
   return getBooleanOption(ObjCInliningMode,
                           "objc-inlining",
                           /* Default = */ true);
 }
 

cfe/trunk/test/Analysis/analyzer-config.c

 }
 
 // CHECK: [config]
 // CHECK-NEXT: cfg-conditional-static-initializers = true
 // CHECK-NEXT: cfg-implicit-dtors = true
 // CHECK-NEXT: cfg-lifetime = false
 // CHECK-NEXT: cfg-loopexit = false
 // CHECK-NEXT: cfg-rich-constructors = true
-// CHECK-NEXT: cfg-temporary-dtors = false
+// CHECK-NEXT: cfg-temporary-dtors = true
 // CHECK-NEXT: exploration_strategy = unexplored_first_queue
 // CHECK-NEXT: faux-bodies = true
 // CHECK-NEXT: graph-trim-interval = 1000
 // CHECK-NEXT: inline-lambdas = true
 // CHECK-NEXT: ipa = dynamic-bifurcate
 // CHECK-NEXT: ipa-always-inline-size = 3
 // CHECK-NEXT: leak-diagnostics-reference-allocation = false
 // CHECK-NEXT: max-inlinable-size = 100

cfe/trunk/test/Analysis/analyzer-config.cpp

 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 %s -o /dev/null -analyzer-checker=core,osx.cocoa,debug.ConfigDumper -analyzer-max-loop 34 > %t 2>&1
 // RUN: FileCheck --input-file=%t %s --match-full-lines
 
 void bar() {}
 void foo() {
   // Call bar 33 times so max-times-inline-large is met and
   // min-blocks-for-inline-large is checked
   for (int i = 0; i < 34; ++i) {
     bar();
   }
 }
 
 class Foo {
 public:
   ~Foo() {}
-  void baz();
+  void baz() { Foo(); }
   void bar() { const Foo &f = Foo(); }
   void foo() { bar(); }
 };
 
 // CHECK: [config]
 // CHECK-NEXT: c++-container-inlining = false
 // CHECK-NEXT: c++-inlining = destructors
 // CHECK-NEXT: c++-shared_ptr-inlining = false
 // CHECK-NEXT: c++-stdlib-inlining = true
+// CHECK-NEXT: c++-temp-dtor-inlining = false
 // CHECK-NEXT: c++-template-inlining = true
 // CHECK-NEXT: cfg-conditional-static-initializers = true
 // CHECK-NEXT: cfg-implicit-dtors = true
 // CHECK-NEXT: cfg-lifetime = false
 // CHECK-NEXT: cfg-loopexit = false
 // CHECK-NEXT: cfg-rich-constructors = true
-// CHECK-NEXT: cfg-temporary-dtors = false
+// CHECK-NEXT: cfg-temporary-dtors = true
 // CHECK-NEXT: experimental-enable-naive-ctu-analysis = false
 // CHECK-NEXT: exploration_strategy = unexplored_first_queue
 // CHECK-NEXT: faux-bodies = true
 // CHECK-NEXT: graph-trim-interval = 1000
 // CHECK-NEXT: inline-lambdas = true
 // CHECK-NEXT: ipa = dynamic-bifurcate
 // CHECK-NEXT: ipa-always-inline-size = 3
 // CHECK-NEXT: leak-diagnostics-reference-allocation = false
 // CHECK-NEXT: max-inlinable-size = 100
 // CHECK-NEXT: max-nodes = 225000
 // CHECK-NEXT: max-times-inline-large = 32
 // CHECK-NEXT: min-cfg-size-treat-functions-as-large = 14
 // CHECK-NEXT: mode = deep
 // CHECK-NEXT: region-store-small-struct-limit = 2
 // CHECK-NEXT: serialize-stats = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 28
+// CHECK-NEXT: num-entries = 29

cfe/trunk/test/Analysis/inlining/temp-dtors-path-notes.cpp

-// RUN: %clang_analyze_cc1 -analyze -analyzer-checker core -analyzer-config cfg-temporary-dtors=true -analyzer-output=text -verify %s
+// RUN: %clang_analyze_cc1 -analyze -analyzer-checker core -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -analyzer-output=text -verify %s
 
 namespace test_simple_temporary {
 class C {
   int x;
 
 public:
   C(int x): x(x) {} // expected-note{{The value 0 is assigned to field 'x'}}
   ~C() { x = 1 / x; } // expected-warning{{Division by zero}}

cfe/trunk/test/Analysis/lifetime-cfg-output.cpp

-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -analyze -analyzer-checker=debug.DumpCFG -analyzer-config cfg-lifetime=true,cfg-rich-constructors=false -analyzer-config cfg-implicit-dtors=false %s > %t 2>&1
+// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -analyze -analyzer-checker=debug.DumpCFG -analyzer-config cfg-lifetime=true,cfg-temporary-dtors=false,cfg-rich-constructors=false -analyzer-config cfg-implicit-dtors=false %s > %t 2>&1
 // RUN: FileCheck --input-file=%t %s
 
 extern bool UV;
 class A {
 public:
   // CHECK:       [B2 (ENTRY)]
   // CHECK-NEXT:    Succs (1): B1
   // CHECK:       [B1]

cfe/trunk/test/Analysis/lifetime-extension.cpp

-// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -verify %s
-// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true -DTEMPORARIES -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -DTEMPORARIES -verify %s
 
 void clang_analyzer_eval(bool);
 
 namespace pr17001_call_wrong_destructor {
 bool x;
 struct A {
   int *a;
   A() {}

cfe/trunk/test/Analysis/temporaries.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true %s -std=c++11
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++11
 
 extern bool clang_analyzer_eval(bool);
 extern bool clang_analyzer_warnIfReached();
 void clang_analyzer_checkInlined(bool);
 
 #include "Inputs/system-header-simulator-cxx.h";
 
 struct Trivial {