[Analyzer] Always use non-reference types when creating expressions in BodyFarm, removes std::call_once crash

[george.karpenkov]

Summary

Remove an option to use a reference type (on by default!) since a non-reference type is always needed for creating expressions, functions with multiple boolean parameters are very hard to use, and in general it was just a booby trap for further crashes.
Furthermore, generalize call_once test case to fix some of the crashes mentioned https://bugs.llvm.org/show_bug.cgi?id=34869

[dcoughlin]

Looks good to me.

This comment is unrelated to this particular patch, but since I know you're doing other work in the area I think it would also be good to have test cases for the two other forms of call_once defined in <mutex>. It looks to me like those will be used for projects that that use pre-C++11 -- so we should have test coverage for them.

[danielmarjamaki]

cfe/trunk/lib/Analysis/BodyFarm.cpp

139 ↗

(On Diff #119392)

this looks strange, did clang-format do this?

[alexfh]

cfe/trunk/lib/Analysis/BodyFarm.cpp

366 ↗

(On Diff #119392)

Remove the spaces inside the comment. /*ParamName=*/value is the format that is understood by clang-format and the https://clang.llvm.org/extra/clang-tidy/checks/misc-argument-comment.html clang-tidy check. Same elsewhere in this patch.

[george.karpenkov]

cfe/trunk/lib/Analysis/BodyFarm.cpp

139 ↗

(On Diff #119392)

yes i believe so.

366 ↗

(On Diff #119392)

yeah, i can do that.
can we also consider changing clang-tidy to ignore those spaces?

[alexfh]

cfe/trunk/lib/Analysis/BodyFarm.cpp

366 ↗

(On Diff #119392)

I've just looked at the source code, and, apparently, clang-tidy already understands spaces in argument comments already. Sorry for misinforming you. However, clang-format is more strict here and there's also the consistency argument (see the comment on r316539), which I find to be a good motivation to change this (or at least to not introduce more inconsistency).

lib/Analysis/BodyFarm.cpp

public:
   BinaryOperator *makeComparison(const Expr *LHS, const Expr *RHS,
                                  BinaryOperator::Opcode Op);
   
   /// Create a new compound stmt using the provided statements.
   CompoundStmt *makeCompound(ArrayRef<Stmt*>);
   
   /// Create a new DeclRefExpr for the referenced variable.
   DeclRefExpr *makeDeclRefExpr(const VarDecl *D,
-                               bool RefersToEnclosingVariableOrCapture = false,
-                               bool GetNonReferenceType = false);
+                               bool RefersToEnclosingVariableOrCapture = false);
   
   /// Create a new UnaryOperator representing a dereference.
   UnaryOperator *makeDereference(const Expr *Arg, QualType Ty);
   
   /// Create an implicit cast for an integer conversion.
   Expr *makeIntegralCast(const Expr *Arg, QualType Ty);
   
   /// Create an implicit cast to a builtin boolean type.
   ImplicitCastExpr *makeIntegralCastToBoolean(const Expr *Arg);
   
   /// Create an implicit cast for lvalue-to-rvaluate conversions.
   ImplicitCastExpr *makeLvalueToRvalue(const Expr *Arg, QualType Ty);
   
   /// Make RValue out of variable declaration, creating a temporary
   /// DeclRefExpr in the process.
   ImplicitCastExpr *
   makeLvalueToRvalue(const VarDecl *Decl,
-                     bool RefersToEnclosingVariableOrCapture = false,
-                     bool GetNonReferenceType = false);
+                     bool RefersToEnclosingVariableOrCapture = false);
 
   /// Create an implicit cast of the given type.
   ImplicitCastExpr *makeImplicitCast(const Expr *Arg, QualType Ty,
                                      CastKind CK = CK_LValueToRValue);
 
   /// Create an Objective-C bool literal.
   ObjCBoolLiteralExpr *makeObjCBool(bool Val);
 
return new (C) BinaryOperator(const_cast<Expr*>(LHS),
                                 VK_RValue,
                                 OK_Ordinary, SourceLocation(), FPOptions());
 }
 
 CompoundStmt *ASTMaker::makeCompound(ArrayRef<Stmt *> Stmts) {
   return new (C) CompoundStmt(C, Stmts, SourceLocation(), SourceLocation());
 }
 
-DeclRefExpr *ASTMaker::makeDeclRefExpr(const VarDecl *D,
-                                       bool RefersToEnclosingVariableOrCapture,
-                                       bool GetNonReferenceType) {
-  auto Type = D->getType();
-  if (GetNonReferenceType)
-    Type = Type.getNonReferenceType();
+DeclRefExpr *ASTMaker::makeDeclRefExpr(
+    const VarDecl *D,
+    bool RefersToEnclosingVariableOrCapture) {
+  QualType Type = D->getType().getNonReferenceType();
 
   DeclRefExpr *DR = DeclRefExpr::Create(
       C, NestedNameSpecifierLoc(), SourceLocation(), const_cast<VarDecl *>(D),
       RefersToEnclosingVariableOrCapture, SourceLocation(), Type, VK_LValue);
   return DR;
 }
 
 UnaryOperator *ASTMaker::makeDereference(const Expr *Arg, QualType Ty) {
   return new (C) UnaryOperator(const_cast<Expr*>(Arg), UO_Deref, Ty,
                                VK_LValue, OK_Ordinary, SourceLocation());
 }
 
 ImplicitCastExpr *ASTMaker::makeLvalueToRvalue(const Expr *Arg, QualType Ty) {
   return makeImplicitCast(Arg, Ty, CK_LValueToRValue);
 }
 
 ImplicitCastExpr *
 ASTMaker::makeLvalueToRvalue(const VarDecl *Arg,
-                             bool RefersToEnclosingVariableOrCapture,
-                             bool GetNonReferenceType) {
-  auto Type = Arg->getType();
-  if (GetNonReferenceType)
-    Type = Type.getNonReferenceType();
+                             bool RefersToEnclosingVariableOrCapture) {
+  QualType Type = Arg->getType().getNonReferenceType();
   return makeLvalueToRvalue(makeDeclRefExpr(Arg,
-                                            RefersToEnclosingVariableOrCapture,
-                                            GetNonReferenceType),
+                                            RefersToEnclosingVariableOrCapture),
                             Type);
 }
 
 ImplicitCastExpr *ASTMaker::makeImplicitCast(const Expr *Arg, QualType Ty,
                                              CastKind CK) {
   return ImplicitCastExpr::Create(C, Ty,
                                   /* CastKind= */ CK,
                                   /* Expr= */ const_cast<Expr *>(Arg),

 typedef Stmt *(*FunctionFarmer)(ASTContext &C, const FunctionDecl *D);
 
 static CallExpr *create_call_once_funcptr_call(ASTContext &C, ASTMaker M,
                                                const ParmVarDecl *Callback,
                                                ArrayRef<Expr *> CallArgs) {
 
   return new (C) CallExpr(
       /*ASTContext=*/C,
-      /*StmtClass=*/M.makeLvalueToRvalue(/*Expr=*/Callback),
+      /*StmtClass=*/M.makeLvalueToRvalue(/*Expr=*/Callback), // TODO: this is a crashling line w/ reference type
       /*args=*/CallArgs,
       /*QualType=*/C.VoidTy,
       /*ExprValueType=*/VK_RValue,
       /*SourceLocation=*/SourceLocation());
 }
 
 static CallExpr *create_call_once_lambda_call(ASTContext &C, ASTMaker M,
                                               const ParmVarDecl *Callback,
bool isLambdaCall = CallbackType->getAsCXXRecordDecl() &&
                       CallbackType->getAsCXXRecordDecl()->isLambda();
 
   SmallVector<Expr *, 5> CallArgs;
 
   if (isLambdaCall)
     // Lambda requires callback itself inserted as a first parameter.
     CallArgs.push_back(
         M.makeDeclRefExpr(Callback,
-                          /* RefersToEnclosingVariableOrCapture= */ true,
-                          /* GetNonReferenceType= */ true));
+                          /* RefersToEnclosingVariableOrCapture= */ true));
 
   // All arguments past first two ones are passed to the callback.
   for (unsigned int i = 2; i < D->getNumParams(); i++)
-    CallArgs.push_back(M.makeLvalueToRvalue(D->getParamDecl(i)));
+    CallArgs.push_back(
+        M.makeLvalueToRvalue(D->getParamDecl(i),
+                             /* RefersToEnclosingVariableOrCapture= */ false));
 
   CallExpr *CallbackCall;
   if (isLambdaCall) {
 
     CallbackCall =
         create_call_once_lambda_call(C, M, Callback, CallbackType, CallArgs);
   } else {
 
     // Function pointer case.
     CallbackCall = create_call_once_funcptr_call(C, M, Callback, CallArgs);
   }
 
   DeclRefExpr *FlagDecl =
       M.makeDeclRefExpr(Flag,
-                        /* RefersToEnclosingVariableOrCapture=*/true,
-                        /* GetNonReferenceType=*/true);
+                        /* RefersToEnclosingVariableOrCapture=*/true);
 
 
   MemberExpr *Deref = M.makeMemberExpression(FlagDecl, FlagFieldDecl);
   assert(Deref->isLValue());
   QualType DerefType = Deref->getType();
 
   // Negation predicate.
   UnaryOperator *FlagCheck = new (C) UnaryOperator(

test/Analysis/call_once.cpp

 } once_flag;
 #else
 typedef struct once_flag_s {
   int _M_once = 0;
 } once_flag;
 #endif
 
 template <class Callable, class... Args>
-void call_once(once_flag &o, Callable func, Args... args) {};
+void call_once(once_flag &o, Callable&& func, Args&&... args) {};
 
 } // namespace std
 
 // Check with Lambdas.
 void test_called_warning() {
   std::once_flag g_initialize;
   int z;