[GCC] Attribute ifunc support in llvm

[DmitryPolukhin]

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. Patch for Clang http://reviews.llvm.org/D15524

[• rafael]

include/llvm/IR/GlobalAlias.h

94

Don't repeat name in comments.

[DmitryPolukhin]

Fixed comment.

[dexonsmith]

Since this modifies the IR, please start with an RFC on llvm-dev.
(I just did a search for "ifunc"... sorry if you already posted
one and I'm just incapable of using search correctly.)

I'd be interested, in particular, to hear whether you considered
adding a separate construct rather than piggy-backing on
GlobalAlias, and if so, why you rejected that alternative. It
seems like, semantically, ifuncs are very different from aliases.
A straw-man counter-proposal might be:

• Move the relevant guts of GlobalAlias into a base class, maybe called GlobalIndirectSymbol (bike shed that).
• Add a new subclass called GlobalIFunc.
• Use ifunc *instead* of alias in textual IR (making it a separate construct, rather than an attribute).

The main benefit? Current optimizations/etc. may assume that
isa<GlobalAlias>(GV) implies that GV is a global alias. It
seems clearer to use the type hierarchy to differentiate. It'll
probably be cleaner for new code, as well.

Regardless, please take it to an llvm-dev RFC so we can get more
eyes on it. (Although given the time of year, you may not get
the usual number of responses over the next couple of weeks...)

[echristo]

FWIW both I and John had the feedback on GlobalAlias on the cfe-commits
thread. :)

[DmitryPolukhin]

Thank you for pointing out the direction. I'll start working on RFC with different approaches how ifunc could be represented in IR.

Just for the sake of completeness, there is a way to avoid IR extension - it is possible to emit global asm statement like __asm__ (".type resolver_alias_name, @gnu_indirect_function") and avoid IR extension but I didn't seriously consider this path.

[DmitryPolukhin]

Work in progress patch that shows amount of changes required to introduce GloablIFunc and put it into separate list in module.

Clang part also needs to be update but it depends on LLVM part so it makes sense to reach at least some consensus on LLVM part first.

[• rafael]

include/llvm-c/Core.h

1890 ↗

(On Diff #43599)

I am not sure about adding a C api right away. Maybe wait a bit once the patch is in?

include/llvm/IR/GlobalIndirectSymbol.h

63 ↗

(On Diff #43599)

Is this meaningful for ifunc?

IMHO it is a design mistake for GlobalAlias to take an arbitrary expression. If ifunc can take just a GlobalValue that would be nice.

include/llvm/IR/Module.h

591 ↗

(On Diff #43599)

Is there real value in these @{}?

593 ↗

(On Diff #43599)

clang-format

lib/AsmParser/LLParser.cpp

717

Any type restrictions when it is not an alias?

lib/AsmParser/LLParser.h

274 ↗

(On Diff #43599)

Start functions with lowercase.

lib/Bitcode/Writer/BitcodeWriter.cpp

782

You are missing the address space in the comment.

[DmitryPolukhin]

Still work in progress, fixed most of comments.

[DmitryPolukhin]

New patch uploaded.

include/llvm-c/Core.h

1890 ↗

(On Diff #43599)

I added it just for completeness. I can remove it before commit if needed. I'm worry that it will be easy to forget if I remove it now.

include/llvm/IR/GlobalIndirectSymbol.h

63 ↗

(On Diff #43599)

It makes sense to me.

include/llvm/IR/Module.h

591 ↗

(On Diff #43599)

I don't see them in generated doxygen so it seems useless. I put them just to follow style of surrounding code. I'll remove them if there a consensus here.

593 ↗

(On Diff #43599)

Same here, such formatting is used in the whole file very consistent so I decided to follow local style. Please let me know if you really want to reformat only new block of code differently.

lib/AsmParser/LLParser.cpp

717

At least we could check that it is a function. Checking the function return type is trickier because GCC allows any type even void same as for the parameters of the function.

[DmitryPolukhin]

Clang part was updated to use new representation so I think now patch is ready for review, please take a look.

[DmitryPolukhin]

Friendly ping, please review this patch.

Clang part of ifunc support was approved for commit but llvm part needs to be committed first.

[vsk]

Please add a short test to test/Bitcode/compatibility.ll.

[DmitryPolukhin]

• added test case in test/Bitcode/compatibility.ll
• added test test/Assembler/ifunc-use-list-order.ll
• fixed ordering problem in bitcode writer
• fix for alias on ifunc
• fixed nit in LangRef

PTAL

[DmitryPolukhin]

Friendly ping, please take a look!

Please let me know if in general things look fine but this patch needs to be splatted into smaller parts to simplify review as Duncan suggested.

[DmitryPolukhin]

Friendly ping, please take a look!

[grooverdan]

Friendly ping. As a user I'm really looking forward to this feature. Thanks DmitryPolukhin for writing it.

[asl]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1101

Why alias? Shouldn't this be llvm_unreachable?

[echristo]

A couple of inline comments for the next patch, but a quick top level comment:

Since we agree that the GlobalIndirectFunc hierarchy is the right way to do this, can you split the patch in two and add that layer of indirection first and then we can add ifunc later to make it easier to see what each side of things is doing?

Also, thank you very much for all of your work and patience here. I really appreciate it.

Thanks!

-eric

include/llvm-c/Core.h

1790 ↗

(On Diff #44637)

We typically add them on demand. I'd rather leave it off for now and we can add it later if it's necessary.

include/llvm/IR/Module.h

582 ↗

(On Diff #44637)

No, consistency is fine here, thanks.

[DmitryPolukhin]

Eric, that you for looking to the code! I'll split the patch and send first part for review ASAP.

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1101

Local linkage is possible here but I'll update text to reflect that ifunc could be here too.

[DmitryPolukhin]

Extracted GlobalIndirectSymbol creation to separate patch http://reviews.llvm.org/D18433

This patch will be rebased when D18433 is committed to continue ifunc specific review.

[DmitryPolukhin]

• rebease on top of GlobalIndirectSymbol committed to LLVM
• removed C interface for ifunc, it seems that it is not required for now

[echristo]

Highlighted a couple of things that could be pulled out into an incremental patch (and you can pull out similar Alias->IndirectSymbol changes).

Thanks!

-eric

include/llvm/CodeGen/AsmPrinter.h

550–552 ↗

(On Diff #52205)

This isn't part of the previous patch?

lib/AsmParser/LLParser.h

278–282 ↗

(On Diff #52205)

Ditto.

[DmitryPolukhin]

include/llvm/CodeGen/AsmPrinter.h

550–552 ↗

(On Diff #52205)

In previous patch I included things that have sense without ifunc and functions that won't be updated in ifunc patch. For printer and bitcode write the same functions will have to be updated twice so it won't reduce this patch so significantly. But OK, I'll extract as much as I can.

[echristo]

include/llvm/CodeGen/AsmPrinter.h

550–552 ↗

(On Diff #52205)

Awesome. Thank you very much :)

[DmitryPolukhin]

Rebased on top of D18754

Eric, thank you a lot for fast review previous parts and please take a look to the remaining changes.

[echristo]

LGTM. Since Duncan has been looking at these as well you might want to wait for an ACK from him as well.

Thanks a ton for doing all of this. :)

-eric

[dexonsmith]

I have a bunch of nitpicks inline below; once you resolve those, LGTM!

[DmitryPolukhin]

• fixed comments

include/llvm/IR/GlobalAlias.h

class GlobalAlias : public GlobalValue, public ilist_node<GlobalAlias> {
   GlobalAlias(const GlobalAlias &) = delete;
 
   void setParent(Module *parent);
 
   GlobalAlias(Type *Ty, unsigned AddressSpace, LinkageTypes Linkage,
               const Twine &Name, Constant *Aliasee, Module *Parent);
 
 public:
+  /// Returns true if given alias has ifunc type.
+  bool isIFunc() const {
+    return getGlobalValueSubClassData();
+  }
+
+  /// Set isIFunc property.
+  void setIFunc(bool IsIFunc) {
+    setGlobalValueSubClassData(IsIFunc);
+  }
+
   // allocate space for exactly one operand
   void *operator new(size_t s) {
     return User::operator new(s, 1);
   }
 
   /// If a parent module is specified, the alias is automatically inserted into
   /// the end of the specified module's alias list.
   static GlobalAlias *create(Type *Ty, unsigned AddressSpace,
public:
   ///
   void removeFromParent() override;
 
   /// eraseFromParent - This method unlinks 'this' from the containing module
   /// and deletes it.
   ///
   void eraseFromParent() override;
 
+  /// Copy all additional attributes (those not needed to create a GlobalAlias)
+  /// from the Src to this one.
+  void copyAttributesFrom(const GlobalValue *Src) override;
+
   /// These methods retrive and set alias target.
   void setAliasee(Constant *Aliasee);
   const Constant *getAliasee() const {
     return const_cast<GlobalAlias *>(this)->getAliasee();
   }
   Constant *getAliasee() {
     return getOperand(0);
   }
 

lib/AsmParser/LLLexer.cpp

#define KEYWORD(STR)                                                           \
   KEYWORD(argmemonly);
   KEYWORD(builtin);
   KEYWORD(byval);
   KEYWORD(inalloca);
   KEYWORD(cold);
   KEYWORD(convergent);
   KEYWORD(dereferenceable);
   KEYWORD(dereferenceable_or_null);
+  KEYWORD(ifunc);
   KEYWORD(inlinehint);
   KEYWORD(inreg);
   KEYWORD(jumptable);
   KEYWORD(minsize);
   KEYWORD(naked);
   KEYWORD(nest);
   KEYWORD(noalias);
   KEYWORD(nobuiltin);

lib/AsmParser/LLParser.cpp

bool LLParser::ParseAlias(const std::string &Name, LocTy NameLoc, unsigned L,
 
   if(!GlobalAlias::isValidLinkage(Linkage))
     return Error(NameLoc, "invalid linkage type for alias");
 
   if (!isValidVisibilityForLinkage(Visibility, L))
     return Error(NameLoc,
                  "symbol with local linkage must have default visibility");
 
+  bool IFunc = false;
+  if (Lex.getKind() == lltok::kw_ifunc) {
+    IFunc = true;
+    Lex.Lex();
+  }
+
   Type *Ty;
   LocTy ExplicitTypeLoc = Lex.getLoc();
   if (ParseType(Ty) ||
       ParseToken(lltok::comma, "expected comma after alias's type"))
     return true;
 
   Constant *Aliasee;
   LocTy AliaseeLoc = Lex.getLoc();
bool LLParser::ParseAlias(const std::string &Name, LocTy NameLoc, unsigned L,
   }
 
   Type *AliaseeType = Aliasee->getType();
   auto *PTy = dyn_cast<PointerType>(AliaseeType);
   if (!PTy)
     return Error(AliaseeLoc, "An alias must have pointer type");
   unsigned AddrSpace = PTy->getAddressSpace();
 
   if (Ty != PTy->getElementType())
     return Error(
         ExplicitTypeLoc,
         "explicit pointee type doesn't match operand's pointee type");
 
   GlobalValue *GVal = nullptr;
 
   // See if the alias was forward referenced, if so, prepare to replace the
   // forward reference.
bool LLParser::ParseAlias(const std::string &Name, LocTy NameLoc, unsigned L,
   // Okay, create the alias but do not insert it into the module yet.
   std::unique_ptr<GlobalAlias> GA(
       GlobalAlias::create(Ty, AddrSpace, (GlobalValue::LinkageTypes)Linkage,
                           Name, Aliasee, /*Parent*/ nullptr));
   GA->setThreadLocalMode(TLM);
   GA->setVisibility((GlobalValue::VisibilityTypes)Visibility);
   GA->setDLLStorageClass((GlobalValue::DLLStorageClassTypes)DLLStorageClass);
   GA->setUnnamedAddr(UnnamedAddr);
+  GA->setIFunc(IFunc);
 
   if (Name.empty())
     NumberedVals.push_back(GA.get());
 
   if (GVal) {
     // Verify that types agree.
     if (GVal->getType() != GA->getType())
       return Error(

lib/AsmParser/LLToken.h

enum Kind {
     kw_sanitize_address,
     kw_builtin,
     kw_byval,
     kw_inalloca,
     kw_cold,
     kw_convergent,
     kw_dereferenceable,
     kw_dereferenceable_or_null,
+    kw_ifunc,
     kw_inlinehint,
     kw_inreg,
     kw_jumptable,
     kw_minsize,
     kw_naked,
     kw_nest,
     kw_noalias,
     kw_nobuiltin,

lib/Bitcode/Reader/BitcodeReader.cpp

case bitc::MODULE_CODE_ALIAS_OLD: {
       if (OpNum != Record.size())
         NewGA->setDLLStorageClass(getDecodedDLLStorageClass(Record[OpNum++]));
       else
         upgradeDLLImportExportLinkage(NewGA, Linkage);
       if (OpNum != Record.size())
         NewGA->setThreadLocalMode(getDecodedThreadLocalMode(Record[OpNum++]));
       if (OpNum != Record.size())
         NewGA->setUnnamedAddr(Record[OpNum++]);
+      if (OpNum != Record.size())
+        NewGA->setIFunc(Record[OpNum++]);
       ValueList.push_back(NewGA);
       AliasInits.push_back(std::make_pair(NewGA, Val));
       break;
     }
     /// MODULE_CODE_PURGEVALS: [numvals]
     case bitc::MODULE_CODE_PURGEVALS:
       // Trim down the value list to the specified size.
       if (Record.size() < 1 || Record[0] > ValueList.size())

lib/Bitcode/Writer/BitcodeWriter.cpp

for (const GlobalAlias &A : M->aliases()) {
     Vals.push_back(VE.getTypeID(A.getValueType()));
     Vals.push_back(A.getType()->getAddressSpace());
     Vals.push_back(VE.getValueID(A.getAliasee()));
     Vals.push_back(getEncodedLinkage(A));
     Vals.push_back(getEncodedVisibility(A));
     Vals.push_back(getEncodedDLLStorageClass(A));
     Vals.push_back(getEncodedThreadLocalMode(A));
     Vals.push_back(A.hasUnnamedAddr());
+    Vals.push_back(A.isIFunc());
     unsigned AbbrevToUse = 0;
     Stream.EmitRecord(bitc::MODULE_CODE_ALIAS, Vals, AbbrevToUse);
     Vals.clear();
   }
 
   // Write a record indicating the number of module-level metadata IDs
   // This is needed because the ids of metadata are assigned implicitly
   // based on their ordering in the bitcode, with the function-level
   // metadata ids starting after the module-level metadata ids. For
   // function importing where we lazy load the metadata as a postpass,
   // we want to avoid parsing the module-level metadata before parsing
   // the imported functions.
   BitCodeAbbrev *Abbv = new BitCodeAbbrev();
   Abbv->Add(BitCodeAbbrevOp(bitc::MODULE_CODE_METADATA_VALUES));
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6));
   unsigned MDValsAbbrev = Stream.EmitAbbrev(Abbv);

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

bool AsmPrinter::doFinalization(Module &M) {
   // Set the MachineFunction to nullptr so that we can catch attempted
   // accesses to MF specific features at the module level and so that
   // we can conditionalize accesses based on whether or not it is nullptr.
   MF = nullptr;
 
   // Gather all GOT equivalent globals in the module. We really need two
   // passes over the globals: one to compute and another to avoid its emission
   // in EmitGlobalVariable, otherwise we would not be able to handle cases
   // where the got equivalent shows up before its use.
   computeGlobalGOTEquivs(M);
 
   // Emit global variables.
   for (const auto &G : M.globals())
     EmitGlobalVariable(&G);
 
   // Emit remaining GOT equivalent globals.
   emitGlobalGOTEquivs();
if (Alias.hasExternalLinkage() || !MAI->getWeakRefDirective())
       OutStreamer->EmitSymbolAttribute(Name, MCSA_Global);
     else if (Alias.hasWeakLinkage() || Alias.hasLinkOnceLinkage())
       OutStreamer->EmitSymbolAttribute(Name, MCSA_WeakReference);
     else
       assert(Alias.hasLocalLinkage() && "Invalid alias linkage");
 
     // Set the symbol type to function if the alias has a function type.
     // This affects codegen when the aliasee is not a function.
-    if (Alias.getType()->getPointerElementType()->isFunctionTy())
+    if (Alias.getType()->getPointerElementType()->isFunctionTy()) {
       OutStreamer->EmitSymbolAttribute(Name, MCSA_ELF_TypeFunction);
+      if (Alias.isIFunc())
+        OutStreamer->EmitSymbolAttribute(Name, MCSA_ELF_TypeIndFunction);
+    }
 
     EmitVisibility(Name, Alias.getVisibility());
 
     // Emit the directives as assignments aka .set:
     OutStreamer->EmitAssignment(Name, lowerConstant(Alias.getAliasee()));
 
     // If the aliasee does not correspond to a symbol in the output, i.e. the
     // alias is not of an object or the aliased object is private, then set the

lib/IR/AsmWriter.cpp

void AssemblyWriter::printAlias(const GlobalAlias *GA) {
   PrintVisibility(GA->getVisibility(), Out);
   PrintDLLStorageClass(GA->getDLLStorageClass(), Out);
   PrintThreadLocalModel(GA->getThreadLocalMode(), Out);
   if (GA->hasUnnamedAddr())
     Out << "unnamed_addr ";
 
   Out << "alias ";
 
+  if (GA->isIFunc())
+    Out << "ifunc ";
+
   TypePrinter.print(GA->getValueType(), Out);
 
   Out << ", ";
 
   const Constant *Aliasee = GA->getAliasee();
 
   if (!Aliasee) {
     TypePrinter.print(GA->getType(), Out);

lib/IR/Globals.cpp

GlobalAlias::GlobalAlias(Type *Ty, unsigned AddressSpace, LinkageTypes Link,
                          const Twine &Name, Constant *Aliasee,
                          Module *ParentModule)
     : GlobalValue(Ty, Value::GlobalAliasVal, &Op<0>(), 1, Link, Name,
                   AddressSpace) {
   Op<0>() = Aliasee;
 
   if (ParentModule)
     ParentModule->getAliasList().push_back(this);
+
+  setIFunc(false);
 }
 
 GlobalAlias *GlobalAlias::create(Type *Ty, unsigned AddressSpace,
                                  LinkageTypes Link, const Twine &Name,
                                  Constant *Aliasee, Module *ParentModule) {
   return new GlobalAlias(Ty, AddressSpace, Link, Name, Aliasee, ParentModule);
 }
 
void GlobalAlias::eraseFromParent() {
   getParent()->getAliasList().erase(getIterator());
 }
 
 void GlobalAlias::setAliasee(Constant *Aliasee) {
   assert((!Aliasee || Aliasee->getType() == getType()) &&
          "Alias and aliasee types should match!");
   setOperand(0, Aliasee);
 }
+
+void GlobalAlias::copyAttributesFrom(const GlobalValue *Src) {
+  GlobalValue::copyAttributesFrom(Src);
+  if (const auto *GV = dyn_cast<GlobalAlias>(Src))
+    setIFunc(GV->isIFunc());
+}

test/Assembler/ifunc-alias.ll

+; RUN: llvm-as < %s | llvm-dis | FileCheck %s --check-prefix=CHECK-LLVM
+; RUN: llvm-as < %s -o - | llc -filetype=asm | FileCheck %s --check-prefix=CHECK-ASM
+
+target triple = "x86_64-unknown-linux-gnu"
+
+@foo = linkonce alias ifunc i32 (i32), bitcast (i64 ()* @foo_ifunc to i32 (i32)*)
+; CHECK-LLVM: @foo = linkonce alias ifunc i32 (i32), bitcast (i64 ()* @foo_ifunc to i32 (i32)*)
+; CHECK-ASM: .type   foo,@gnu_indirect_function
+
+define internal i64 @foo_ifunc() {
+entry:
+  ret i64 0
+}
+; CHECK-LLVM: define internal i64 @foo_ifunc()