Page MenuHomePhabricator

[GCC] Attribute ifunc support in clang
ClosedPublic

Authored by DmitryPolukhin on Dec 15 2015, 6:32 AM.

Details

Summary

This patch add support for GCC attribute((ifunc("resolver"))) for targets that use ELF as object file format. In general ifunc is a special kind of function alias with type @gnu_indirect_function. LLVM patch http://reviews.llvm.org/D15525

Diff Detail

Repository
rL LLVM

Event Timeline

DmitryPolukhin retitled this revision from to [GCC] Attribute ifunc support in clang.
DmitryPolukhin updated this object.
DmitryPolukhin added a subscriber: cfe-commits.
aaron.ballman edited edge metadata.Dec 15 2015, 9:07 AM

Attribute implementation LGTM, but I do not know the semantics of ifunc well enough to comment on whether this implementation is correct or not. From what I can tell of GCC's documentation, it looks reasonable, but it would be good for Eric or John to take a look as well.

rjmccall added inline comments.Dec 16 2015, 10:25 AM
include/clang/Basic/AttrDocs.td
1866 ↗(On Diff #42844)

This is unnecessarily technical for user-level documentation. Fortunately, you need to change it anyway because you copied that sentence directly from documentation that I believe is GPL'ed. Also, it would be better to actually document the behavior of the attribute here rather than forwarding to GCC's documentation.

I would say something like this:

``__attribute__((ifunc("resolver")))`` is used to mark that the address of a declaration should be resolved at runtime by calling a resolver function.

The symbol name of the resolver function is given in quotes.  A function with this name (after mangling) must be defined in the current translation unit; it may be ``static``.  The resolver function should take no arguments and return a function pointer of type ``void (*)(void)``.

The ``ifunc`` attribute may only be used on a function declaration.  A function declaration with an ``ifunc`` attribute is considered to be a definition of the declared entity.  The entity must not have weak linkage; for example, in C++, it cannot be applied to a declaration if a definition at that location would be considered inline.

Not all targets support this attribute.  ELF targets support this attribute when using binutils v2.20.1 or higher and glibc v2.11.1 or higher.  Non-ELF targets currently do not support this attribute.
include/clang/Basic/DiagnosticSemaKinds.td
4130 ↗(On Diff #42844)

This is a good start, but there are a lot of diagnostics involving the alias attribute that you need to adjust to say something appropriate for ifuncs.

lib/AST/Decl.cpp
1943 ↗(On Diff #42844)

Please extract this logic into a method on Decl called something like hasDefiningAttr() and use it consistently throughout this patch.

You will also want a getDefiningAttr().

lib/CodeGen/CodeGenModule.cpp
299 ↗(On Diff #42844)

Please adjust this diagnostic to say something correct for ifuncs.

302 ↗(On Diff #42844)

Please adjust this diagnostic to say something correct for ifuncs.

316 ↗(On Diff #42844)

Please adjust this diagnostic to say something correct for ifuncs.

327 ↗(On Diff #42844)

Please adjust this diagnostic to say something correct for ifuncs.

2686 ↗(On Diff #42844)

typo

2691 ↗(On Diff #42844)

Please adjust this diagnostic to say something correct for ifuncs.

2696 ↗(On Diff #42844)

Do not allow this for ifunc.

2700 ↗(On Diff #42844)

Please diagnose that the resolver function has the appropriate type here. Given the constraints, you should be able to do this by inspecting the LLVM function.

2722 ↗(On Diff #42844)

Please adjust this diagnostic to say something correct for ifuncs.

2757 ↗(On Diff #42844)

This seems like a very poor choice of representation for this in LLVM IR. I understand that there are some basic parallels between ifuncs and global aliases in terms of what they store — they're both global definitions that refer to a different symbol — but they are semantically extremely different. In particular, code that sees an llvm::GlobalAlias should not have to check !isIFunc().

2758 ↗(On Diff #42844)

Can you explain the purpose of this line?

lib/Sema/SemaDecl.cpp
2316 ↗(On Diff #42844)

Please adjust this diagnostic to say something correct for ifuncs.

John, I'm still working on new patch but meanwhile I would like to clarify few things in your comments.

include/clang/Basic/AttrDocs.td
1866 ↗(On Diff #42844)

The symbol name of the resolver function is given in quotes. A function with this name (after mangling) must be defined in the current translation unit; it may be `static. The resolver function should take no arguments and return a function pointer of type void (*)(void)

Actually GCC doesn't make much less checks on resolver function itself. For example, it could return size_t or even void. So I removed part about return value type. Same as resolver function may have arguments but nothing will be passed there and it is not checked. With arguments of resolver perhaps we should add checks as for return value I think we have to be relaxed here to support existing code that uses void* as far as I can see.

lib/CodeGen/CodeGenModule.cpp
2757 ↗(On Diff #42844)

Could you please clarify what do you mean by "check !isIFunc()"? I don't have such checks I only check for if (isIFunc()) and some extra work. I only have setIFunc(false) in c-tor just to initialize the field. ifunc is an alias with some additional properties so it could be modeled as derivative class from alias.

2758 ↗(On Diff #42844)

I need it don't allow optimization that use resolver function directly instead of alias. I could patch checks or I can make special linkage in LLVM for ifunc.

rjmccall added inline comments.Dec 17 2015, 10:21 AM
include/clang/Basic/AttrDocs.td
1866 ↗(On Diff #42844)

That generally makes sense, but let's at least require it to return a pointer.

lib/CodeGen/CodeGenModule.cpp
2758 ↗(On Diff #42844)

Okay, this ties into the previous comment. The problem I have with ifuncs just being represented as global aliases with a special flag set is that now every LLVM analysis that sees a global alias has to check the flag before it can correctly interpret it. It doesn't promote maintainable, conservatively-correct code. You're working around that by setting a particular kind of linkage, but that's just going to cause other problems.

A much better fix is to make a new kind of llvm::GlobalValue that represents a dynamically resolved global. This is a lot less work than you probably think it is — there are very few exhaustive switches over all value kinds in LLVM, and frankly most of those are places you need to be updating for ifuncs anyway. It might make sense for this to share a common base class with llvm::GlobalAlias, but it shouldn't be a *subclass* of llvm::GlobalAlias.

echristo added inline comments.Dec 17 2015, 10:36 AM
lib/CodeGen/CodeGenModule.cpp
2758 ↗(On Diff #42844)

FWIW I completely agree with this. :)

DmitryPolukhin edited edge metadata.
DmitryPolukhin marked 17 inline comments as done.

Comments resolved + this patch uses new GlobalIFunc representation in llvm.

lib/CodeGen/CodeGenModule.cpp
2700 ↗(On Diff #42844)

I did it in checkAliases because here resolver function may not be available yet.

lib/Sema/SemaDecl.cpp
2316 ↗(On Diff #42844)

It is for VarDecl only so it is not applicable to ifunc.

rjmccall edited edge metadata.Dec 29 2015, 11:41 AM

This looks great, thanks. A few minor comment tweaks and this will be ready to commit.

include/clang/AST/DeclBase.h
562 ↗(On Diff #43721)

"Return true if this declaration has an attribute which acts as a definition of the entity, such as 'alias' or 'ifunc'."

565 ↗(On Diff #43721)

"Return this declaration's defining attribute if it has one."

Also, please put the * next to the method name rather than the type.

include/clang/Basic/AttrDocs.td
1868 ↗(On Diff #43721)

Okay, this should just say "...and return a pointer", since that's all we're enforcing.

lib/Sema/SemaDecl.cpp
2316 ↗(On Diff #43721)

Oh, of course, makes sense.

aaron.ballman added inline comments.Dec 29 2015, 11:57 AM
include/clang/AST/DeclBase.h
563 ↗(On Diff #43721)

I think this function and getDefiningAttr() can be defined in the header instead of split into the source file. The implementations are short enough that inlining may be nice to allow.

565 ↗(On Diff #43721)

Also, since this is a const method, I'd prefer if it returned a const Attr * if possible.

lib/AST/DeclBase.cpp
369 ↗(On Diff #43721)

Attr* -> Attr *

lib/Sema/SemaDeclAttr.cpp
1538 ↗(On Diff #43721)

Can use cast<> instead of dyn_cast<> and remove the if.

1543 ↗(On Diff #43721)

I think that this should be codified in Attr.td as a target-specific attribute; though that may require a bit of work for the attribute emitter to handle. I don't think this needs to be done in this patch, but it should at least have a FIXME or be handled in a follow-up patch.

1549 ↗(On Diff #43721)

Can remove the else clause entirely; common attribute handling already takes care of this and by switching to use cast<> instead of dyn_cast<> you already get an assert if the type is incorrect.

rjmccall added inline comments.Dec 29 2015, 12:26 PM
include/clang/AST/DeclBase.h
563 ↗(On Diff #43721)

getDefiningAttr can't be defined in the header without including Attr.h. We could define hasDefiningAttr in terms of getDefiningAttr, but it wouldn't allow much interesting optimization.

aaron.ballman added inline comments.Dec 29 2015, 12:29 PM
include/clang/AST/DeclBase.h
563 ↗(On Diff #43721)

Good point; definitely not worth it.

DmitryPolukhin edited edge metadata.
DmitryPolukhin marked 8 inline comments as done.

Fixed comments.

aaron.ballman accepted this revision.Dec 30 2015, 6:27 AM
aaron.ballman edited edge metadata.

LGTM, thank you!

This revision is now accepted and ready to land.Dec 30 2015, 6:27 AM

Thank you for the review!

I'm waiting for llvm part of ifunc support because it has to be committed first.

DmitryPolukhin edited edge metadata.
  • rebase after committing llvm patch
echristo accepted this revision.Apr 7 2016, 1:58 PM
echristo edited edge metadata.

This all seems reasonable to me, one inline rewording comment, and check with rjmccall before committing.

Thanks!

include/clang/Basic/AttrDocs.td
2371 ↗(On Diff #52906)

Probably better to say linux fwiw and not ELF.

rjmccall added inline comments.Apr 7 2016, 3:16 PM
include/clang/Basic/AttrDocs.td
2371 ↗(On Diff #52906)

The validation code in Sema is checking for an ELF target. If the restriction is more precise than that, then we should make a TargetInfo callback. Do the BSDs and other ELF targets not use binutils/glibc?

echristo added inline comments.Apr 7 2016, 3:20 PM
include/clang/Basic/AttrDocs.td
2371 ↗(On Diff #52906)

We should make a TargetInfo callback. BSDs and other ELF targets aren't guaranteed to use binutils/glibc (some of them have even switched to llvm already) - and I don't know what the state of ifunc support on those architectures is.

probinson added inline comments.
include/clang/Basic/AttrDocs.td
2371 ↗(On Diff #52906)

Hear hear. PS4 is ELF but we don't use glibc.

joerg added a subscriber: joerg.Apr 8 2016, 6:50 AM
joerg added inline comments.
include/clang/Basic/AttrDocs.td
2066 ↗(On Diff #52906)

I'd really prefer such checks to be more strict, meaning:

  1. It can return a void * pointer, no further signature checks are done
  2. It can be a function pointer. In that case, it must match the signature of the function declaration the attribute is on.
2371 ↗(On Diff #52906)

The attribute is not Linux specific, so ELF is a reasonable first approximation. Most BSDs have some ifunc support at least. I'm not in favor of doing any checks beyond ELF -- even on Linux the availability of working ifunc support depends on other factors like whether the binary is dynamically linked.

probinson added inline comments.Apr 8 2016, 10:45 AM
include/clang/Basic/AttrDocs.td
2371 ↗(On Diff #52906)

What's the failure mode if the target doesn't actually support it?

echristo added inline comments.Apr 8 2016, 1:04 PM
include/clang/Basic/AttrDocs.td
2371 ↗(On Diff #52906)

The failure mode is an unknown relocation if your linker doesn't support it. If your linker supports it but your libc doesn't that'll be an unknown dynamic relocation at program start up.

In retrospect I think we can leave it just as an ELF check here and go. There are similar features in Mach-O that we might want this to use at some point as well so we can leave it as general as possible and rely on downstream tools to provide more errors?

I honestly don't have a strong opinion here given the current vaguely broad platform support and future possibilities.

probinson added inline comments.Apr 8 2016, 1:15 PM
include/clang/Basic/AttrDocs.td
2371 ↗(On Diff #52906)

Yeah, we can live with that.

echristo added inline comments.Apr 8 2016, 1:18 PM
include/clang/Basic/AttrDocs.td
2371 ↗(On Diff #52906)

Cool. So in summary: "Ignore this entire thread" :)

Sorry, I was out for couple days.

It seems that there are no real objections so I'm committing this patch and I'm going to keep working on this topic to support target attribute with dynamic dispatching on x86 so if you think that something is missing, please let me know. Thank you for the review!

include/clang/Basic/AttrDocs.td
2371 ↗(On Diff #52906)

I think check for ELF only is a reasonable first approximation when attribute can be used.

This revision was automatically updated to reflect the committed changes.