[Sema] Fix address-of + enable_if overloading logic

Previously, our logic when taking the address of an overloaded function
would not consider enable_if attributes, so long as all of the enable_if
conditions on a given candidate were true. So, two functions with
identical signatures (one with enable_if attributes, the other without),
would be considered equally good overloads. If we were calling the
function instead of taking its address, then the function with enable_if
attributes would be preferred.

This patch makes us prefer the candidate with enable_if regardless of if
we're calling or taking the address of an overloaded function.

Differential Revision: http://reviews.llvm.org/D13795

llvm-svn: 250486

clang/lib/Sema/SemaOverload.cpp

@@ -5855,23 +5855,31 @@ ObjCMethodDecl *Sema::SelectBestMethod(Selector Sel, MultiExprArg Args,
   return nullptr;
 }
 
-static bool IsNotEnableIfAttr(Attr *A) { return !isa<EnableIfAttr>(A); }
+// specific_attr_iterator iterates over enable_if attributes in reverse, and
+// enable_if is order-sensitive. As a result, we need to reverse things
+// sometimes. Size of 4 elements is arbitrary.
+static SmallVector<EnableIfAttr *, 4>
+getOrderedEnableIfAttrs(const FunctionDecl *Function) {
+  SmallVector<EnableIfAttr *, 4> Result;
+  if (!Function->hasAttrs())
+    return Result;
+
+  const auto &FuncAttrs = Function->getAttrs();
+  for (Attr *Attr : FuncAttrs)
+    if (auto *EnableIf = dyn_cast<EnableIfAttr>(Attr))
+      Result.push_back(EnableIf);
+
+  std::reverse(Result.begin(), Result.end());
+  return Result;
+}
 
 EnableIfAttr *Sema::CheckEnableIf(FunctionDecl *Function, ArrayRef<Expr *> Args,
                                   bool MissingImplicitThis) {
-  // FIXME: specific_attr_iterator<EnableIfAttr> iterates in reverse order, but
-  // we need to find the first failing one.
-  if (!Function->hasAttrs())
+  auto EnableIfAttrs = getOrderedEnableIfAttrs(Function);
+  if (EnableIfAttrs.empty())
     return nullptr;
-  AttrVec Attrs = Function->getAttrs();
-  AttrVec::iterator E = std::remove_if(Attrs.begin(), Attrs.end(),
-                                       IsNotEnableIfAttr);
-  if (Attrs.begin() == E)
-    return nullptr;
-  std::reverse(Attrs.begin(), E);
 
   SFINAETrap Trap(*this);
-
   SmallVector<Expr *, 16> ConvertedArgs;
   bool InitializationFailed = false;
   bool ContainsValueDependentExpr = false;
@@ -5908,7 +5916,7 @@ EnableIfAttr *Sema::CheckEnableIf(FunctionDecl *Function, ArrayRef<Expr *> Args,
   }
 
   if (InitializationFailed || Trap.hasErrorOccurred())
-    return cast<EnableIfAttr>(Attrs[0]);
+    return EnableIfAttrs[0];
 
   // Push default arguments if needed.
   if (!Function->isVariadic() && Args.size() < Function->getNumParams()) {
@@ -5929,12 +5937,11 @@ EnableIfAttr *Sema::CheckEnableIf(FunctionDecl *Function, ArrayRef<Expr *> Args,
     }
 
     if (InitializationFailed || Trap.hasErrorOccurred())
-      return cast<EnableIfAttr>(Attrs[0]);
+      return EnableIfAttrs[0];
   }
 
-  for (AttrVec::iterator I = Attrs.begin(); I != E; ++I) {
+  for (auto *EIA : EnableIfAttrs) {
     APValue Result;
-    EnableIfAttr *EIA = cast<EnableIfAttr>(*I);
     if (EIA->getCond()->isValueDependent()) {
       // Don't even try now, we'll examine it after instantiation.
       continue;
@@ -8397,6 +8404,44 @@ Sema::AddArgumentDependentLookupCandidates(DeclarationName Name,
   }
 }
 
+// Determines whether Cand1 is "better" in terms of its enable_if attrs than
+// Cand2 for overloading. This function assumes that all of the enable_if attrs
+// on Cand1 and Cand2 have conditions that evaluate to true.
+//
+// Cand1's set of enable_if attributes are said to be "better" than Cand2's iff
+// Cand1's first N enable_if attributes have precisely the same conditions as
+// Cand2's first N enable_if attributes (where N = the number of enable_if
+// attributes on Cand2), and Cand1 has more than N enable_if attributes.
+static bool hasBetterEnableIfAttrs(Sema &S, const FunctionDecl *Cand1,
+                                   const FunctionDecl *Cand2) {
+
+  // FIXME: The next several lines are just
+  // specific_attr_iterator<EnableIfAttr> but going in declaration order,
+  // instead of reverse order which is how they're stored in the AST.
+  auto Cand1Attrs = getOrderedEnableIfAttrs(Cand1);
+  auto Cand2Attrs = getOrderedEnableIfAttrs(Cand2);
+
+  // Candidate 1 is better if it has strictly more attributes and
+  // the common sequence is identical.
+  if (Cand1Attrs.size() <= Cand2Attrs.size())
+    return false;
+
+  auto Cand1I = Cand1Attrs.begin();
+  llvm::FoldingSetNodeID Cand1ID, Cand2ID;
+  for (auto &Cand2A : Cand2Attrs) {
+    Cand1ID.clear();
+    Cand2ID.clear();
+
+    auto &Cand1A = *Cand1I++;
+    Cand1A->getCond()->Profile(Cand1ID, S.getASTContext(), true);
+    Cand2A->getCond()->Profile(Cand2ID, S.getASTContext(), true);
+    if (Cand1ID != Cand2ID)
+      return false;
+  }
+
+  return true;
+}
+
 /// isBetterOverloadCandidate - Determines whether the first overload
 /// candidate is a better candidate than the second (C++ 13.3.3p1).
 bool clang::isBetterOverloadCandidate(Sema &S, const OverloadCandidate &Cand1,
@@ -8507,47 +8552,8 @@ bool clang::isBetterOverloadCandidate(Sema &S, const OverloadCandidate &Cand1,
   // Check for enable_if value-based overload resolution.
   if (Cand1.Function && Cand2.Function &&
       (Cand1.Function->hasAttr<EnableIfAttr>() ||
-       Cand2.Function->hasAttr<EnableIfAttr>())) {
-    // FIXME: The next several lines are just
-    // specific_attr_iterator<EnableIfAttr> but going in declaration order,
-    // instead of reverse order which is how they're stored in the AST.
-    AttrVec Cand1Attrs;
-    if (Cand1.Function->hasAttrs()) {
-      Cand1Attrs = Cand1.Function->getAttrs();
-      Cand1Attrs.erase(std::remove_if(Cand1Attrs.begin(), Cand1Attrs.end(),
-                                      IsNotEnableIfAttr),
-                       Cand1Attrs.end());
-      std::reverse(Cand1Attrs.begin(), Cand1Attrs.end());
-    }
-
-    AttrVec Cand2Attrs;
-    if (Cand2.Function->hasAttrs()) {
-      Cand2Attrs = Cand2.Function->getAttrs();
-      Cand2Attrs.erase(std::remove_if(Cand2Attrs.begin(), Cand2Attrs.end(),
-                                      IsNotEnableIfAttr),
-                       Cand2Attrs.end());
-      std::reverse(Cand2Attrs.begin(), Cand2Attrs.end());
-    }
-
-    // Candidate 1 is better if it has strictly more attributes and
-    // the common sequence is identical.
-    if (Cand1Attrs.size() <= Cand2Attrs.size())
-      return false;
-
-    auto Cand1I = Cand1Attrs.begin();
-    for (auto &Cand2A : Cand2Attrs) {
-      auto &Cand1A = *Cand1I++;
-      llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-      cast<EnableIfAttr>(Cand1A)->getCond()->Profile(Cand1ID,
-                                                     S.getASTContext(), true);
-      cast<EnableIfAttr>(Cand2A)->getCond()->Profile(Cand2ID,
-                                                     S.getASTContext(), true);
-      if (Cand1ID != Cand2ID)
-        return false;
-    }
-
-    return true;
-  }
+       Cand2.Function->hasAttr<EnableIfAttr>()))
+    return hasBetterEnableIfAttrs(S, Cand1.Function, Cand2.Function);
 
   if (S.getLangOpts().CUDA && S.getLangOpts().CUDATargetOverloads &&
       Cand1.Function && Cand2.Function) {
@@ -9986,7 +9992,7 @@ public:
     if (FindAllFunctionsThatMatchTargetTypeExactly()) {
       // C++ [over.over]p4:
       //   If more than one function is selected, [...]
-      if (Matches.size() > 1) {
+      if (Matches.size() > 1 && !eliminiateSuboptimalOverloadCandidates()) {
         if (FoundNonTemplateFunction)
           EliminateAllTemplateMatches();
         else
@@ -10002,6 +10008,36 @@ public:
   bool hasComplained() const { return HasComplained; }
 
 private:
+  // Is A considered a better overload candidate for the desired type than B?
+  bool isBetterCandidate(const FunctionDecl *A, const FunctionDecl *B) {
+    return hasBetterEnableIfAttrs(S, A, B);
+  }
+
+  // Returns true if we've eliminated any (read: all but one) candidates, false
+  // otherwise.
+  bool eliminiateSuboptimalOverloadCandidates() {
+    // Same algorithm as overload resolution -- one pass to pick the "best",
+    // another pass to be sure that nothing is better than the best.
+    auto Best = Matches.begin();
+    for (auto I = Matches.begin()+1, E = Matches.end(); I != E; ++I)
+      if (isBetterCandidate(I->second, Best->second))
+        Best = I;
+
+    const FunctionDecl *BestFn = Best->second;
+    auto IsBestOrInferiorToBest = [this, BestFn](
+        const std::pair<DeclAccessPair, FunctionDecl *> &Pair) {
+      return BestFn == Pair.second || isBetterCandidate(BestFn, Pair.second);
+    };
+
+    // Note: We explicitly leave Matches unmodified if there isn't a clear best
+    // option, so we can potentially give the user a better error
+    if (!std::all_of(Matches.begin(), Matches.end(), IsBestOrInferiorToBest))
+      return false;
+    Matches[0] = *Best;
+    Matches.resize(1);
+    return true;
+  }
+
   bool isTargetTypeAFunction() const {
     return TargetFunctionType->isFunctionType();
   }

clang/test/CodeGen/enable_if.c

@@ -49,3 +49,34 @@ void test2() {
   // CHECK: store i8* bitcast (void (i32)* @_Z3barUa9enable_ifIXLi1EEEi to i8*)
   vp1 = (void*)bar;
 }
+
+void baz(int m) __attribute__((overloadable, enable_if(1, "")));
+void baz(int m) __attribute__((overloadable));
+// CHECK-LABEL: define void @test3
+void test3() {
+  // CHECK: store void (i32)* @_Z3bazUa9enable_ifIXLi1EEEi
+  void (*p)(int) = baz;
+  // CHECK: store void (i32)* @_Z3bazUa9enable_ifIXLi1EEEi
+  void (*p2)(int) = &baz;
+  // CHECK: store void (i32)* @_Z3bazUa9enable_ifIXLi1EEEi
+  p = baz;
+  // CHECK: store void (i32)* @_Z3bazUa9enable_ifIXLi1EEEi
+  p = &baz;
+}
+
+
+const int TRUEFACTS = 1;
+void qux(int m) __attribute__((overloadable, enable_if(1, ""),
+                               enable_if(TRUEFACTS, "")));
+void qux(int m) __attribute__((overloadable, enable_if(1, "")));
+// CHECK-LABEL: define void @test4
+void test4() {
+  // CHECK: store void (i32)* @_Z3quxUa9enable_ifIXLi1EEXL_Z9TRUEFACTSEEEi
+  void (*p)(int) = qux;
+  // CHECK: store void (i32)* @_Z3quxUa9enable_ifIXLi1EEXL_Z9TRUEFACTSEEEi
+  void (*p2)(int) = &qux;
+  // CHECK: store void (i32)* @_Z3quxUa9enable_ifIXLi1EEXL_Z9TRUEFACTSEEEi
+  p = qux;
+  // CHECK: store void (i32)* @_Z3quxUa9enable_ifIXLi1EEXL_Z9TRUEFACTSEEEi
+  p = &qux;
+}

clang/test/CodeGenCXX/enable_if.cpp

@@ -1,4 +1,13 @@
 // RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-pc-linux-gnu| FileCheck %s
+
+// Test address-of overloading logic
+int test5(int);
+template <typename T>
+T test5(T) __attribute__((enable_if(1, "better than non-template")));
+
+// CHECK: @_Z5test5IiEUa9enable_ifIXLi1EEET_S0_
+int (*Ptr)(int) = &test5;
+
 // Test itanium mangling for attribute enable_if
 
 // CHECK: _Z5test1Ua9enable_ifIXeqfL0p_Li1EEEi

clang/test/Sema/enable_if.c

@@ -5,6 +5,8 @@
 typedef int mode_t;
 typedef unsigned long size_t;
 
+const int TRUE = 1;
+
 int open(const char *pathname, int flags) __attribute__((enable_if(!(flags & O_CREAT), "must specify mode when using O_CREAT"))) __attribute__((overloadable));  // expected-note{{candidate disabled: must specify mode when using O_CREAT}}
 int open(const char *pathname, int flags, mode_t mode) __attribute__((overloadable));  // expected-note{{candidate function not viable: requires 3 arguments, but 2 were provided}}
 
@@ -118,20 +120,20 @@ void test_return_cst() { return_cst(); }
 
 void f2(void) __attribute__((overloadable)) __attribute__((enable_if(1, "always chosen")));
 void f2(void) __attribute__((overloadable)) __attribute__((enable_if(0, "never chosen")));
-void f2(void) __attribute__((overloadable));
+void f2(void) __attribute__((overloadable)) __attribute__((enable_if(TRUE, "always chosen #2")));
 void test6() {
-  void (*p1)(void) = &f2; // expected-error{{initializing 'void (*)(void)' with an expression of incompatible type '<overloaded function type>'}} expected-note@119{{candidate function}} expected-note@120{{candidate function made ineligible by enable_if}} expected-note@121{{candidate function}}
-  void (*p2)(void) = f2; // expected-error{{initializing 'void (*)(void)' with an expression of incompatible type '<overloaded function type>'}} expected-note@119{{candidate function}} expected-note@120{{candidate function made ineligible by enable_if}} expected-note@121{{candidate function}}
-  void *p3 = (void*)&f2; // expected-error{{address of overloaded function 'f2' is ambiguous}} expected-note@119{{candidate function}} expected-note@120{{candidate function made ineligible by enable_if}} expected-note@121{{candidate function}}
-  void *p4 = (void*)f2; // expected-error{{address of overloaded function 'f2' is ambiguous}} expected-note@119{{candidate function}} expected-note@120{{candidate function made ineligible by enable_if}} expected-note@121{{candidate function}}
+  void (*p1)(void) = &f2; // expected-error{{initializing 'void (*)(void)' with an expression of incompatible type '<overloaded function type>'}} expected-note@121{{candidate function}} expected-note@122{{candidate function made ineligible by enable_if}} expected-note@123{{candidate function}}
+  void (*p2)(void) = f2; // expected-error{{initializing 'void (*)(void)' with an expression of incompatible type '<overloaded function type>'}} expected-note@121{{candidate function}} expected-note@122{{candidate function made ineligible by enable_if}} expected-note@123{{candidate function}}
+  void *p3 = (void*)&f2; // expected-error{{address of overloaded function 'f2' is ambiguous}} expected-note@121{{candidate function}} expected-note@122{{candidate function made ineligible by enable_if}} expected-note@123{{candidate function}}
+  void *p4 = (void*)f2; // expected-error{{address of overloaded function 'f2' is ambiguous}} expected-note@121{{candidate function}} expected-note@122{{candidate function made ineligible by enable_if}} expected-note@123{{candidate function}}
 }
 
 void f3(int m) __attribute__((overloadable)) __attribute__((enable_if(m >= 0, "positive")));
 void f3(int m) __attribute__((overloadable)) __attribute__((enable_if(m < 0, "negative")));
 void test7() {
-  void (*p1)(int) = &f3; // expected-error{{initializing 'void (*)(int)' with an expression of incompatible type '<overloaded function type>'}} expected-note@129{{candidate function made ineligible by enable_if}} expected-note@130{{candidate function made ineligible by enable_if}}
-  void (*p2)(int) = f3; // expected-error{{initializing 'void (*)(int)' with an expression of incompatible type '<overloaded function type>'}} expected-note@129{{candidate function made ineligible by enable_if}} expected-note@130{{candidate function made ineligible by enable_if}}
-  void *p3 = (void*)&f3; // expected-error{{address of overloaded function 'f3' does not match required type 'void'}} expected-note@129{{candidate function made ineligible by enable_if}} expected-note@130{{candidate function made ineligible by enable_if}}
-  void *p4 = (void*)f3; // expected-error{{address of overloaded function 'f3' does not match required type 'void'}} expected-note@129{{candidate function made ineligible by enable_if}} expected-note@130{{candidate function made ineligible by enable_if}}
+  void (*p1)(int) = &f3; // expected-error{{initializing 'void (*)(int)' with an expression of incompatible type '<overloaded function type>'}} expected-note@131{{candidate function made ineligible by enable_if}} expected-note@132{{candidate function made ineligible by enable_if}}
+  void (*p2)(int) = f3; // expected-error{{initializing 'void (*)(int)' with an expression of incompatible type '<overloaded function type>'}} expected-note@131{{candidate function made ineligible by enable_if}} expected-note@132{{candidate function made ineligible by enable_if}}
+  void *p3 = (void*)&f3; // expected-error{{address of overloaded function 'f3' does not match required type 'void'}} expected-note@131{{candidate function made ineligible by enable_if}} expected-note@132{{candidate function made ineligible by enable_if}}
+  void *p4 = (void*)f3; // expected-error{{address of overloaded function 'f3' does not match required type 'void'}} expected-note@131{{candidate function made ineligible by enable_if}} expected-note@132{{candidate function made ineligible by enable_if}}
 }
 #endif

clang/test/SemaCXX/enable_if.cpp

@@ -189,7 +189,7 @@ namespace FnPtrs {
   }
 
   int ovlConflict(int m) __attribute__((enable_if(true, "")));
-  int ovlConflict(int m);
+  int ovlConflict(int m) __attribute__((enable_if(1, "")));
   void test3() {
     int (*p)(int) = ovlConflict; // expected-error{{address of overloaded function 'ovlConflict' is ambiguous}} expected-note@191{{candidate function}} expected-note@192{{candidate function}}
     int (*p2)(int) = &ovlConflict; // expected-error{{address of overloaded function 'ovlConflict' is ambiguous}} expected-note@191{{candidate function}} expected-note@192{{candidate function}}
@@ -225,7 +225,7 @@ namespace FnPtrs {
   template <typename T>
   T templatedConflict(T m) __attribute__((enable_if(true, ""))) { return T(); }
   template <typename T>
-  T templatedConflict(T m) { return T(); }
+  T templatedConflict(T m) __attribute__((enable_if(1, ""))) { return T(); }
   void test6() {
     int (*p)(int) = templatedConflict<int>; // expected-error{{address of overloaded function 'templatedConflict' is ambiguous}} expected-note@224{{candidate function made ineligible by enable_if}} expected-note@226{{candidate function}} expected-note@228{{candidate function}}
     int (*p0)(int) = &templatedConflict<int>; // expected-error{{address of overloaded function 'templatedConflict' is ambiguous}} expected-note@224{{candidate function made ineligible by enable_if}} expected-note@226{{candidate function}} expected-note@228{{candidate function}}