[Coroutines] Find custom allocators in class scope

[modocache]

Summary

https://reviews.llvm.org/rL325291 implemented Coroutines TS N4723
section [dcl.fct.def.coroutine]/7, but it performed lookup of allocator
functions within both the global and class scope, whereas the specified
behavior is to perform lookup for custom allocators within just the
class scope.

To fix, add parameters to the Sema::FindAllocationFunctions function
such that it can be used to lookup allocators in global scope,
class scope, or both (instead of just being able to look up in just global
scope or in both global and class scope). Then, use those parameters
from within the coroutine Sema.

This incorrect behavior had the unfortunate side-effect of causing the
bug https://bugs.llvm.org/show_bug.cgi?id=36578 (or at least the reports
of that bug in C++ programs). That bug would occur for any C++ user with
a coroutine frame that took a single pointer argument, since it would
then find the global placement form operator new, described in the
C++ standard 18.6.1.3.1. This patch prevents Clang from generating code
that triggers the LLVM assert described in that bug report.

Test Plan: check-clang

[GorNishanov]

LGTM with some stylistic suggestions

lib/Sema/SemaExprCXX.cpp

2355

drop else?

if (R.empty()) {
  if (NewScope == AFS_Class)
    return true;

  LookupQualifiedName(R, Context.getTranslationUnitDecl());
}

2406

drop else? so that it will read:

if (FoundDelete.empty()) {
  if (DeleteScope == AFS_Class)
    return true;

  DeclareGlobalNewDelete();
  LookupQualifiedName(FoundDelete, Context.getTranslationUnitDecl());
}

[modocache]

Thanks for the review, @GorNishanov! I adopted your suggestions and made the commit.

include/clang/Sema/Sema.h

ExprResult BuildCXXNew(SourceRange Range, bool UseGlobal,
                          QualType AllocType,
                          TypeSourceInfo *AllocTypeInfo,
                          Expr *ArraySize,
                          SourceRange DirectInitRange,
                          Expr *Initializer);
 
   bool CheckAllocatedType(QualType AllocType, SourceLocation Loc,
                           SourceRange R);
+
+  /// \brief The scope in which to find allocation functions.
+  enum AllocationFunctionScope {
+    /// \brief Only look for allocation functions in the global scope.
+    AFS_Global,
+    /// \brief Only look for allocation functions in the scope of the
+    /// allocated class.
+    AFS_Class,
+    /// \brief Look for allocation functions in both the global scope
+    /// and in the scope of the allocated class.
+    AFS_Both
+  };
+
+  /// \brief Finds the overloads of operator new and delete that are appropriate
+  /// for the allocation.
   bool FindAllocationFunctions(SourceLocation StartLoc, SourceRange Range,
-                               bool UseGlobal, QualType AllocType, bool IsArray,
+                               AllocationFunctionScope NewScope,
+                               AllocationFunctionScope DeleteScope,
+                               QualType AllocType, bool IsArray,
                                bool &PassAlignment, MultiExprArg PlaceArgs,
                                FunctionDecl *&OperatorNew,
                                FunctionDecl *&OperatorDelete,
                                bool Diagnose = true);
   void DeclareGlobalNewDelete();
   void DeclareGlobalAllocationFunction(DeclarationName Name, QualType Return,
                                        ArrayRef<QualType> Params);
 

lib/Sema/SemaCoroutine.cpp

for (auto *PD : FD.parameters()) {
     ExprResult PDRefExpr =
         S.BuildDeclRefExpr(PD, PD->getOriginalType().getNonReferenceType(),
                            ExprValueKind::VK_LValue, PDLoc);
     if (PDRefExpr.isInvalid())
       return false;
 
     PlacementArgs.push_back(PDRefExpr.get());
   }
-  S.FindAllocationFunctions(Loc, SourceRange(),
-                            /*UseGlobal*/ false, PromiseType,
+  S.FindAllocationFunctions(Loc, SourceRange(), /*NewScope*/ Sema::AFS_Class,
+                            /*DeleteScope*/ Sema::AFS_Both, PromiseType,
                             /*isArray*/ false, PassAlignment, PlacementArgs,
                             OperatorNew, UnusedResult, /*Diagnose*/ false);
 
   // [dcl.fct.def.coroutine]/7
   // "If no matching function is found, overload resolution is performed again
   // on a function call created by passing just the amount of space required as
   // an argument of type std::size_t."
   if (!OperatorNew && !PlacementArgs.empty()) {
     PlacementArgs.clear();
-    S.FindAllocationFunctions(Loc, SourceRange(),
-                              /*UseGlobal*/ false, PromiseType,
-                              /*isArray*/ false, PassAlignment,
-                              PlacementArgs, OperatorNew, UnusedResult);
+    S.FindAllocationFunctions(Loc, SourceRange(), /*NewScope*/ Sema::AFS_Class,
+                              /*DeleteScope*/ Sema::AFS_Both, PromiseType,
+                              /*isArray*/ false, PassAlignment, PlacementArgs,
+                              OperatorNew, UnusedResult, /*Diagnose*/ false);
+  }
+
+  // [dcl.fct.def.coroutine]/7
+  // "The allocation function’s name is looked up in the scope of P. If this
+  // lookup fails, the allocation function’s name is looked up in the global
+  // scope."
+  if (!OperatorNew) {
+    S.FindAllocationFunctions(Loc, SourceRange(), /*NewScope*/ Sema::AFS_Global,
+                              /*DeleteScope*/ Sema::AFS_Both, PromiseType,
+                              /*isArray*/ false, PassAlignment, PlacementArgs,
+                              OperatorNew, UnusedResult);
   }
 
   bool IsGlobalOverload =
       OperatorNew && !isa<CXXRecordDecl>(OperatorNew->getDeclContext());
   // If we didn't find a class-local new declaration and non-throwing new
   // was is required then we need to lookup the non-throwing global operator
   // instead.
   if (RequiresNoThrowAlloc && (!OperatorNew || IsGlobalOverload)) {
     auto *StdNoThrow = buildStdNoThrowDeclRef(S, Loc);
     if (!StdNoThrow)
       return false;
     PlacementArgs = {StdNoThrow};
     OperatorNew = nullptr;
-    S.FindAllocationFunctions(Loc, SourceRange(),
-                              /*UseGlobal*/ true, PromiseType,
+    S.FindAllocationFunctions(Loc, SourceRange(), /*NewScope*/ Sema::AFS_Both,
+                              /*DeleteScope*/ Sema::AFS_Both, PromiseType,
                               /*isArray*/ false, PassAlignment, PlacementArgs,
                               OperatorNew, UnusedResult);
   }
 
   if (!OperatorNew)
     return false;
 
   if (RequiresNoThrowAlloc) {

lib/Sema/SemaExprCXX.cpp

Sema::BuildCXXNew(SourceRange Range, bool UseGlobal,
   FunctionDecl *OperatorNew = nullptr;
   FunctionDecl *OperatorDelete = nullptr;
   unsigned Alignment =
       AllocType->isDependentType() ? 0 : Context.getTypeAlign(AllocType);
   unsigned NewAlignment = Context.getTargetInfo().getNewAlign();
   bool PassAlignment = getLangOpts().AlignedAllocation &&
                        Alignment > NewAlignment;
 
+  AllocationFunctionScope Scope = UseGlobal ? AFS_Global : AFS_Both;
   if (!AllocType->isDependentType() &&
       !Expr::hasAnyTypeDependentArguments(PlacementArgs) &&
       FindAllocationFunctions(StartLoc,
                               SourceRange(PlacementLParen, PlacementRParen),
-                              UseGlobal, AllocType, ArraySize, PassAlignment,
+                              Scope, Scope, AllocType, ArraySize, PassAlignment,
                               PlacementArgs, OperatorNew, OperatorDelete))
     return ExprError();
 
   // If this is an array allocation, compute whether the usual array
   // deallocation function for the type has a size_t parameter.
   bool UsualArrayDeleteWantsSize = false;
   if (ArraySize && !AllocType->isDependentType())
     UsualArrayDeleteWantsSize =
if (Diagnose) {
       Candidates.NoteCandidates(S, OCD_AllCandidates, Args);
     }
     return true;
   }
   }
   llvm_unreachable("Unreachable, bad result from BestViableFunction");
 }
 
-/// FindAllocationFunctions - Finds the overloads of operator new and delete
-/// that are appropriate for the allocation.
 bool Sema::FindAllocationFunctions(SourceLocation StartLoc, SourceRange Range,
-                                   bool UseGlobal, QualType AllocType,
-                                   bool IsArray, bool &PassAlignment,
-                                   MultiExprArg PlaceArgs,
+                                   AllocationFunctionScope NewScope,
+                                   AllocationFunctionScope DeleteScope,
+                                   QualType AllocType, bool IsArray,
+                                   bool &PassAlignment, MultiExprArg PlaceArgs,
                                    FunctionDecl *&OperatorNew,
                                    FunctionDecl *&OperatorDelete,
                                    bool Diagnose) {
   // --- Choosing an allocation function ---
   // C++ 5.3.4p8 - 14 & 18
-  // 1) If UseGlobal is true, only look in the global scope. Else, also look
-  //   in the scope of the allocated class.
+  // 1) If looking in AFS_Global scope for allocation functions, only look in
+  //    the global scope. Else, if AFS_Class, only look in the scope of the
+  //    allocated class. If AFS_Both, look in both.
   // 2) If an array size is given, look for operator new[], else look for
   //   operator new.
   // 3) The first argument is always size_t. Append the arguments from the
   //   placement form.
 
   SmallVector<Expr*, 8> AllocArgs;
   AllocArgs.reserve((PassAlignment ? 2 : 1) + PlaceArgs.size());
 
bool Sema::FindAllocationFunctions(SourceLocation StartLoc, SourceRange Range,
   {
     LookupResult R(*this, NewName, StartLoc, LookupOrdinaryName);
 
     // C++1z [expr.new]p9:
     //   If the new-expression begins with a unary :: operator, the allocation
     //   function's name is looked up in the global scope. Otherwise, if the
     //   allocated type is a class type T or array thereof, the allocation
     //   function's name is looked up in the scope of T.
-    if (AllocElemType->isRecordType() && !UseGlobal)
+    if (AllocElemType->isRecordType() && NewScope != AFS_Global)
       LookupQualifiedName(R, AllocElemType->getAsCXXRecordDecl());
 
     // We can see ambiguity here if the allocation function is found in
     // multiple base classes.
     if (R.isAmbiguous())
       return true;
 
     //   If this lookup fails to find the name, or if the allocated type is not
     //   a class type, the allocation function's name is looked up in the
     //   global scope.
-    if (R.empty())
+    if (R.empty()) {
+      if (NewScope == AFS_Class)
+        return true;
+
       LookupQualifiedName(R, Context.getTranslationUnitDecl());
+    }
 
     assert(!R.empty() && "implicitly declared allocation functions not found");
     assert(!R.isAmbiguous() && "global allocation functions are ambiguous");
 
     // We do our own custom access checks below.
     R.suppressDiagnostics();
 
     if (resolveAllocationOverload(*this, R, Range, AllocArgs, PassAlignment,
                                   OperatorNew, /*Candidates=*/nullptr,
                                   /*AlignArg=*/nullptr, Diagnose))
bool Sema::FindAllocationFunctions(SourceLocation StartLoc, SourceRange Range,
   //   If the new-expression begins with a unary :: operator, the
   //   deallocation function's name is looked up in the global
   //   scope. Otherwise, if the allocated type is a class type T or an
   //   array thereof, the deallocation function's name is looked up in
   //   the scope of T. If this lookup fails to find the name, or if
   //   the allocated type is not a class type or array thereof, the
   //   deallocation function's name is looked up in the global scope.
   LookupResult FoundDelete(*this, DeleteName, StartLoc, LookupOrdinaryName);
-  if (AllocElemType->isRecordType() && !UseGlobal) {
+  if (AllocElemType->isRecordType() && DeleteScope != AFS_Global) {
     CXXRecordDecl *RD
       = cast<CXXRecordDecl>(AllocElemType->getAs<RecordType>()->getDecl());
     LookupQualifiedName(FoundDelete, RD);
   }
   if (FoundDelete.isAmbiguous())
     return true; // FIXME: clean up expressions?
 
   bool FoundGlobalDelete = FoundDelete.empty();
   if (FoundDelete.empty()) {
+    if (DeleteScope == AFS_Class)
+      return true;
+
     DeclareGlobalNewDelete();
     LookupQualifiedName(FoundDelete, Context.getTranslationUnitDecl());
   }
 
   FoundDelete.suppressDiagnostics();
 
   SmallVector<std::pair<DeclAccessPair,FunctionDecl*>, 2> Matches;
 
   // Whether we're looking for a placement operator delete is dictated
   // by whether we selected a placement operator new, not by whether
   // we had explicit placement arguments.  This matters for things like
   //   struct A { void *operator new(size_t, int = 0); ... };

test/CodeGenCoroutines/coro-alloc.cpp

extern "C" void f1a(promise_matching_placement_new_tag, int x, float y , double z) {
   // CHECK: %[[SIZE:.+]] = call i64 @llvm.coro.size.i64()
   // CHECK: %[[INT:.+]] = load i32, i32* %x.addr, align 4
   // CHECK: %[[FLOAT:.+]] = load float, float* %y.addr, align 4
   // CHECK: %[[DOUBLE:.+]] = load double, double* %z.addr, align 8
   // CHECK: call i8* @_ZNSt12experimental16coroutine_traitsIJv34promise_matching_placement_new_tagifdEE12promise_typenwEmS1_ifd(i64 %[[SIZE]], i32 %[[INT]], float %[[FLOAT]], double %[[DOUBLE]])
   co_return;
 }
 
+// Declare a placement form operator new, such as the one described in
+// C++ 18.6.1.3.1, which takes a void* argument.
+void* operator new(SizeT __sz, void *__p) noexcept;
+
+struct promise_matching_global_placement_new_tag {};
+struct dummy {};
+template<>
+struct std::experimental::coroutine_traits<void, promise_matching_global_placement_new_tag, dummy*> {
+  struct promise_type {
+    void get_return_object() {}
+    suspend_always initial_suspend() { return {}; }
+    suspend_always final_suspend() { return {}; }
+    void return_void() {}
+  };
+};
+
+// A coroutine that takes a single pointer argument should not invoke this
+// placement form operator. [dcl.fct.def.coroutine]/7 dictates that lookup for
+// allocation functions matching the coroutine function's signature be done
+// within the scope of the promise type's class.
+// CHECK-LABEL: f1b(
+extern "C" void f1b(promise_matching_global_placement_new_tag, dummy *) {
+  // CHECK: call i8* @_Znwm(i64
+  co_return;
+}
+
 struct promise_delete_tag {};
 
 template<>
 struct std::experimental::coroutine_traits<void, promise_delete_tag> {
   struct promise_type {
     void operator delete(void*);
     void get_return_object() {}
     suspend_always initial_suspend() { return {}; }

test/SemaCXX/coroutines.cpp

struct good_promise_nonstatic_member_custom_new_operator {
   coro<good_promise_nonstatic_member_custom_new_operator> get_return_object();
   suspend_always initial_suspend();
   suspend_always final_suspend();
   void return_void();
   void unhandled_exception();
   void *operator new(SizeT, coroutine_nonstatic_member_struct &, double);
 };
 
-struct bad_promise_nonstatic_member_mismatched_custom_new_operator {
-  coro<bad_promise_nonstatic_member_mismatched_custom_new_operator> get_return_object();
-  suspend_always initial_suspend();
-  suspend_always final_suspend();
-  void return_void();
-  void unhandled_exception();
-  // expected-note@+1 {{candidate function not viable: requires 2 arguments, but 1 was provided}}
-  void *operator new(SizeT, double);
-};
-
-struct coroutine_nonstatic_member_struct {
-  coro<good_promise_nonstatic_member_custom_new_operator>
-  good_coroutine_calls_nonstatic_member_custom_new_operator(double) {
-    co_return;
-  }
-
-  coro<bad_promise_nonstatic_member_mismatched_custom_new_operator>
-  bad_coroutine_calls_nonstatic_member_mistmatched_custom_new_operator(double) {
-    // expected-error@-1 {{no matching function for call to 'operator new'}}
-    co_return;
-  }
-};
-
-struct bad_promise_mismatched_custom_new_operator {
-  coro<bad_promise_mismatched_custom_new_operator> get_return_object();
-  suspend_always initial_suspend();
-  suspend_always final_suspend();
-  void return_void();
-  void unhandled_exception();
-  // expected-note@+1 {{candidate function not viable: requires 4 arguments, but 1 was provided}}
-  void *operator new(SizeT, double, float, int);
-};
-
-coro<bad_promise_mismatched_custom_new_operator>
-bad_coroutine_calls_mismatched_custom_new_operator(double) {
-  // expected-error@-1 {{no matching function for call to 'operator new'}}
-  co_return;
-}
-
-struct bad_promise_throwing_custom_new_operator {
-  static coro<bad_promise_throwing_custom_new_operator> get_return_object_on_allocation_failure();
-  coro<bad_promise_throwing_custom_new_operator> get_return_object();
-  suspend_always initial_suspend();
-  suspend_always final_suspend();
-  void return_void();
-  void unhandled_exception();
-  // expected-error@+1 {{'operator new' is required to have a non-throwing noexcept specification when the promise type declares 'get_return_object_on_allocation_failure()'}}
-  void *operator new(SizeT, double, float, int);
-};
-
-coro<bad_promise_throwing_custom_new_operator>
-bad_coroutine_calls_throwing_custom_new_operator(double, float, int) {
-  // expected-note@-1 {{call to 'operator new' implicitly required by coroutine function here}}
-  co_return;
-}
-
 struct good_promise_noexcept_custom_new_operator {
   static coro<good_promise_noexcept_custom_new_operator> get_return_object_on_allocation_failure();
   coro<good_promise_noexcept_custom_new_operator> get_return_object();
   suspend_always initial_suspend();
   suspend_always final_suspend();
   void return_void();
   void unhandled_exception();
   void *operator new(SizeT, double, float, int) noexcept;