[Bugfix] Disqualify unaddressable overload candidates when a function is directly called indirectly.

[george.burgess.iv]

Summary

"directly called indirectly" wasn't a typo; I can't think of a better name for this pattern. :)

Currently, Sema treats foo() and (&foo)() both as direct calls of foo, for the purposes of overload resolution. CodeGen treats the latter as an indirect call. This allows one to write code that indirectly calls an unaddressable function, which can result in ICEs (yay pass_object_size).

It seems that some code relies on how Sema resolves (&foo)(), so I don't think we can simply ask AddressOfFunctionResolver to handle this for us.

[george.burgess.iv]

Rebased

cfe/trunk/include/clang/Sema/Overload.h

enum OverloadFailureKind {
     /// template-argument deduction is supposed to ignore such
     /// instantiations, but we can still get one with the right
     /// kind of implicit instantiation.
     ovl_fail_illegal_constructor,
 
     /// This conversion candidate is not viable because its result
     /// type is not implicitly convertible to the desired type.
     ovl_fail_bad_final_conversion,
-    
-    /// This conversion function template specialization candidate is not 
+    /// This conversion function template specialization candidate is not
     /// viable because the final conversion was not an exact match.
     ovl_fail_final_conversion_not_exact,
 
     /// (CUDA) This candidate was not viable because the callee
     /// was not accessible from the caller's target (i.e. host->device,
     /// global->host, device->host).
     ovl_fail_bad_target,
 
     /// This candidate function was not viable because an enable_if
     /// attribute disabled it.
-    ovl_fail_enable_if
+    ovl_fail_enable_if,
+
+    /// This candidate was not viable because its address could not be taken.
+    ovl_fail_addr_not_available
   };
 
   /// OverloadCandidate - A single candidate in an overload set (C++ 13.3).
   struct OverloadCandidate {
     /// Function - The actual function that this candidate
     /// represents. When NULL, this is a built-in candidate
     /// (C++ [over.oper]) or a surrogate for a conversion to a
     /// function pointer or reference (C++ [over.call.object]).

cfe/trunk/include/clang/Sema/Sema.h

ForRangeStatus BuildForRangeBeginEndCall(SourceLocation Loc,
                                            Expr *Range, ExprResult *CallExpr);
 
   ExprResult BuildOverloadedCallExpr(Scope *S, Expr *Fn,
                                      UnresolvedLookupExpr *ULE,
                                      SourceLocation LParenLoc,
                                      MultiExprArg Args,
                                      SourceLocation RParenLoc,
                                      Expr *ExecConfig,
-                                     bool AllowTypoCorrection=true);
+                                     bool AllowTypoCorrection=true,
+                                     bool CalleesAddressIsTaken=false);
 
   bool buildOverloadedCallSet(Scope *S, Expr *Fn, UnresolvedLookupExpr *ULE,
                               MultiExprArg Args, SourceLocation RParenLoc,
                               OverloadCandidateSet *CandidateSet,
                               ExprResult *Result);
 
   ExprResult CreateOverloadedUnaryOp(SourceLocation OpLoc,
                                      UnaryOperatorKind Opc,

cfe/trunk/lib/Sema/SemaExpr.cpp

Sema::ActOnCallExpr(Scope *S, Expr *Fn, SourceLocation LParenLoc,
   if (Fn->getType() == Context.OverloadTy) {
     OverloadExpr::FindResult find = OverloadExpr::find(Fn);
 
     // We aren't supposed to apply this logic for if there's an '&' involved.
     if (!find.HasFormOfMemberPointer) {
       OverloadExpr *ovl = find.Expression;
       if (UnresolvedLookupExpr *ULE = dyn_cast<UnresolvedLookupExpr>(ovl))
         return BuildOverloadedCallExpr(S, Fn, ULE, LParenLoc, ArgExprs,
-                                       RParenLoc, ExecConfig);
+                                       RParenLoc, ExecConfig,
+                                       /*AllowTypoCorrection=*/true,
+                                       find.IsAddressOfOperand);
       return BuildCallToMemberFunction(S, Fn, LParenLoc, ArgExprs, RParenLoc);
     }
   }
 
   // If we're directly calling a function, get the appropriate declaration.
   if (Fn->getType() == Context.UnknownAnyTy) {
     ExprResult result = rebuildUnknownAnyFunction(*this, Fn);
     if (result.isInvalid()) return ExprError();
     Fn = result.get();
   }
 
   Expr *NakedFn = Fn->IgnoreParens();
 
+  bool CallingNDeclIndirectly = false;
   NamedDecl *NDecl = nullptr;
-  if (UnaryOperator *UnOp = dyn_cast<UnaryOperator>(NakedFn))
-    if (UnOp->getOpcode() == UO_AddrOf)
+  if (UnaryOperator *UnOp = dyn_cast<UnaryOperator>(NakedFn)) {
+    if (UnOp->getOpcode() == UO_AddrOf) {
+      CallingNDeclIndirectly = true;
       NakedFn = UnOp->getSubExpr()->IgnoreParens();
+    }
+  }
 
   if (isa<DeclRefExpr>(NakedFn)) {
     NDecl = cast<DeclRefExpr>(NakedFn)->getDecl();
 
     FunctionDecl *FDecl = dyn_cast<FunctionDecl>(NDecl);
     if (FDecl && FDecl->getBuiltinID()) {
       // Rewrite the function decl for this builtin by replacing parameters
       // with no explicit address space with the address space of the arguments
       // in ArgExprs.
       if ((FDecl = rewriteBuiltinFunctionDecl(this, Context, FDecl, ArgExprs))) {
         NDecl = FDecl;
         Fn = DeclRefExpr::Create(Context, FDecl->getQualifierLoc(),
                            SourceLocation(), FDecl, false,
                            SourceLocation(), FDecl->getType(),
                            Fn->getValueKind(), FDecl);
       }
     }
   } else if (isa<MemberExpr>(NakedFn))
     NDecl = cast<MemberExpr>(NakedFn)->getMemberDecl();
 
   if (FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(NDecl)) {
+    if (CallingNDeclIndirectly &&
+        !checkAddressOfFunctionIsAvailable(FD, /*Complain=*/true,
+                                           Fn->getLocStart()))
+      return ExprError();
+
     if (FD->hasAttr<EnableIfAttr>()) {
       if (const EnableIfAttr *Attr = CheckEnableIf(FD, ArgExprs, true)) {
         Diag(Fn->getLocStart(),
              isa<CXXMethodDecl>(FD) ?
                  diag::err_ovl_no_viable_member_function_in_call :
                  diag::err_ovl_no_viable_function_in_call)
           << FD << FD->getSourceRange();
         Diag(FD->getLocation(),

cfe/trunk/lib/Sema/SemaOverload.cpp

case ovl_fail_bad_conversion: {
     return S.NoteOverloadCandidate(Fn);
   }
 
   case ovl_fail_bad_target:
     return DiagnoseBadTarget(S, Cand);
 
   case ovl_fail_enable_if:
     return DiagnoseFailedEnableIfAttr(S, Cand);
+
+  case ovl_fail_addr_not_available: {
+    bool Available = checkAddressOfCandidateIsAvailable(S, Cand->Function);
+    (void)Available;
+    assert(!Available);
+    break;
+  }
   }
 }
 
 static void NoteSurrogateCandidate(Sema &S, OverloadCandidate *Cand) {
   // Desugar the type of the surrogate down to a function type,
   // retaining as many typedefs as possible while still showing
   // the function type (and, therefore, its parameter types).
   QualType FnType = Cand->Surrogate->getConversionType();
return SemaRef.BuildResolvedCallExpr(Fn, FDecl, LParenLoc, Args, RParenLoc,
                                          ExecConfig);
   }
   }
 
   // Overload resolution failed.
   return ExprError();
 }
 
+static void markUnaddressableCandidatesUnviable(Sema &S,
+                                                OverloadCandidateSet &CS) {
+  for (auto I = CS.begin(), E = CS.end(); I != E; ++I) {
+    if (I->Viable &&
+        !S.checkAddressOfFunctionIsAvailable(I->Function, /*Complain=*/false)) {
+      I->Viable = false;
+      I->FailureKind = ovl_fail_addr_not_available;
+    }
+  }
+}
+
 /// BuildOverloadedCallExpr - Given the call expression that calls Fn
 /// (which eventually refers to the declaration Func) and the call
 /// arguments Args/NumArgs, attempt to resolve the function call down
 /// to a specific function. If overload resolution succeeds, returns
 /// the call expression produced by overload resolution.
 /// Otherwise, emits diagnostics and returns ExprError.
 ExprResult Sema::BuildOverloadedCallExpr(Scope *S, Expr *Fn,
                                          UnresolvedLookupExpr *ULE,
                                          SourceLocation LParenLoc,
                                          MultiExprArg Args,
                                          SourceLocation RParenLoc,
                                          Expr *ExecConfig,
-                                         bool AllowTypoCorrection) {
+                                         bool AllowTypoCorrection,
+                                         bool CalleesAddressIsTaken) {
   OverloadCandidateSet CandidateSet(Fn->getExprLoc(),
                                     OverloadCandidateSet::CSK_Normal);
   ExprResult result;
 
   if (buildOverloadedCallSet(S, Fn, ULE, Args, LParenLoc, &CandidateSet,
                              &result))
     return result;
 
+  // If the user handed us something like `(&Foo)(Bar)`, we need to ensure that
+  // functions that aren't addressible are considered unviable.
+  if (CalleesAddressIsTaken)
+    markUnaddressableCandidatesUnviable(*this, CandidateSet);
+
   OverloadCandidateSet::iterator Best;
   OverloadingResult OverloadResult =
       CandidateSet.BestViableFunction(*this, Fn->getLocStart(), Best);
 
   return FinishOverloadedCallExpr(*this, S, Fn, ULE, LParenLoc, Args,
                                   RParenLoc, ExecConfig, &CandidateSet,
                                   &Best, OverloadResult,
                                   AllowTypoCorrection);

cfe/trunk/test/CodeGenCXX/pass-object-size.cpp

void Lambdas(char *ptr) {
   gi = L2(ptr);
 }
 
 // CHECK-DAG: define internal i64 @"_ZZN7lambdas7LambdasEPcENK3$_0clEPvU17pass_object_size0"
 // CHECK-NOT: call i64 @llvm.objectsize
 // CHECK-DAG: define internal i64 @"_ZZN7lambdas7LambdasEPcENK3$_1clEPvU17pass_object_size0"
 // CHECK-NOT: call i64 @llvm.objectsize
 }
+
+// This is here instead of in Sema/ because we need to check to make sure the
+// proper function is called. If it's not, we'll end up with assertion errors.
+namespace addrof {
+void OvlFoo(void *const __attribute__((pass_object_size(0)))) {}
+void OvlFoo(int *const) {}
+
+// CHECK: define void @_ZN6addrof4TestEv
+void Test() {
+  // Treating parens-only calls as though they were direct is consistent with
+  // how we handle other implicitly unaddressable functions (e.g. builtins).
+  // CHECK: call void @_ZN6addrof6OvlFooEPvU17pass_object_size0
+  (OvlFoo)(nullptr);
+
+  // CHECK: call void @_ZN6addrof6OvlFooEPi
+  (&OvlFoo)(nullptr);
+}
+}

cfe/trunk/test/Sema/pass-object-size.c

 void Overloaded3(void *p, void *p2 PS(0)) overloaded; //expected-error{{conflicting pass_object_size attributes on parameters}}
 
 void TakeFn(void (*)(void *));
 void TakeFnOvl(void (*)(void *)) overloaded;
 void TakeFnOvl(void (*)(int *)) overloaded;
 
 void NotOverloaded(void *p PS(0));
 void IsOverloaded(void *p PS(0)) overloaded;
-void IsOverloaded(char *p) overloaded;
+void IsOverloaded(char *p) overloaded; // char* inestead of void* is intentional
 void FunctionPtrs() {
   void (*p)(void *) = NotOverloaded; //expected-error{{cannot take address of function 'NotOverloaded' because parameter 1 has pass_object_size attribute}}
   void (*p2)(void *) = &NotOverloaded; //expected-error{{cannot take address of function 'NotOverloaded' because parameter 1 has pass_object_size attribute}}
 
   void (*p3)(void *) = IsOverloaded; //expected-error{{initializing 'void (*)(void *)' with an expression of incompatible type '<overloaded function type>'}} expected-note@-6{{candidate address cannot be taken because parameter 1 has pass_object_size attribute}} expected-note@-5{{type mismatch}}
   void (*p4)(void *) = &IsOverloaded; //expected-error{{initializing 'void (*)(void *)' with an expression of incompatible type '<overloaded function type>'}} expected-note@-7{{candidate address cannot be taken because parameter 1 has pass_object_size attribute}} expected-note@-6{{type mismatch}}
 
   void (*p5)(char *) = IsOverloaded;
   void (*p6)(char *) = &IsOverloaded;
 
   TakeFn(NotOverloaded); //expected-error{{cannot take address of function 'NotOverloaded' because parameter 1 has pass_object_size attribute}}
   TakeFn(&NotOverloaded); //expected-error{{cannot take address of function 'NotOverloaded' because parameter 1 has pass_object_size attribute}}
 
   TakeFnOvl(NotOverloaded); //expected-error{{cannot take address of function 'NotOverloaded' because parameter 1 has pass_object_size attribute}}
   TakeFnOvl(&NotOverloaded); //expected-error{{cannot take address of function 'NotOverloaded' because parameter 1 has pass_object_size attribute}}
+
+  int P;
+  (&NotOverloaded)(&P); //expected-error{{cannot take address of function 'NotOverloaded' because parameter 1 has pass_object_size attribute}}
+  (&IsOverloaded)(&P); //expected-error{{no matching function}} expected-note@35{{candidate address cannot be taken because parameter 1 has pass_object_size attribute}} expected-note@36{{candidate function not viable: no known conversion from 'int *' to 'char *' for 1st argument}}
 }

cfe/trunk/test/SemaCXX/pass-object-size.cpp

 void Foo(void *p __attribute__((pass_object_size(getObjSizeType()))));
 }
 
 namespace lambdas {
 void Bar() {
   (void)+[](void *const p __attribute__((pass_object_size(0)))) {}; //expected-error-re{{invalid argument type '(lambda at {{.*}})' to unary expression}}
 }
 }
+
+namespace ovlbug {
+// Directly calling an address-of function expression (e.g. in (&foo)(args...))
+// doesn't go through regular address-of-overload logic. This caused the above
+// code to generate an ICE.
+void DirectAddrOf(void *__attribute__((pass_object_size(0))));
+void DirectAddrOfOvl(void *__attribute__((pass_object_size(0))));
+void DirectAddrOfOvl(int *);
+
+void Test() {
+  (&DirectAddrOf)(nullptr); //expected-error{{cannot take address of function 'DirectAddrOf' because parameter 1 has pass_object_size attribute}}
+  (&DirectAddrOfOvl)((char*)nullptr); //expected-error{{no matching function}} expected-note@129{{candidate address cannot be taken because parameter 1 has pass_object_size attribute}} expected-note@130{{candidate function not viable: no known conversion from 'char *' to 'int *' for 1st argument}}
+}
+}