[PATCH] D15524: [GCC] Attribute ifunc support in clang

[John McCall via cfe-commits]

rjmccall added inline comments.

================
Comment at: include/clang/Basic/AttrDocs.td:1866
@@ +1865,3 @@
+  let Content = [{
+The attribute ``__attribute__((ifunc("resolver")))`` is used to mark a function as an indirect function using the STT_GNU_IFUNC symbol type extension to the ELF standard. For more information, see GCC ifunc attribute documentation https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
+  }];
----------------
DmitryPolukhin wrote:
> rjmccall wrote:
> > 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.
> > 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.
That generally makes sense, but let's at least require it to return a pointer.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:2758
@@ +2757,3 @@
+    GA->setIFunc(true);
+    GA->setLinkage(llvm::GlobalValue::LinkOnceAnyLinkage);
+  }
----------------
DmitryPolukhin wrote:
> rjmccall wrote:
> > Can you explain the purpose of this line?
> 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.
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.


http://reviews.llvm.org/D15524