[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
+  }];
----------------
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.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4130
@@ -4129,3 +4129,3 @@
 def err_alias_is_definition :
-  Error<"definition %0 cannot also be an alias">;
+  Error<"definition %0 cannot also be an %select{alias|ifunc}1">;
 def err_definition_of_implicitly_declared_member : Error<
----------------
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.

================
Comment at: lib/AST/Decl.cpp:1943
@@ -1942,3 +1942,3 @@
 
-  if (hasAttr<AliasAttr>())
+  if (hasAttr<AliasAttr>() || hasAttr<IFuncAttr>())
     return Definition;
----------------
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().

================
Comment at: lib/CodeGen/CodeGenModule.cpp:299
@@ -292,3 +298,3 @@
       Error = true;
-      Diags.Report(AA->getLocation(), diag::err_cyclic_alias);
+      Diags.Report(Location, diag::err_cyclic_alias);
     } else if (GV->isDeclaration()) {
----------------
Please adjust this diagnostic to say something correct for ifuncs.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:302
@@ -295,3 +301,3 @@
       Error = true;
-      Diags.Report(AA->getLocation(), diag::err_alias_to_undefined);
+      Diags.Report(Location, diag::err_alias_to_undefined);
     }
----------------
Please adjust this diagnostic to say something correct for ifuncs.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:316
@@ -309,3 +315,3 @@
         Diags.Report(SA->getLocation(), diag::warn_alias_with_section)
             << AliasSection;
     }
----------------
Please adjust this diagnostic to say something correct for ifuncs.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:327
@@ -321,2 +326,3 @@
+        Diags.Report(Location, diag::warn_alias_to_weak_alias)
             << GV->getName() << GA->getName();
         Aliasee = llvm::ConstantExpr::getPointerBitCastOrAddrSpaceCast(
----------------
Please adjust this diagnostic to say something correct for ifuncs.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:2686
@@ -2673,1 +2685,3 @@
+  } else
+    llvm_unreachable("Not an aliasi or ifunc?");
 
----------------
typo

================
Comment at: lib/CodeGen/CodeGenModule.cpp:2691
@@ -2678,1 +2690,3 @@
+  if (AliaseeName == MangledName) {
+    Diags.Report(Location, diag::err_cyclic_alias);
     return;
----------------
Please adjust this diagnostic to say something correct for ifuncs.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:2696
@@ -2681,3 +2695,3 @@
   // If there is a definition in the module, then it wins over the alias.
   // This is dubious, but allow it to be safe.  Just ignore the alias.
   llvm::GlobalValue *Entry = GetGlobalValue(MangledName);
----------------
Do not allow this for ifunc.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:2700
@@ -2685,3 +2699,3 @@
     return;
 
   Aliases.push_back(GD);
----------------
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.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:2722
@@ -2707,3 +2721,3 @@
     if (GA->getAliasee() == Entry) {
-      Diags.Report(AA->getLocation(), diag::err_cyclic_alias);
+      Diags.Report(Location, diag::err_cyclic_alias);
       return;
----------------
Please adjust this diagnostic to say something correct for ifuncs.

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

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

================
Comment at: lib/Sema/SemaDecl.cpp:2316
@@ -2315,3 +2315,3 @@
                             ? diag::err_alias_after_tentative
                             : diag::err_redefinition;
         S.Diag(VD->getLocation(), Diag) << VD->getDeclName();
----------------
Please adjust this diagnostic to say something correct for ifuncs.


http://reviews.llvm.org/D15524