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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin added a comment.

John, I'm still working on new patch but meanwhile I would like to clarify few things in your 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
+  }];
----------------
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.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:2757
@@ +2756,3 @@
+  if (D->hasAttr<IFuncAttr>()) {
+    GA->setIFunc(true);
+    GA->setLinkage(llvm::GlobalValue::LinkOnceAnyLinkage);
----------------
rjmccall wrote:
> 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().
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. 

================
Comment at: lib/CodeGen/CodeGenModule.cpp:2758
@@ +2757,3 @@
+    GA->setIFunc(true);
+    GA->setLinkage(llvm::GlobalValue::LinkOnceAnyLinkage);
+  }
----------------
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.


http://reviews.llvm.org/D15524