Add support for transparent overloadable functions in clang

[george.burgess.iv]

Summary

One of the common use-cases for the overloadable attribute is to create small
wrapper functions around an existing function that was previously not
overloadable. For example:

// C Library v1
void foo(int i);


// C Library v2
// Note the __asm__("foo"); it ensures that foo isn't mangled. Our docs state
// that the mangling isn't stable, so we encourage users to either use __asm__
// to force a stable name, or to only use overloads with internal linkage
__attribute__((overloadable))
int foo(int i) __asm__("foo");

__attribute__((overloadable))
static inline int foo(long l) {
  assert(l <= INT_MAX && l >= INT_MIN && "Out of bounds number!");
  return foo(l);
}

"Library v2" is suboptimal for a few reasons:

• It can break existing code; if users added int foo(int); declarations to their code, upgrading to library v2 requires that the users either remove these declarations or add __attribute__((overloadable)) to them.
• The __asm__ name is both redundant and easy to typo (or forget to update, or ...).
• The __asm__ name also makes it quite a bit more difficult to hide all of this behind a macro, since we need a different name for each function.

This patch aims to fix these usability challenges with the idea of transparent
function overloads.

For any overloadable function in C, one of the overloads is allowed to be
transparent. Transparency means that the name will not be mangled, and later
redeclarations of that particular overload don't require
__attribute__((overloadable)).

Users can make an overload transparent by using the transparently_overloadable
spelling of overloadable.

This also includes a bugfix for how we handled overloadable functions declared
inside functions. I can factor that out if that would make life easier. :)

[george.burgess.iv]

ping :)

[aaron.ballman]

I'm adding Richard to the review because he may have opinions on this functionality.

Is there a reason we want the second spelling instead of making the current spelling behave in the manner you describe? While the change is fairly fundamental, I don't know that it would break code either (but I could be wrong). I think that forcing the user to choice which spelling of "overloadable" they want to use is not very user friendly.

include/clang/Basic/Attr.td

1416 ↗

(On Diff #96074)

I'm not too keen on the name, especially since there's already a "transparent_union" attribute, where the "transparent" means something slightly different than it does here. However, my attempts to dream up a better name are coming up short...

include/clang/Basic/DiagnosticSemaKinds.td

3259 ↗

(On Diff #96074)

Why should this be an error instead of simply an ignored attribute?

test/Sema/overloadable.c

189 ↗

(On Diff #96074)

Why should this be a mismatch? Attributes can usually be added to redeclarations without an issue, and it's not unheard of for subsequent redeclarations to gain additional attributes. It seems like the behavior the user would expect from this is for the transparently_overloadable attribute to "win" and simply replaces, or silently augments, the overloadable attribute.

[george.burgess.iv]

thanks for the feedback!

fwiw, at a high level, the main problem i'm trying to solve with this is that you can't really make a __overloadable_without_mangling macro without handing it the function name, since you currently need to use __asm__("name-you'd-like-the-function-to-have") to rename the function. if you can think of a better way to go about this, even if it requires that we drop the "no overloadable required on some redeclarations" feature this adds, i'm all ears. :)

Is there a reason we want the second spelling instead of making the current spelling behave in the manner you describe?

there are two features this is adding, and i'm not sure which you want overloadable to imply.

1. this new spelling brings with it the promise that we won't mangle function names; if we made every overloadable act this way, we'd have:

void foo(int) __attribute__((overloadable)); // emitted as `declare void @foo(i32)`
void foo(long) __attribute__((overloadable)); // emitted as `declare void @foo(i64)`. LLVM gets angry, since we have different declarations for @foo.

the only way i can think of to get "no indication of which overloads aren't mangled" to work is choosing which overload to mangle based on source order, and that seems overly subtle to me. so, i think this behavior needs to be spelled out in the source.

2. for the "no overloadable required on redecls" feature this adds: i wasn't there for the initial design of the overloadable attribute, so i can't say for certain, but i think part of the reason that we required overloadable to be on every redecl was so that people would have an indication of "hey, by the way, this name will probably be mangled and there may be other functions you end up calling." in C, i can totally see that being valuable. if you're suggesting we relax that for all overloads, i'll find who originally implemented all of this to figure out what their reasoning was and get back to you. :)

---

in any case, whether we actually do this as a new spelling, an optional argument to overloadable, etc. makes no difference to me. i chose a new spelling because AFAICT, we don't currently have a standard way for a user to query clang for if it has support for specific features in an attribute. (outside of checking clang's version, but that's discouraged AFAIK.) if we decide an optional arg would be a better approach than a new spelling, i'm happy to add something like __has_attribute_ext so you can do __has_attribute_ext(overloadable, transparent_overloads).

I think that forcing the user to choice which spelling of "overloadable" they want to use is not very user friendly.

yeah. the idea was that users should only need to reach for transparently_overloadable if they're trying to make a previously non-overloadable function overloadable. like said, if you think there's a better way to surface this functionality, i'm all for it.

include/clang/Basic/Attr.td

1416 ↗

(On Diff #96074)

yeah :/

my only other idea was implicitly_overloadable. maybe that would be preferable?

include/clang/Basic/DiagnosticSemaKinds.td

3259 ↗

(On Diff #96074)

if the user asks for us to not mangle two overloads with the same name, i don't think we can (sanely) go to CodeGen. see below.

test/Sema/overloadable.c

189 ↗

(On Diff #96074)

my issue with this was:

// foo.h
void foo(int) __attribute__((overloadable));

// foo.c
void foo(int) __attribute__((overloadable)) {}

// bar.c
#include "foo.h"

void foo(int) __attribute__((transparently_overloadable));

// calls to foo(int) now silently call @foo instead of the mangled version, but only in this TU

though, i suppose this code going against our guidance of "overloads should generally have internal linkage", and it's already possible to get yourself in a similar situation today. so, as long as we don't allow overloadable to "win" against transparently_overloadable, i'm OK with this.

[aaron.ballman]

In D32332#742796, @george.burgess.iv wrote:

thanks for the feedback!

fwiw, at a high level, the main problem i'm trying to solve with this is that you can't really make a __overloadable_without_mangling macro without handing it the function name, since you currently need to use __asm__("name-you'd-like-the-function-to-have") to rename the function. if you can think of a better way to go about this, even if it requires that we drop the "no overloadable required on some redeclarations" feature this adds, i'm all ears. :)

Yeah, it would require the function name (then you could use the stringize preprocessor directive).

Is there a reason we want the second spelling instead of making the current spelling behave in the manner you describe?

there are two features this is adding, and i'm not sure which you want overloadable to imply.

1. this new spelling brings with it the promise that we won't mangle function names; if we made every overloadable act this way, we'd have:

void foo(int) __attribute__((overloadable)); // emitted as `declare void @foo(i32)`
void foo(long) __attribute__((overloadable)); // emitted as `declare void @foo(i64)`. LLVM gets angry, since we have different declarations for @foo.

the only way i can think of to get "no indication of which overloads aren't mangled" to work is choosing which overload to mangle based on source order, and that seems overly subtle to me. so, i think this behavior needs to be spelled out in the source.

I'd like to understand this use case a little bit better. If you don't perform the mangling, then choosing which overload gets called is already based on the source order, isn't it? See https://godbolt.org/g/8b10Ns

2. for the "no overloadable required on redecls" feature this adds: i wasn't there for the initial design of the overloadable attribute, so i can't say for certain, but i think part of the reason that we required overloadable to be on every redecl was so that people would have an indication of "hey, by the way, this name will probably be mangled and there may be other functions you end up calling." in C, i can totally see that being valuable. if you're suggesting we relax that for all overloads, i'll find who originally implemented all of this to figure out what their reasoning was and get back to you. :)

The logic as to why overloadable is required on every redeclaration makes sense, but also flies in the face of your rational for why you want a new spelling of the same attribute that doesn't need to be on every redeclaration. I worry about this difference being mildly confusing to user.

in any case, whether we actually do this as a new spelling, an optional argument to overloadable, etc. makes no difference to me. i chose a new spelling because AFAICT, we don't currently have a standard way for a user to query clang for if it has support for specific features in an attribute. (outside of checking clang's version, but that's discouraged AFAIK.) if we decide an optional arg would be a better approach than a new spelling, i'm happy to add something like __has_attribute_ext so you can do __has_attribute_ext(overloadable, transparent_overloads).

That's a fair reason for a new spelling rather than an argument.

I think that forcing the user to choice which spelling of "overloadable" they want to use is not very user friendly.

yeah. the idea was that users should only need to reach for transparently_overloadable if they're trying to make a previously non-overloadable function overloadable. like said, if you think there's a better way to surface this functionality, i'm all for it.

[george.burgess.iv]

I'd like to understand this use case a little bit better. If you don't perform the mangling, then choosing which overload gets called is already based on the source order, isn't it? See https://godbolt.org/g/8b10Ns

I think I might have miscommunicated: transparently_overloadable only enforces no mangling on one function out of the set of functions with the same name. So,

void foo(int) __attribute__((transparently_overloadable));
void foo(float) __attribute__((overloadable));

void bar() {
  foo(1); // calls @foo
  foo(1f); // calls @_Z3foof
}

The logic as to why overloadable is required on every redeclaration makes sense, but also flies in the face of your rational for why you want a new spelling of the same attribute that doesn't need to be on every redeclaration. I worry about this difference being mildly confusing to user.

Yeah, good point. The more I think about that, the more I don't like the inconsistency; I'll drop that change.

[george.burgess.iv]

We now require transparently_overloadable on all redeclarations. Allowing mixed transparently_overloadable and overloadable coming soon.

[aaron.ballman]

lib/AST/ItaniumMangle.cpp

583 ↗

(On Diff #97698)

const auto *

lib/AST/MicrosoftMangle.cpp

372 ↗

(On Diff #97698)

const auto *

lib/Sema/SemaDecl.cpp

2820 ↗

(On Diff #97698)

const auto *

2921 ↗

(On Diff #97698)

Can NewOvl be null here (in an error case)?

9303 ↗

(On Diff #97698)

const auto *

9306–9307 ↗

(On Diff #97698)

const auto * in both places.

test/Sema/overloadable.c

189 ↗

(On Diff #96074)

Hmm, I can see how your example might cause confusion for the user as well. Perhaps downgrading it from an error to a warning and maybe putting something in the docs about why that warning could lead to bad things would be a good approach?

[george.burgess.iv]

• Addressed all feedback
• Now we emit a warning when transparently_overloadable "overrides" overloadable, rather than an error

lib/Sema/SemaDecl.cpp

2921 ↗

(On Diff #97698)

If it's null here, then the bug is in another piece of code.

In theory, after one FunctionDecl with some name N is tagged with overloadable, all proceeding declarations/definition(s) with the name N should be tagged with overloadable. We'll make implicit OverloadableAttrs if the user fails to do this (fixMissingOverloadableAttr).

...Though, this code doesn't do a good job of calling that out. I tried adding a note of this to Sema::CheckFunctionDeclaration; I dunno if there's a better place to put it.

Regardless, added an assert here to clarify. :)

test/Sema/overloadable.c

189 ↗

(On Diff #96074)

Done.

I wasn't sure what you thought was best in the case of

void foo(int) __attribute__((overloadable));
void foo(int) __attribute__((transparently_overloadable));
void foo(int) __attribute__((overloadable));

so I made clang emit an error on the last redeclaration of foo because it's not transparently_overloadable. If you'd prefer for us to silently turn the last overloadable into transparently_overloadable, I'm happy to allow that, too.

(In any case, we'll now warn about going from overloadable -> transparently_overloadable on the middle decl)

[rsmith]

I'd like to suggest an alternative design: don't add a new attribute., and instead change the semantics of __attribute__((overloadable)) to permit at most one non-overloadable function in an overload set. That one function would implicitly get the transparently_overloadable semantics: it can be overloaded by overloadable functions and doesn't get a mangled name. This is basically exactly how extern "C" and extern "C++" functions overload in C++: you can have up to one extern "C" function plus as many (overloadable) C++ functions as you like.

Does that seem reasonable? Or is there some reason we need or want an explicit attribute for this case?

[george.burgess.iv]

I'd be happy with that approach. Do you like it, Aaron?

FWIW, I did a bit of archaeology, and it looks like the commit that added the requirement that all overloads must have overloadable (r64414) did so to keep users from "trying to be too sneaky for their own good." The issue it was trying to solve was

double sin(double) __attribute__((overloadable));
#include <math.h>

// calls to `sin` are now mangled, which probably resulted in fun linker errors.

If we go back to no longer requiring some spelling of overloadable on all (re)decls of this special non-mangled function, the above problem shouldn't reappear.

[aaron.ballman]

In D32332#751785, @george.burgess.iv wrote:

I'd be happy with that approach. Do you like it, Aaron?

I think that makes a lot of sense.

FWIW, I did a bit of archaeology, and it looks like the commit that added the requirement that all overloads must have overloadable (r64414) did so to keep users from "trying to be too sneaky for their own good." The issue it was trying to solve was

double sin(double) __attribute__((overloadable));
#include <math.h>

// calls to `sin` are now mangled, which probably resulted in fun linker errors.

If we go back to no longer requiring some spelling of overloadable on all (re)decls of this special non-mangled function, the above problem shouldn't reappear.

[george.burgess.iv]

Remove the transparent_overloadable attribute entirely.

This approach presents one problem that I didn't see until I implemented it: I'd like to have something to detect that this feature exists. The quick fix seems to be "readd transparently_overloadable, make it equivalent to not having the attribute at all, and be happy," but that feels really icky (as does adding a special __has_enhanced_overloadable or whatever macro just for this).

Do we have a standard way of saying "does clang support an enhanced version of attribute X"? If not, I'm happy to put together a patch so people can query for that in a somewhat uniform way. This would let users write something like __has_attribute_enhancement(overloadable, unmarked_overloads), and would be more broadly useful if we decide to ever add features to another attribute in the future (diagnose_if comes to mind if I can ever find time to get back to it...).

Apologies for the lag; life is busy. :)

[george.burgess.iv]

Shortly after I pressed submit, I realized that this patch allows the following code if you tweak an assert to check for overloadable on the most recent redecl of a function:

void foo(int);
void foo(int) __attribute__((overloadable));
void foo(float);
void foo(float) __attribute__((overloadable));
void foo(double);
void foo(double) __attribute__((overloadable));

...Which is bad. I'll fix that soon. :)

[george.burgess.iv]

Fix the aforementioned issue; PTAL.

Note that this fix is slightly backwards incompatible. It disallows code like:

void foo(int);
void foo(int) __attribute__((overloadable)); // first decl lacked overloadable, so this one must lack it, as well.

I chose to do it this way because r64414 wanted to make us reject code like:

double sin(double) __attribute__((overloadable));
#include <math.h> // error: sin redecl without overloadable

...but it was trivial to get around that by moving the overloadable sin decl below the include. So, I feel like us accepting the first code snippet is a small bug.

I tested this change on ChromeOS, which uses overloadable in some parts of its C standard library. This backwards incompatibility caused no build breakages on that platform.

If we'd rather stay as backwards-compatible as possible, I have no issues with tweaking this to allow the first example.

[george.burgess.iv]

Ping :)

https://reviews.llvm.org/D33904 is what I imagine would be a good way to detect this change.

[george.burgess.iv]

Swap to using __has_feature(overloadable_unmarked) for detection, as recommended by Hal in https://reviews.llvm.org/D33904

[aaron.ballman]

This is generally looking good to me, with a few small nits. @rsmith, do you have thought on this?

lib/Sema/SemaDecl.cpp

2875–2876 ↗

(On Diff #102207)

Seems to be a formatting-only change that's unrelated to the patch; can be reverted.

9225 ↗

(On Diff #102207)

Why add Sema:: to these?

9383 ↗

(On Diff #102207)

Why does this need a capture?

[george.burgess.iv]

Addressed all feedback

[rsmith]

include/clang/Basic/AttrDocs.td

608–611 ↗

(On Diff #102850)

Can you update this to point out the exception below?

include/clang/Basic/DiagnosticSemaKinds.td

3294–3295 ↗

(On Diff #102850)

I think calling the functions 'overloadable' in this case is confusing.

lib/Lex/PPMacroExpansion.cpp

1136 ↗

(On Diff #102850)

Should this be __has_extension rather than __has_feature, since it's not a standard feature? (Per http://clang.llvm.org/docs/LanguageExtensions.html#has-feature-and-has-extension __has_feature is only for detecting standard features, despite us using it for other things.)

lib/Sema/SemaDecl.cpp

9249–9252 ↗

(On Diff #102850)

checkForConflictWithnonVisibleExternC should probably be responsible for finding the most recent declaration of the entity that is actually visible (and not, say, a block-scope declaration in some unrelated function, or a declaration in an unimported module).

9375–9381 ↗

(On Diff #102850)

Seems worth factoring out this "contains any overloadable functions" check, since it's quite a few lines and you're doing it in two different places.

test/Sema/overloadable.c

112 ↗

(On Diff #102850)

It'd be nice to check in these changes from @NNN -> @-M separately.

[george.burgess.iv]

Addressed all feedback

[george.burgess.iv]

Thank you both for the comments!

include/clang/Basic/DiagnosticSemaKinds.td

3294–3295 ↗

(On Diff #102850)

Yeah, I couldn't think of a good name, either. Is this better?

lib/Lex/PPMacroExpansion.cpp

1136 ↗

(On Diff #102850)

Ah, I missed the "For backward compatibility" bit. Good call

lib/Sema/SemaDecl.cpp

9225 ↗

(On Diff #102207)

Artifact of me moving code around and then putting it back. :)

9249–9252 ↗

(On Diff #102850)

Isn't part of the goal of checkForConflictWithNonVisibleExternC to report things that (potentially) aren't visible? If so, I don't see how it can also be its responsibility to always report things that are visible, unless it's a best-effort sort of thing.

In any case, this bit of code boils down to me trying to slip a bugfix in here. Since I think fixing this properly will just pollute this diff even more, I'll back it out and try again in a separate patch. :)

9375–9381 ↗

(On Diff #102850)

With the removal of one unnecessary change (see a later response), this is no longer repeated. Happy to still pull it out into a function if you think that helps readability.

test/Sema/overloadable.c

112 ↗

(On Diff #102850)

r305947.

[rsmith]

LGTM, thanks!

[george.burgess.iv]

Woohoo! Thanks again to both of you.

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

 
 * If no viable candidates are otherwise available, we allow a conversion from a
   pointer of type ``T*`` to a pointer of type ``U*``, where ``T`` and ``U`` are
   incompatible. This conversion is ranked below all other types of conversions.
   Please note: ``U`` lacking qualifiers that are present on ``T`` is sufficient
   for ``T`` and ``U`` to be incompatible.
 
 The declaration of ``overloadable`` functions is restricted to function
-declarations and definitions.  Most importantly, if any function with a given
-name is given the ``overloadable`` attribute, then all function declarations
-and definitions with that name (and in that scope) must have the
-``overloadable`` attribute.  This rule even applies to redeclarations of
-functions whose original declaration had the ``overloadable`` attribute, e.g.,
+declarations and definitions.  If a function is marked with the ``overloadable``
+attribute, then all declarations and definitions of functions with that name,
+except for at most one (see the note below about unmarked overloads), must have
+the ``overloadable`` attribute.  In addition, redeclarations of a function with
+the ``overloadable`` attribute must have the ``overloadable`` attribute, and
+redeclarations of a function without the ``overloadable`` attribute must *not*
+have the ``overloadable`` attribute. e.g.,
 
 .. code-block:: c
 
   int f(int) __attribute__((overloadable));
   float f(float); // error: declaration of "f" must have the "overloadable" attribute
+  int f(int); // error: redeclaration of "f" must have the "overloadable" attribute
 
   int g(int) __attribute__((overloadable));
   int g(int) { } // error: redeclaration of "g" must also have the "overloadable" attribute
 
+  int h(int);
+  int h(int) __attribute__((overloadable)); // error: declaration of "h" must not
+                                            // have the "overloadable" attribute
+
 Functions marked ``overloadable`` must have prototypes.  Therefore, the
 following code is ill-formed:
 
 .. code-block:: c
 
   int h() __attribute__((overloadable)); // error: h does not have a prototype
 
 However, ``overloadable`` functions are allowed to use a ellipsis even if there
* Future versions of Clang may change the name mangling of functions overloaded
   functions.
 
 * The ``overloadable`` attribute has almost no meaning when used in C++,
   because names will already be mangled and functions are already overloadable.
   However, when an ``overloadable`` function occurs within an ``extern "C"``
   linkage specification, it's name *will* be mangled in the same way as it
   would in C.
 
-Query for this feature with ``__has_extension(attribute_overloadable)``.
+For the purpose of backwards compatibility, at most one function with the same
+name as other ``overloadable`` functions may omit the ``overloadable``
+attribute. In this case, the function without the ``overloadable`` attribute
+will not have its name mangled.
+
+For example:
+
+.. code-block:: c
+
+  // Notes with mangled names assume Itanium mangling.
+  int f(int);
+  int f(double) __attribute__((overloadable));
+  void foo() {
+    f(5); // Emits a call to f (not _Z1fi, as it would with an overload that
+          // was marked with overloadable).
+    f(1.0); // Emits a call to _Z1fd.
+  }
+
+Support for unmarked overloads is not present in some versions of clang. You may
+query for it using ``__has_extension(overloadable_unmarked)``.
+
+Query for this attribute with ``__has_attribute(overloadable)``.
   }];
 }
 
 def ObjCMethodFamilyDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 Many methods in Objective-C have conventional meanings determined by their
 selectors. It is sometimes useful to be able to mark a method as having a

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

def err_iboutletcollection_builtintype : Error<
   "type argument of iboutletcollection attribute cannot be a builtin type">;
 def warn_iboutlet_object_type : Warning<
   "%select{instance variable|property}2 with %0 attribute must "
   "be an object type (invalid %1)">, InGroup<ObjCInvalidIBOutletProperty>;
 def warn_iboutletcollection_property_assign : Warning<
   "IBOutletCollection properties should be copy/strong and not assign">,
   InGroup<ObjCInvalidIBOutletProperty>;
   
-def err_attribute_overloadable_missing : Error<
-  "%select{overloaded function|redeclaration of}0 %1 must have the "
-  "'overloadable' attribute">;
+def err_attribute_overloadable_mismatch : Error<
+  "redeclaration of %0 must %select{not |}1have the 'overloadable' attribute">;
 def note_attribute_overloadable_prev_overload : Note<
-  "previous overload of function is here">;
+  "previous %select{unmarked |}0overload of function is here">;
 def err_attribute_overloadable_no_prototype : Error<
   "'overloadable' function %0 must have a prototype">;
+def err_attribute_overloadable_multiple_unmarked_overloads : Error<
+  "at most one overload for a given name may lack the 'overloadable' "
+  "attribute">;
 def warn_ns_attribute_wrong_return_type : Warning<
   "%0 attribute only applies to %select{functions|methods|properties}1 that "
   "return %select{an Objective-C object|a pointer|a non-retainable pointer}2">,
   InGroup<IgnoredAttributes>;
 def err_ns_attribute_wrong_parameter_type : Error<
   "%0 attribute only applies to "
   "%select{Objective-C object|pointer|pointer-to-CF-pointer}1 parameters">;
 def warn_ns_attribute_wrong_parameter_type : Warning<

cfe/trunk/lib/Lex/PPMacroExpansion.cpp

return llvm::StringSwitch<bool>(Extension)
            .Case("cxx_range_for", LangOpts.CPlusPlus)
            .Case("cxx_reference_qualified_functions", LangOpts.CPlusPlus)
            .Case("cxx_rvalue_references", LangOpts.CPlusPlus)
            .Case("cxx_variadic_templates", LangOpts.CPlusPlus)
            // C++14 features supported by other languages as extensions.
            .Case("cxx_binary_literals", true)
            .Case("cxx_init_captures", LangOpts.CPlusPlus11)
            .Case("cxx_variable_templates", LangOpts.CPlusPlus)
+           // Miscellaneous language extensions
+           .Case("overloadable_unmarked", true)
            .Default(false);
 }
 
 /// EvaluateHasIncludeCommon - Process a '__has_include("path")'
 /// or '__has_include_next("path")' expression.
 /// Returns true if successful.
 static bool EvaluateHasIncludeCommon(Token &Tok,
                                      IdentifierInfo *II, Preprocessor &PP,

cfe/trunk/lib/Sema/SemaDecl.cpp

 ///
 /// This routine determines whether overloading is possible, not
 /// whether some new function is actually an overload. It will return
 /// true in C++ (where we can always provide overloads) or, as an
 /// extension, in C when the previous function is already an
 /// overloaded function declaration or has the "overloadable"
 /// attribute.
 static bool AllowOverloadingOfFunction(LookupResult &Previous,
-                                       ASTContext &Context) {
+                                       ASTContext &Context,
+                                       const FunctionDecl *New) {
   if (Context.getLangOpts().CPlusPlus)
     return true;
 
   if (Previous.getResultKind() == LookupResult::FoundOverloaded)
     return true;
 
-  return (Previous.getResultKind() == LookupResult::Found
-          && Previous.getFoundDecl()->hasAttr<OverloadableAttr>());
+  return Previous.getResultKind() == LookupResult::Found &&
+         (Previous.getFoundDecl()->hasAttr<OverloadableAttr>() ||
+          New->hasAttr<OverloadableAttr>());
 }
 
 /// Add this decl to the scope shadowed decl chains.
 void Sema::PushOnScopeChains(NamedDecl *D, Scope *S, bool AddToContext) {
   // Move up the scope chain until we find the nearest enclosing
   // non-transparent context. The declaration will be introduced into this
   // scope.
   while (S->getEntity() && S->getEntity()->isTransparentContext())
bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *&OldD,
   if (New->hasAttr<InternalLinkageAttr>() &&
       !Old->hasAttr<InternalLinkageAttr>()) {
     Diag(New->getLocation(), diag::err_internal_linkage_redeclaration)
         << New->getDeclName();
     notePreviousDefinition(Old, New->getLocation());
     New->dropAttr<InternalLinkageAttr>();
   }
 
+  if (!getLangOpts().CPlusPlus) {
+    bool OldOvl = Old->hasAttr<OverloadableAttr>();
+    if (OldOvl != New->hasAttr<OverloadableAttr>() && !Old->isImplicit()) {
+      Diag(New->getLocation(), diag::err_attribute_overloadable_mismatch)
+        << New << OldOvl;
+
+      // Try our best to find a decl that actually has the overloadable
+      // attribute for the note. In most cases (e.g. programs with only one
+      // broken declaration/definition), this won't matter.
+      //
+      // FIXME: We could do this if we juggled some extra state in
+      // OverloadableAttr, rather than just removing it.
+      const Decl *DiagOld = Old;
+      if (OldOvl) {
+        auto OldIter = llvm::find_if(Old->redecls(), [](const Decl *D) {
+          const auto *A = D->getAttr<OverloadableAttr>();
+          return A && !A->isImplicit();
+        });
+        // If we've implicitly added *all* of the overloadable attrs to this
+        // chain, emitting a "previous redecl" note is pointless.
+        DiagOld = OldIter == Old->redecls_end() ? nullptr : *OldIter;
+      }
+
+      if (DiagOld)
+        Diag(DiagOld->getLocation(),
+             diag::note_attribute_overloadable_prev_overload)
+          << OldOvl;
+
+      if (OldOvl)
+        New->addAttr(OverloadableAttr::CreateImplicit(Context));
+      else
+        New->dropAttr<OverloadableAttr>();
+    }
+  }
+
   // If a function is first declared with a calling convention, but is later
   // declared or defined without one, all following decls assume the calling
   // convention of the first.
   //
   // It's OK if a function is first declared without a calling convention,
   // but is later declared or defined with the default calling convention.
   //
   // To test if either decl has an explicit calling convention, we look for
bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD,
   // Determine whether the type of this function should be merged with
   // a previous visible declaration. This never happens for functions in C++,
   // and always happens in C if the previous declaration was visible.
   bool MergeTypeWithPrevious = !getLangOpts().CPlusPlus &&
                                !Previous.isShadowed();
 
   bool Redeclaration = false;
   NamedDecl *OldDecl = nullptr;
+  bool MayNeedOverloadableChecks = false;
 
   // Merge or overload the declaration with an existing declaration of
   // the same name, if appropriate.
   if (!Previous.empty()) {
     // Determine whether NewFD is an overload of PrevDecl or
     // a declaration that requires merging. If it's an overload,
     // there's no more work to do here; we'll just add the new
     // function to the scope.
-    if (!AllowOverloadingOfFunction(Previous, Context)) {
+    if (!AllowOverloadingOfFunction(Previous, Context, NewFD)) {
       NamedDecl *Candidate = Previous.getRepresentativeDecl();
       if (shouldLinkPossiblyHiddenDecl(Candidate, NewFD)) {
         Redeclaration = true;
         OldDecl = Candidate;
       }
     } else {
+      MayNeedOverloadableChecks = true;
       switch (CheckOverload(S, NewFD, Previous, OldDecl,
                             /*NewIsUsingDecl*/ false)) {
       case Ovl_Match:
         Redeclaration = true;
         break;
 
       case Ovl_NonFunction:
         Redeclaration = true;
         break;
 
       case Ovl_Overload:
         Redeclaration = false;
         break;
       }
-
-      if (!getLangOpts().CPlusPlus && !NewFD->hasAttr<OverloadableAttr>()) {
-        // If a function name is overloadable in C, then every function
-        // with that name must be marked "overloadable".
-        Diag(NewFD->getLocation(), diag::err_attribute_overloadable_missing)
-          << Redeclaration << NewFD;
-        NamedDecl *OverloadedDecl =
-            Redeclaration ? OldDecl : Previous.getRepresentativeDecl();
-        Diag(OverloadedDecl->getLocation(),
-             diag::note_attribute_overloadable_prev_overload);
-        NewFD->addAttr(OverloadableAttr::CreateImplicit(Context));
-      }
     }
   }
 
   // Check for a previous extern "C" declaration with this name.
   if (!Redeclaration &&
       checkForConflictWithNonVisibleExternC(*this, NewFD, Previous)) {
     if (!Previous.empty()) {
       // This is an extern "C" declaration with the same name as a previous
       // declaration, and thus redeclares that entity...
       Redeclaration = true;
       OldDecl = Previous.getFoundDecl();
       MergeTypeWithPrevious = false;
 
       // ... except in the presence of __attribute__((overloadable)).
-      if (OldDecl->hasAttr<OverloadableAttr>()) {
-        if (!getLangOpts().CPlusPlus && !NewFD->hasAttr<OverloadableAttr>()) {
-          Diag(NewFD->getLocation(), diag::err_attribute_overloadable_missing)
-            << Redeclaration << NewFD;
-          Diag(Previous.getFoundDecl()->getLocation(),
-               diag::note_attribute_overloadable_prev_overload);
-          NewFD->addAttr(OverloadableAttr::CreateImplicit(Context));
-        }
+      if (OldDecl->hasAttr<OverloadableAttr>() ||
+          NewFD->hasAttr<OverloadableAttr>()) {
         if (IsOverload(NewFD, cast<FunctionDecl>(OldDecl), false)) {
+          MayNeedOverloadableChecks = true;
           Redeclaration = false;
           OldDecl = nullptr;
         }
       }
     }
   }
 
   // C++11 [dcl.constexpr]p8:
if (Redeclaration) {
     } else {
       if (shouldLinkDependentDeclWithPrevious(NewFD, OldDecl)) {
         // This needs to happen first so that 'inline' propagates.
         NewFD->setPreviousDeclaration(cast<FunctionDecl>(OldDecl));
         if (isa<CXXMethodDecl>(NewFD))
           NewFD->setAccess(OldDecl->getAccess());
       }
     }
+  } else if (!getLangOpts().CPlusPlus && MayNeedOverloadableChecks &&
+             !NewFD->getAttr<OverloadableAttr>()) {
+    assert((Previous.empty() ||
+            llvm::any_of(Previous,
+                         [](const NamedDecl *ND) {
+                           return ND->hasAttr<OverloadableAttr>();
+                         })) &&
+           "Non-redecls shouldn't happen without overloadable present");
+
+    auto OtherUnmarkedIter = llvm::find_if(Previous, [](const NamedDecl *ND) {
+      const auto *FD = dyn_cast<FunctionDecl>(ND);
+      return FD && !FD->hasAttr<OverloadableAttr>();
+    });
+
+    if (OtherUnmarkedIter != Previous.end()) {
+      Diag(NewFD->getLocation(),
+           diag::err_attribute_overloadable_multiple_unmarked_overloads);
+      Diag((*OtherUnmarkedIter)->getLocation(),
+           diag::note_attribute_overloadable_prev_overload)
+          << false;
+
+      NewFD->addAttr(OverloadableAttr::CreateImplicit(Context));
+    }
   }
 
   // Semantic checking for this function declaration (in isolation).
 
   if (getLangOpts().CPlusPlus) {
     // C++-specific checks.
     if (CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(NewFD)) {
       CheckConstructor(Constructor);

cfe/trunk/test/CodeGen/mangle-ms.c

 // RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-pc-win32 | FileCheck %s
 
 // CHECK: define void @"\01?f@@$$J0YAXP6AX@Z@Z"
 __attribute__((overloadable)) void f(void (*x)()) {}
+
+// CHECK: define void @f
+void f(void (*x)(int)) {}
+
+// CHECK: define void @g
+void g(void (*x)(int)) {}
+
+// CHECK: define void @"\01?g@@$$J0YAXP6AX@Z@Z"
+__attribute__((overloadable)) void g(void (*x)()) {}

cfe/trunk/test/CodeGen/mangle.c

 // RUN: %clang_cc1 -triple i386-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
 
 // CHECK: @foo
 
 // Make sure we mangle overloadable, even in C system headers.
 # 1 "somesystemheader.h" 1 3 4
 // CHECK: @_Z2f0i
 void __attribute__((__overloadable__)) f0(int a) {}
 // CHECK: @_Z2f0l
 void __attribute__((__overloadable__)) f0(long b) {}
 
+// Unless it's unmarked.
+// CHECK: @f0
+void f0(float b) {}
+
 // CHECK: @bar
 
 // These should get merged.
 void foo() __asm__("bar");
 void foo2() __asm__("bar");
 
 int nux __asm__("foo");
 extern float nux2 __asm__("foo");

cfe/trunk/test/CodeGenCXX/mangle-ms.cpp

 void fn_tmpl() {}
 
 template void fn_tmpl<extern_c_func>();
 // CHECK-DAG: @"\01??$fn_tmpl@$1?extern_c_func@@YAXXZ@@YAXXZ"
 
 extern "C" void __attribute__((overloadable)) overloaded_fn() {}
 // CHECK-DAG: @"\01?overloaded_fn@@$$J0YAXXZ"
 
+extern "C" void overloaded_fn2() {}
+// CHECK-DAG: @overloaded_fn2
+//
+extern "C" void __attribute__((overloadable)) overloaded_fn3();
+extern "C" void overloaded_fn3() {}
+// CHECK-DAG: @overloaded_fn3
+
 namespace UnnamedType {
 struct S {
   typedef struct {} *T1[1];
   typedef struct {} T2;
   typedef struct {} *T3, T4;
   using T5 = struct {};
   using T6 = struct {} *;
 };

cfe/trunk/test/PCH/attrs.c

 // Test this without pch.
 // RUN: %clang_cc1 -include %s -fsyntax-only -verify %s
 
 // Test with pch.
 // RUN: %clang_cc1 -emit-pch -o %t %s
 // RUN: %clang_cc1 -include-pch %t -fsyntax-only -verify %s 
 
 #ifndef HEADER
 #define HEADER
 
 int f(int) __attribute__((visibility("default"), overloadable));
 int g(int) __attribute__((abi_tag("foo", "bar", "baz"), no_sanitize("address", "memory")));
 
 #else
 
+float f(float);
 double f(double); // expected-error{{overloadable}}
-                  // expected-note@11{{previous overload}}
+                  // expected-note@-2{{previous unmarked overload}}
 void h() { g(0); }
 
 #endif

cfe/trunk/test/Sema/overloadable.c

 // 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) __attribute__((overloadable)); // expected-note{{previous overload of function is here}}
+float *f(float);
 int *f(int); // expected-error{{redeclaration of 'f' must have the 'overloadable' attribute}} \
              // expected-note{{previous declaration is here}}
 double *f(double) __attribute__((overloadable)); // okay, new
 
+// Ensure we don't complain about overloadable on implicitly declared functions.
+int isdigit(int) __attribute__((overloadable));
+
 void test_f(int iv, float fv, double dv) {
   int *ip = f(iv);
   float *fp = f(fv);
   double *dp = f(dv);
 }
 
 int *accept_funcptr(int (*)()) __attribute__((overloadable)); //         \
   // expected-note{{candidate function}}

 typedef int f1_type();
 f1_type __attribute__((overloadable)) f1; // expected-error{{'overloadable' function 'f1' must have a prototype}}
 
 void test() {
   f0();
   f1();
 }
 
-void before_local_1(int) __attribute__((overloadable)); // expected-note {{here}}
+void before_local_1(int) __attribute__((overloadable));
 void before_local_2(int); // expected-note {{here}}
 void before_local_3(int) __attribute__((overloadable));
 void local() {
-  void before_local_1(char); // expected-error {{must have the 'overloadable' attribute}}
-  void before_local_2(char) __attribute__((overloadable)); // expected-error {{conflicting types}}
+  void before_local_1(char);
+  void before_local_2(char); // expected-error {{conflicting types}}
   void before_local_3(char) __attribute__((overloadable));
-  void after_local_1(char); // expected-note {{here}}
-  void after_local_2(char) __attribute__((overloadable)); // expected-note {{here}}
+  void after_local_1(char);
+  void after_local_2(char) __attribute__((overloadable));
   void after_local_3(char) __attribute__((overloadable));
 }
-void after_local_1(int) __attribute__((overloadable)); // expected-error {{conflicting types}}
-void after_local_2(int); // expected-error {{must have the 'overloadable' attribute}}
+void after_local_1(int) __attribute__((overloadable));
+void after_local_2(int);
 void after_local_3(int) __attribute__((overloadable));
 
 // Make sure we allow C-specific conversions in C.
 void conversions() {
   void foo(char *c) __attribute__((overloadable));
   void foo(char *c) __attribute__((overloadable, enable_if(c, "nope.jpg")));
 
   void *ptr;
void dropping_qualifiers_is_incompatible() {
 
   void foo(char *c) __attribute__((overloadable));
   void foo(const volatile unsigned char *c) __attribute__((overloadable));
 
   foo(ccharbuf); // expected-error{{call to 'foo' is ambiguous}} expected-note@-3{{candidate function}} expected-note@-2{{candidate function}}
   foo(vcharbuf); // expected-error{{call to 'foo' is ambiguous}} expected-note@-4{{candidate function}} expected-note@-3{{candidate function}}
 }
 
+void overloadable_with_global() {
+  void wg_foo(void) __attribute__((overloadable)); // expected-note{{previous}}
+  void wg_foo(int) __attribute__((overloadable));
+}
+
+int wg_foo; // expected-error{{redefinition of 'wg_foo' as different kind of symbol}}
+
+#if !__has_extension(overloadable_unmarked)
+#error "We should have unmarked overload support"
+#endif
+
+void to_foo0(int);
+void to_foo0(double) __attribute__((overloadable)); // expected-note{{previous overload}}
+void to_foo0(int);
+void to_foo0(double); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo0(int);
+
+void to_foo1(int) __attribute__((overloadable)); // expected-note 2{{previous overload}}
+void to_foo1(double);
+void to_foo1(int); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo1(double);
+void to_foo1(int); // expected-error{{must have the 'overloadable' attribute}}
+
+void to_foo2(int); // expected-note{{previous unmarked overload}}
+void to_foo2(double) __attribute__((overloadable)); // expected-note 2{{previous overload}}
+void to_foo2(int) __attribute__((overloadable)); // expected-error {{must not have the 'overloadable' attribute}}
+void to_foo2(double); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo2(int);
+void to_foo2(double); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo2(int);
+
+void to_foo3(int);
+void to_foo3(double) __attribute__((overloadable)); // expected-note{{previous overload}}
+void to_foo3(int);
+void to_foo3(double); // expected-error{{must have the 'overloadable' attribute}}
+
+void to_foo4(int) __attribute__((overloadable)); // expected-note{{previous overload}}
+void to_foo4(int); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo4(double) __attribute__((overloadable));
+
+void to_foo5(int);
+void to_foo5(int); // expected-note 3{{previous unmarked overload}}
+void to_foo5(float) __attribute__((overloadable));
+void to_foo5(double); // expected-error{{at most one overload for a given name may lack the 'overloadable' attribute}}
+void to_foo5(float) __attribute__((overloadable));
+void to_foo5(short); // expected-error{{at most one overload for a given name may lack the 'overloadable' attribute}}
+void to_foo5(long); // expected-error{{at most one overload for a given name may lack the 'overloadable' attribute}}
+void to_foo5(double) __attribute__((overloadable));
+
+void to_foo6(int) __attribute__((enable_if(1, ""), overloadable)); // expected-note{{previous overload}}
+void to_foo6(int) __attribute__((enable_if(1, ""))); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo6(int) __attribute__((enable_if(1, ""), overloadable));
+
+void to_foo7(int) __attribute__((enable_if(1, ""))); // expected-note{{previous unmarked overload}}
+void to_foo7(int) __attribute__((enable_if(1, ""), overloadable)); // expected-error{{must not have the 'overloadable' attribute}}
+void to_foo7(int) __attribute__((enable_if(1, "")));
+
+void to_foo8(char *__attribute__((pass_object_size(0))))
+  __attribute__((enable_if(1, "")));
+void to_foo8(char *__attribute__((pass_object_size(0))))
+  __attribute__((overloadable));
+
+void to_foo9(int); // expected-note{{previous unmarked overload}}
+// FIXME: It would be nice if we did better with the "previous unmarked
+// overload" diag.
+void to_foo9(int) __attribute__((overloadable)); // expected-error{{must not have the 'overloadable' attribute}} expected-note{{previous declaration}} expected-note{{previous unmarked overload}}
+void to_foo9(float); // expected-error{{conflicting types for 'to_foo9'}}
+void to_foo9(float) __attribute__((overloadable));
+void to_foo9(double); // expected-error{{at most one overload for a given name may lack the 'overloadable' attribute}}
+void to_foo9(double) __attribute__((overloadable));
+
+void to_foo10(int) __attribute__((overloadable));
+void to_foo10(double); // expected-note{{previous unmarked overload}}
+// no "note: previous redecl" if no previous decl has `overloadable`
+// spelled out
+void to_foo10(float); // expected-error{{at most one overload for a given name may lack the 'overloadable' attribute}}
+void to_foo10(float); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo10(float); // expected-error{{must have the 'overloadable' attribute}}
+
 // Bug: we used to treat `__typeof__(foo)` as though it was `__typeof__(&foo)`
 // if `foo` was overloaded with only one function that could have its address
 // taken.
 void typeof_function_is_not_a_pointer() {
   void not_a_pointer(void *) __attribute__((overloadable));
   void not_a_pointer(char *__attribute__((pass_object_size(1))))
     __attribute__((overloadable));
 
   __typeof__(not_a_pointer) *fn;
 
   void take_fn(void (*)(void *));
   // if take_fn is passed a void (**)(void *), we'll get a warning.
   take_fn(fn);
 }