[analyzer] do not crash on libcxx03 call_once implementation

[george.karpenkov]

Summary

Addresses https://bugs.llvm.org/show_bug.cgi?id=35075

[xazax.hun]

lib/Analysis/BodyFarm.cpp

415 ↗

(On Diff #121206)

Wouldn't it be equivalent for checking only callback->getType()->isReferenceType()?

[george.karpenkov]

Updated the diff, addressed review concerns.
Made it more explicit in comments in tests that we do not crash on libcxx03 implementation, but that we don't model it either.

@dcoughlin sorry I disagree on previous testing notes (requiring to explicitly test that the std::call_once on libcxx03 is ignored).
Current tests test exactly the specification we want them to test: libcxx11 is modeled, libcxx03 does not crash.
Testing actual behavior on libcxx03 would entail many problems, with very little benefit:

a) The fact that libcxx03 std::call_once is ignored is not part of the specification, it's just not specified
b) Splitting libcxx03 into a separate file would case many issues:

• Higher cognitive overhead when looking for a particular file
• Harder to run tests with a single command, harder to see all tests for this functionality
• Tests would have to be copied, causing the copying of almost the entire file. Furthermore, all future tests would have to be duplicated as well.
• If we were to eventually model libcxx03 std::call_once, we would have to merge two files back together (or maintain the duplication) to say that we get the same behavior
• This would encourage further stacking up of technical debt: if we would model another libcxx version, and get a separate file for that, we would have to now duplicate every test across three files.

c) Having separate behaviors with ifndef would make the file larger, increase the cognitive overhead, and also make writing future tests more difficult. This is also something easy to forget while writing future tests. Also it's not clear how ifndef would even help, since LIT directives are not controlled by the preprocessor.

[dcoughlin]

In D39518#914515, @george.karpenkov wrote:

Updated the diff, addressed review concerns.
Made it more explicit in comments in tests that we do not crash on libcxx03 implementation, but that we don't model it either.

@dcoughlin sorry I disagree on previous testing notes (requiring to explicitly test that the std::call_once on libcxx03 is ignored).
Current tests test exactly the specification we want them to test: libcxx11 is modeled, libcxx03 does not crash.

At a minimum we need to test that calling call_once() doesn't emit a diagnostic. If the analyzer were to emit a diagnostic on every call, that would be very bad.

It would probably also be good to also test that the arguments are escaped as well.

Testing actual behavior on libcxx03 would entail many problems, with very little benefit:

a) The fact that libcxx03 std::call_once is ignored is not part of the specification, it's just not specified
b) Splitting libcxx03 into a separate file would case many issues:

You don't have to copy the whole file. Please, just add a test (somewhere, anywhere) making sure that that the analyzer does what we expect on a call to call_once() under libcxx03. This should not be difficult and it tests a supported analyzer configuration.

c) Having separate behaviors with ifndef would make the file larger, increase the cognitive overhead, and also make writing future tests more difficult. This is also something easy to forget while writing future tests. Also it's not clear how ifndef would even help, since LIT directives are not controlled by the preprocessor.

You are testing with -verify and // expected-warning. Note that -verify does respect the preprocessor; this is a pattern used throughout the clang tests.

[dcoughlin]

Looks great. Thank you!

cfe/trunk/lib/Analysis/BodyFarm.cpp

static Stmt *create_call_once(ASTContext &C, const FunctionDecl *D) {
   // We need at least two parameters.
   if (D->param_size() < 2)
     return nullptr;
 
   ASTMaker M(C);
 
   const ParmVarDecl *Flag = D->getParamDecl(0);
   const ParmVarDecl *Callback = D->getParamDecl(1);
+
+  if (!Callback->getType()->isReferenceType()) {
+    llvm::dbgs() << "libcxx03 std::call_once implementation, skipping.\n";
+    return nullptr;
+  }
+  if (!Flag->getType()->isReferenceType()) {
+    llvm::dbgs() << "unknown std::call_once implementation, skipping.\n";
+    return nullptr;
+  }
+
   QualType CallbackType = Callback->getType().getNonReferenceType();
 
   // Nullable pointer, non-null iff function is a CXXRecordDecl.
   CXXRecordDecl *CallbackRecordDecl = CallbackType->getAsCXXRecordDecl();
   QualType FlagType = Flag->getType().getNonReferenceType();
   auto *FlagRecordDecl = dyn_cast_or_null<RecordDecl>(FlagType->getAsTagDecl());
 
   if (!FlagRecordDecl) {
     DEBUG(llvm::dbgs() << "Flag field is not a record: "
                        << "unknown std::call_once implementation, "
                        << "ignoring the call.\n");
     return nullptr;
   }
 
   // We initially assume libc++ implementation of call_once,
   // where the once_flag struct has a field `__state_`.
   ValueDecl *FlagFieldDecl = M.findMemberField(FlagRecordDecl, "__state_");
 
   // Otherwise, try libstdc++ implementation, with a field
   // `_M_once`
   if (!FlagFieldDecl) {
-    DEBUG(llvm::dbgs() << "No field __state_ found on std::once_flag struct, "
-                       << "assuming libstdc++ implementation\n");
     FlagFieldDecl = M.findMemberField(FlagRecordDecl, "_M_once");
   }
 
   if (!FlagFieldDecl) {
-    DEBUG(llvm::dbgs() << "No field _M_once found on std::once flag struct: "
-                       << "unknown std::call_once implementation, "
-                       << "ignoring the call");
+    DEBUG(llvm::dbgs() << "No field _M_once or __state_ found on "
+                       << "std::once_flag struct: unknown std::call_once "
+                       << "implementation, ignoring the call.");
     return nullptr;
   }
 
   bool isLambdaCall = CallbackRecordDecl && CallbackRecordDecl->isLambda();
   if (CallbackRecordDecl && !isLambdaCall) {
     DEBUG(llvm::dbgs() << "Not supported: synthesizing body for functors when "
                        << "body farming std::call_once, ignoring the call.");
     return nullptr;
if (isLambdaCall) {
     CallbackFunctionType = CallbackType->getAs<FunctionProtoType>();
   }
 
   if (!CallbackFunctionType)
     return nullptr;
 
   // First two arguments are used for the flag and for the callback.
   if (D->getNumParams() != CallbackFunctionType->getNumParams() + 2) {
-    DEBUG(llvm::dbgs() << "Number of params of the callback does not match "
-                       << "the number of params passed to std::call_once, "
-                       << "ignoring the call");
+    DEBUG(llvm::dbgs() << "Types of params of the callback do not match "
+                       << "params passed to std::call_once, "
+                       << "ignoring the call\n");
     return nullptr;
   }
 
   // All arguments past first two ones are passed to the callback,
   // and we turn lvalues into rvalues if the argument is not passed by
   // reference.
   for (unsigned int ParamIdx = 2; ParamIdx < D->getNumParams(); ParamIdx++) {
     const ParmVarDecl *PDecl = D->getParamDecl(ParamIdx);

cfe/trunk/test/Analysis/call_once.cpp

 // RUN: %clang_analyze_cc1 -std=c++11 -fblocks -analyzer-checker=core,debug.ExprInspection -verify %s
 // RUN: %clang_analyze_cc1 -std=c++11 -fblocks -analyzer-checker=core,debug.ExprInspection -DEMULATE_LIBSTDCPP -verify %s
 
+// We do NOT model libcxx03 implementation, but the analyzer should still
+// not crash.
+// RUN: %clang_analyze_cc1 -std=c++11 -fblocks -analyzer-checker=core,debug.ExprInspection -DEMULATE_LIBCXX03 -verify %s
+// RUN: %clang_analyze_cc1 -std=c++11 -fblocks -analyzer-checker=core,debug.ExprInspection -DEMULATE_LIBCXX03 -DEMULATE_LIBSTDCPP -verify %s
+
 void clang_analyzer_eval(bool);
 
 // Faking std::std::call_once implementation.
 namespace std {
 
 #ifndef EMULATE_LIBSTDCPP
 typedef struct once_flag_s {
   unsigned long __state_ = 0;
 } once_flag;
 #else
 typedef struct once_flag_s {
   int _M_once = 0;
 } once_flag;
 #endif
 
+#ifndef EMULATE_LIBCXX03
 template <class Callable, class... Args>
 void call_once(once_flag &o, Callable&& func, Args&&... args) {};
+#else
+template <class Callable, class... Args> // libcxx03 call_once
+void call_once(once_flag &o, Callable func, Args&&... args) {};
+#endif
 
 } // namespace std
 
 // Check with Lambdas.
 void test_called_warning() {
   std::once_flag g_initialize;
   int z;
 
   std::call_once(g_initialize, [&] {
     int *x = nullptr;
+#ifndef EMULATE_LIBCXX03
     int y = *x; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+#endif
     z = 200;
   });
 }
 
 void test_called_on_path_inside_no_warning() {
   std::once_flag g_initialize;
 
   int *x = nullptr;
   int y = 100;
   int z;
 
   std::call_once(g_initialize, [&] {
     z = 200;
     x = &z;
   });
 
+#ifndef EMULATE_LIBCXX03
   *x = 100; // no-warning
   clang_analyzer_eval(z == 100); // expected-warning{{TRUE}}
+#endif
 }
 
 void test_called_on_path_no_warning() {
   std::once_flag g_initialize;
 
   int *x = nullptr;
   int y = 100;
 
   std::call_once(g_initialize, [&] {
     x = &y;
   });
 
+#ifndef EMULATE_LIBCXX03
   *x = 100; // no-warning
+#else
+  *x = 100; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+#endif
 }
 
 void test_called_on_path_warning() {
   std::once_flag g_initialize;
 
   int y = 100;
   int *x = &y;
 
   std::call_once(g_initialize, [&] {
     x = nullptr;
   });
 
+#ifndef EMULATE_LIBCXX03
   *x = 100; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+#endif
 }
 
 void test_called_once_warning() {
   std::once_flag g_initialize;
 
   int *x = nullptr;
   int y = 100;
 
   std::call_once(g_initialize, [&] {
     x = nullptr;
   });
 
   std::call_once(g_initialize, [&] {
     x = &y;
   });
 
+#ifndef EMULATE_LIBCXX03
   *x = 100; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+#endif
 }
 
 void test_called_once_no_warning() {
   std::once_flag g_initialize;
 
   int *x = nullptr;
   int y = 100;
 
   std::call_once(g_initialize, [&] {
     x = &y;
   });
 
   std::call_once(g_initialize, [&] {
     x = nullptr;
   });
 
+#ifndef EMULATE_LIBCXX03
   *x = 100; // no-warning
+#endif
 }
 
 static int global = 0;
 void funcPointer() {
   global = 1;
 }
 
 void test_func_pointers() {
   static std::once_flag flag;
   std::call_once(flag, &funcPointer);
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(global == 1); // expected-warning{{TRUE}}
+#endif
 }
 
 template <class _Fp>
 class function; // undefined
 template <class _Rp, class... _ArgTypes>
 struct function<_Rp(_ArgTypes...)> {
   _Rp operator()(_ArgTypes...) const {};
   template <class _Fp>
void test_param_passing_lambda() {
   int x = 120;
   int y = 0;
 
   std::call_once(flag, [&](int p) {
     y = p;
   },
                  x);
 
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(y == 120); // expected-warning{{TRUE}}
+#endif
 }
 
 void test_param_passing_lambda_false() {
   std::once_flag flag;
   int x = 120;
 
   std::call_once(flag, [&](int p) {
     x = 0;
   },
                  x);
 
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(x == 120); // expected-warning{{FALSE}}
+#endif
 }
 
 void test_param_passing_stored_lambda() {
   std::once_flag flag;
   int x = 120;
   int y = 0;
 
   auto lambda = [&](int p) {
     y = p;
   };
 
   std::call_once(flag, lambda, x);
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(y == 120); // expected-warning{{TRUE}}
+#endif
 }
 
 void test_multiparam_passing_lambda() {
   std::once_flag flag;
   int x = 120;
 
   std::call_once(flag, [&](int a, int b, int c) {
     x = a + b + c;
   },
                  1, 2, 3);
 
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(x == 120); // expected-warning{{FALSE}}
   clang_analyzer_eval(x == 6); // expected-warning{{TRUE}}
+#endif
 }
 
 static int global2 = 0;
 void test_param_passing_lambda_global() {
   std::once_flag flag;
   global2 = 0;
   std::call_once(flag, [&](int a, int b, int c) {
     global2 = a + b + c;
   },
                  1, 2, 3);
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(global2 == 6); // expected-warning{{TRUE}}
+#endif
 }
 
 static int global3 = 0;
 void funcptr(int a, int b, int c) {
   global3 = a + b + c;
 }
 
 void test_param_passing_funcptr() {
   std::once_flag flag;
   global3 = 0;
 
   std::call_once(flag, &funcptr, 1, 2, 3);
 
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(global3 == 6); // expected-warning{{TRUE}}
+#endif
 }
 
 void test_blocks() {
   global3 = 0;
   std::once_flag flag;
   std::call_once(flag, ^{
     global3 = 120;
   });
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(global3 == 120); // expected-warning{{TRUE}}
+#endif
 }
 
 int call_once() {
   return 5;
 }
 
 void test_non_std_call_once() {
   int x = call_once();
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(x == 5); // expected-warning{{TRUE}}
+#endif
 }
 
 namespace std {
 template <typename d, typename e>
 void call_once(d, e);
 }
 void g();
 void test_no_segfault_on_different_impl() {
+#ifndef EMULATE_LIBCXX03
   std::call_once(g, false); // no-warning
+#endif
 }
 
 void test_lambda_refcapture() {
   static std::once_flag flag;
   int a = 6;
   std::call_once(flag, [&](int &a) { a = 42; }, a);
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(a == 42); // expected-warning{{TRUE}}
+#endif
 }
 
 void test_lambda_refcapture2() {
   static std::once_flag flag;
   int a = 6;
   std::call_once(flag, [=](int &a) { a = 42; }, a);
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(a == 42); // expected-warning{{TRUE}}
+#endif
 }
 
 void test_lambda_fail_refcapture() {
   static std::once_flag flag;
   int a = 6;
   std::call_once(flag, [=](int a) { a = 42; }, a);
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(a == 42); // expected-warning{{FALSE}}
+#endif
 }
 
 void mutator(int &param) {
   param = 42;
 }
 void test_reftypes_funcptr() {
   static std::once_flag flag;
   int a = 6;
   std::call_once(flag, &mutator, a);
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(a == 42); // expected-warning{{TRUE}}
+#endif
 }
 
 void fail_mutator(int param) {
   param = 42;
 }
 void test_mutator_noref() {
   static std::once_flag flag;
   int a = 6;
   std::call_once(flag, &fail_mutator, a);
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(a == 42); // expected-warning{{FALSE}}
+#endif
 }
 
 // Function is implicitly treated as a function pointer
 // even when an ampersand is not explicitly set.
 void callbackn(int &param) {
   param = 42;
 }
 void test_implicit_funcptr() {
   int x = 0;
   static std::once_flag flagn;
 
   std::call_once(flagn, callbackn, x);
+#ifndef EMULATE_LIBCXX03
   clang_analyzer_eval(x == 42); // expected-warning{{TRUE}}
+#endif
 }