Page MenuHomePhabricator

Add support for transparent overloadable functions in clang
ClosedPublic

Authored by george.burgess.iv on Apr 20 2017, 6:56 PM.

Details

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. :)

Diff Detail

Event Timeline

aaron.ballman edited edge metadata.

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
3293

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

test/Sema/overloadable.c
189

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.

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.

  1. 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
3293

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

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.

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

  1. 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.

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.

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

aaron.ballman added inline comments.May 10 2017, 12:01 PM
lib/AST/ItaniumMangle.cpp
583 ↗(On Diff #97698)

const auto *

lib/AST/MicrosoftMangle.cpp
372 ↗(On Diff #97698)

const auto *

lib/Sema/SemaDecl.cpp
2875

const auto *

2944

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

9374

const auto *

9377–9378

const auto * in both places.

test/Sema/overloadable.c
189

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 marked 7 inline comments as done.
  • Addressed all feedback
  • Now we emit a warning when transparently_overloadable "overrides" overloadable, rather than an error
lib/Sema/SemaDecl.cpp
2944

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

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 edited edge metadata.May 10 2017, 5:49 PM

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?

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.

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.

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. :)

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. :)

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.

Ping :)

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

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

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

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

9225

Why add Sema:: to these?

9383

Why does this need a capture?

george.burgess.iv marked 3 inline comments as done.

Addressed all feedback

rsmith added inline comments.Jun 20 2017, 5:37 PM
include/clang/Basic/AttrDocs.td
608–611

Can you update this to point out the exception below?

include/clang/Basic/DiagnosticSemaKinds.td
3294–3295

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

lib/Lex/PPMacroExpansion.cpp
1136

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

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

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–113

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

george.burgess.iv marked 6 inline comments as done.

Addressed all feedback

Thank you both for the comments!

include/clang/Basic/DiagnosticSemaKinds.td
3294–3295

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

lib/Lex/PPMacroExpansion.cpp
1136

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

lib/Sema/SemaDecl.cpp
9225

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

9249–9252

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

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–113

r305947.

rsmith accepted this revision.Jun 27 2017, 1:54 PM

LGTM, thanks!

This revision is now accepted and ready to land.Jun 27 2017, 1:54 PM

Woohoo! Thanks again to both of you.

This revision was automatically updated to reflect the committed changes.