[PATCH] D39518: [analyzer] do not crash on libcxx03 call_once implementation

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov updated this revision to Diff 121371.
george.karpenkov added a comment.

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.


https://reviews.llvm.org/D39518

Files:
  lib/Analysis/BodyFarm.cpp
  test/Analysis/call_once.cpp


Index: test/Analysis/call_once.cpp
===================================================================
--- test/Analysis/call_once.cpp
+++ test/Analysis/call_once.cpp
@@ -1,6 +1,12 @@
 // 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.
+// Note that the calls below don't have the -verify flag.
+// RUN: %clang_analyze_cc1 -std=c++11 -fblocks -analyzer-checker=core,debug.ExprInspection -DEMULATE_LIBCXX03 %s
+// RUN: %clang_analyze_cc1 -std=c++11 -fblocks -analyzer-checker=core,debug.ExprInspection -DEMULATE_LIBCXX03 -DEMULATE_LIBSTDCPP %s
+
 void clang_analyzer_eval(bool);
 
 // Faking std::std::call_once implementation.
@@ -16,8 +22,13 @@
 } 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
 
Index: lib/Analysis/BodyFarm.cpp
===================================================================
--- lib/Analysis/BodyFarm.cpp
+++ lib/Analysis/BodyFarm.cpp
@@ -325,6 +325,16 @@
 
   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.
@@ -346,15 +356,13 @@
   // 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;
   }
 
@@ -388,9 +396,9 @@
 
   // 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;
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D39518.121371.patch
Type: text/x-patch
Size: 3355 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171102/2d70ce34/attachment.bin>