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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to