[Fix] Allow implicit conversions of the address of overloadable functions in C + docs update

[george.burgess.iv]

Summary

Two smallish patches in one. Happy to split into two (for review and/or commit) if that's preferred.

In C, we allow (as an extension) incompatible pointer conversions, like so:

int foo(int *);
int (*p)(void *) = foo; // Compiles; warns if -Wincompatible-pointer-types is specified

So, we should provide primitive support for this with overloadable functions. Specifically, this patch is trying to fix cases like this:

int foo(void *) __attribute__((overloadable));
int foo(double *d) __attribute__((overloadable, enable_if(d != nullptr, "")));

int (*p)(int *) = foo; // Before patch, error because we couldn't match the function type exactly. After patch, we see that 'int foo(void *)' is the only option, and choose it. A warning is emitted if -Wincompatible-pointer-types is given

Which means that explicit casts are necessary if there's more than one overload that isn't disabled by enable_if attrs. The updated docs have an example of this, but here's another:

int foo(void *) __attribute__((overloadable));
int foo(double *d) __attribute__((overloadable));

int (*p)(int *) = foo; // 0 candidates before patch, 2 after. Won't compile either way.
int (*p2)(int *) = (int (*)(void *))foo; // OK. Issues warning if -Wincompatible-pointer-types is given

[george.burgess.iv]

Rebased

[george.burgess.iv]

FYI: I noticed a few cases this patch misses. Will add them soon.

[george.burgess.iv]

• Reworded docs update
• Added support for reinterpret_casts of overloaded functions in both C and C++
• Added general facility in Sema to query if an overloaded expression can resolve to being not-overloaded on its own (e.g. if it's an address-of function expression *and* it's the only addressable overload with the given name)

I made it generally available because there's a problem that this patch doesn't fix (will submit as separate phabricator review), and I suspect that this new functionality will be useful for solving that problem. Namely, the problem is template argument deduction (joy):

template <typename Fn> int foo(Fn);

void bar();
void bar(int) __attribute__((enable_if(0, "")));
int a = foo(bar); // cannot deduce `Fn` because `bar` is an overload.

[george.burgess.iv]

Rebased, and narrowed the scope of the patch a bit.

[rsmith]

lib/Sema/SemaOverload.cpp

10419 ↗

(On Diff #49333)

Why is the void* check removed from this case? Note that clang and GCC intentionally treat these two cases differently today:

int f();
void *p = f; // ok (warning under -pedantic)
int *q = f; // warning: incompatible pointer types

(That is: the first is a silent-by-default extension and the second is a warn-by-default extension.)

[george.burgess.iv]

lib/Sema/SemaOverload.cpp

10419 ↗

(On Diff #49333)

Because this is overload resolution logic, so we shouldn't care about what warnings we will emit :)

This is how we act prior to applying this patch:

void f(int) __attribute__((overloadable));
void f(double) __attribute__((overloadable, enable_if(0, "")));

void *fp = f; // OK. This is C and the target is void*.
void (*fp)(void) = f; // Error. This is C, but the target isn't void*.

I'm simply removing the "the target must be a void*" restriction; the user should still get warnings in the latter case (the tests changed in test/Sema/pass-object-size.c make sure of this).

[rsmith]

lib/Sema/SemaOverload.cpp

10419 ↗

(On Diff #49333)

OK, this seems fine so long as we somewhere choose exact matches over inexact ones:

void f(int) __attribute__((overloadable));
void f(int, int) __attribute__((overloadable));
void g(void (*)(int));
void h() { g(f); } // should pick f(int)

[george.burgess.iv]

lib/Sema/SemaOverload.cpp

10419 ↗

(On Diff #49333)

We do; the tests in test/CodeGen/overloadable.c make sure of this -- thanks for the review! :)

cfe/trunk/include/clang/Basic/AttrDocs.td

 In this example, a call to f() is always resolved to #2, as the first enable_if
 expression is ODR-equivalent for both declarations, but #1 does not have another
 enable_if expression to continue evaluating, so the next round of evaluation has
 only a single candidate. In a call to g(1, 1), the call is ambiguous even though
 #2 has more enable_if attributes, because the first enable_if expressions are
 not ODR-equivalent.
 
 Query for this feature with ``__has_attribute(enable_if)``.
+
+Note that functions with one or more ``enable_if`` attributes may not have
+their address taken, unless all of the conditions specified by said
+``enable_if`` are constants that evaluate to ``true``. For example:
+
+.. code-block:: c
+
+  const int TrueConstant = 1;
+  const int FalseConstant = 0;
+  int f(int a) __attribute__((enable_if(a > 0, "")));
+  int g(int a) __attribute__((enable_if(a == 0 || a != 0, "")));
+  int h(int a) __attribute__((enable_if(1, "")));
+  int i(int a) __attribute__((enable_if(TrueConstant, "")));
+  int j(int a) __attribute__((enable_if(FalseConstant, "")));
+
+  void fn() {
+    int (*ptr)(int);
+    ptr = &f; // error: 'a > 0' is not always true
+    ptr = &g; // error: 'a == 0 || a != 0' is not a truthy constant
+    ptr = &h; // OK: 1 is a truthy constant
+    ptr = &i; // OK: 'TrueConstant' is a truthy constant
+    ptr = &j; // error: 'FalseConstant' is a constant, but not truthy
+  }
   }];
 }
 
 def PassObjectSizeDocs : Documentation {
   let Category = DocCatVariable; // Technically it's a parameter doc, but eh.
   let Content = [{
 .. Note:: The mangling of functions with parameters that are annotated with
   ``pass_object_size`` is subject to change. You can get around this by

cfe/trunk/lib/Sema/SemaOverload.cpp

 // 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,
+static bool hasBetterEnableIfAttrs(const 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);
 
AddressOfFunctionResolver(Sema &S, Expr *SourceExpr,
     if (S.getLangOpts().CUDA && S.getLangOpts().CUDATargetOverloads &&
         Matches.size() > 1)
       EliminateSuboptimalCudaMatches();
   }
 
   bool hasComplained() const { return HasComplained; }
 
 private:
-  // Is A considered a better overload candidate for the desired type than B?
+  bool candidateHasExactlyCorrectType(const FunctionDecl *FD) {
+    QualType Discard;
+    return Context.hasSameUnqualifiedType(TargetFunctionType, FD->getType()) ||
+           S.IsNoReturnConversion(FD->getType(), TargetFunctionType, Discard);
+  }
+
+  /// \return true if A is considered a better overload candidate for the
+  /// desired type than B.
   bool isBetterCandidate(const FunctionDecl *A, const FunctionDecl *B) {
-    return hasBetterEnableIfAttrs(S, A, B);
+    // If A doesn't have exactly the correct type, we don't want to classify it
+    // as "better" than anything else. This way, the user is required to
+    // disambiguate for us if there are multiple candidates and no exact match.
+    return candidateHasExactlyCorrectType(A) &&
+           (!candidateHasExactlyCorrectType(B) ||
+            hasBetterEnableIfAttrs(S, A, B));
   }
 
-  // Returns true if we've eliminated any (read: all but one) candidates, false
-  // otherwise.
+  /// \return true if we were able to eliminate all but one overload candidate,
+  /// 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;
 
if (FunctionDecl *FunDecl = dyn_cast<FunctionDecl>(Fn)) {
           S.DeduceReturnType(FunDecl, SourceExpr->getLocStart(), Complain)) {
         HasComplained |= Complain;
         return false;
       }
 
       if (!S.checkAddressOfFunctionIsAvailable(FunDecl))
         return false;
 
-      QualType ResultTy;
-      if (Context.hasSameUnqualifiedType(TargetFunctionType,
-                                         FunDecl->getType()) ||
-          S.IsNoReturnConversion(FunDecl->getType(), TargetFunctionType,
-                                 ResultTy) ||
-          (!S.getLangOpts().CPlusPlus && TargetType->isVoidPointerType())) {
+      // If we're in C, we need to support types that aren't exactly identical.
+      if (!S.getLangOpts().CPlusPlus ||
+          candidateHasExactlyCorrectType(FunDecl)) {
         Matches.push_back(std::make_pair(
             CurAccessFunPair, cast<FunctionDecl>(FunDecl->getCanonicalDecl())));
         FoundNonTemplateFunction = true;
         return true;
       }
     }
     
     return false;

cfe/trunk/test/CodeGen/overloadable.c

int main() {
   v4hi vv;
 
   iv = f(iv);
   fv = f(fv);
   dv = f(dv);
   cdv = f(cdv);
   vv = f(vv);
 }
+
+// Ensuring that we pick the correct function for taking the address of an
+// overload when conversions are involved.
+
+void addrof_many(int *a) __attribute__((overloadable, enable_if(0, "")));
+void addrof_many(void *a) __attribute__((overloadable));
+void addrof_many(char *a) __attribute__((overloadable));
+
+void addrof_single(int *a) __attribute__((overloadable, enable_if(0, "")));
+void addrof_single(char *a) __attribute__((overloadable, enable_if(0, "")));
+void addrof_single(char *a) __attribute__((overloadable));
+
+// CHECK-LABEL: define void @foo
+void foo() {
+  // CHECK: store void (i8*)* @_Z11addrof_manyPc
+  void (*p1)(char *) = &addrof_many;
+  // CHECK: store void (i8*)* @_Z11addrof_manyPv
+  void (*p2)(void *) = &addrof_many;
+  // CHECK: void (i8*)* @_Z11addrof_manyPc
+  void *vp1 = (void (*)(char *)) & addrof_many;
+  // CHECK: void (i8*)* @_Z11addrof_manyPv
+  void *vp2 = (void (*)(void *)) & addrof_many;
+
+  // CHECK: store void (i8*)* @_Z13addrof_singlePc
+  void (*p3)(char *) = &addrof_single;
+  // CHECK: @_Z13addrof_singlePc
+  void (*p4)(int *) = &addrof_single;
+  // CHECK: @_Z13addrof_singlePc
+  void *vp3 = &addrof_single;
+}

cfe/trunk/test/Sema/overloadable.c

-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s -Wincompatible-pointer-types
 
 int var __attribute__((overloadable)); // expected-error{{'overloadable' attribute only applies to functions}}
 void params(void) __attribute__((overloadable(12))); // expected-error {{'overloadable' attribute takes no arguments}}
 
 int *f(int) __attribute__((overloadable)); // expected-note 2{{previous overload of function is here}}
 float *f(float); // expected-error{{overloaded function 'f' must have the 'overloadable' attribute}}
 int *f(int); // expected-error{{redeclaration of 'f' must have the 'overloadable' attribute}} \
              // expected-note{{previous declaration is here}}
void conversions() {
   void *ptr;
   foo(ptr);
 
   void multi_type(unsigned char *c) __attribute__((overloadable));
   void multi_type(signed char *c) __attribute__((overloadable));
   unsigned char *c;
   multi_type(c);
 }
+
+// Ensure that we allow C-specific type conversions in C
+void fn_type_conversions() {
+  void foo(void *c) __attribute__((overloadable));
+  void foo(char *c) __attribute__((overloadable));
+  void (*ptr1)(void *) = &foo;
+  void (*ptr2)(char *) = &foo;
+  void (*ambiguous)(int *) = &foo; // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type '<overloaded function type>'}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+  void *vp_ambiguous = &foo; // expected-error{{initializing 'void *' with an expression of incompatible type '<overloaded function type>'}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+
+  void (*specific1)(int *) = (void (*)(void *))&foo; // expected-warning{{incompatible pointer types initializing 'void (*)(int *)' with an expression of type 'void (*)(void *)'}}
+  void *specific2 = (void (*)(void *))&foo;
+
+  void disabled(void *c) __attribute__((overloadable, enable_if(0, "")));
+  void disabled(int *c) __attribute__((overloadable, enable_if(c, "")));
+  void disabled(char *c) __attribute__((overloadable, enable_if(1, "The function name lies.")));
+  // To be clear, these should all point to the last overload of 'disabled'
+  void (*dptr1)(char *c) = &disabled;
+  void (*dptr2)(void *c) = &disabled; // expected-warning{{incompatible pointer types initializing 'void (*)(void *)' with an expression of type '<overloaded function type>'}} expected-note@115{{candidate function made ineligible by enable_if}} expected-note@116{{candidate function made ineligible by enable_if}} expected-note@117{{candidate function has type mismatch at 1st parameter (expected 'void *' but has 'char *')}}
+  void (*dptr3)(int *c) = &disabled; // expected-warning{{incompatible pointer types initializing 'void (*)(int *)' with an expression of type '<overloaded function type>'}} expected-note@115{{candidate function made ineligible by enable_if}} expected-note@116{{candidate function made ineligible by enable_if}} expected-note@117{{candidate function has type mismatch at 1st parameter (expected 'int *' but has 'char *')}}
+
+  void *specific_disabled = &disabled;
+}

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

 
 void NotOverloaded(void *p PS(0));
 void IsOverloaded(void *p PS(0)) 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 (*p3)(void *) = IsOverloaded; //expected-warning{{incompatible pointer types initializing 'void (*)(void *)' with an expression of 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-warning{{incompatible pointer types initializing 'void (*)(void *)' with an expression of 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}}
 }