Resolve {GlobalValue,GloalIndirectSymol}::getBaseObject confusion

[MaskRay]

Summary

While both GlobalAlias and GlobalIFunc are GlobalIndirectSymbol, their
getIndirectSymbol() usage is quite different (GlobalIFunc's resolver
is an entity different from GlobalIFunc itself).

As discussed on https://lists.llvm.org/pipermail/llvm-dev/2020-September/144904.html
("[IR] Modelling of GlobalIFunc"), the name getBaseObject is confusing when
used with GlobalIFunc.

To resolve the confusion:

• Move GloalIndirectSymol::getBaseObject to GlobalAlias:: (GlobalIFunc should use getResolver instead)
• Change GlobalValue::getBaseObject not to inspect GlobalIFunc. Note: the function has 7 references.
• Add GlobalIFunc::getResolverFunction to peel off potential ConstantExpr indirection (strlen in test/LTO/Resolution/X86/ifunc.ll)

Note: GlobalIFunc::getResolver (like GlobalAlias::getAliasee which does not peel
off ConstantExpr indirection) is kept to be used by ValueEnumerator.

[ibookstein]

Added a couple of comments; I think this change breaks code that wants to get the resolver Function itself because it deprives such code from the ConstantExpr-peeling machinery in findBaseObject.

llvm/include/llvm/IR/GlobalIndirectSymbol.h

65 ↗

(On Diff #372582)

When creating my own commit, I noticed that the two overloads below (with DataLayout and APInt) are completely unused. Perhaps we can use this opportunity to remove them?

llvm/lib/Object/IRSymtab.cpp

289

This will crash on bitcasts, see e.g. https://clang.godbolt.org/z/ssxrj9b7d
In other words, code that wants to deal with getting the resolver of a GlobalIFunc took a reliance upon the ConstantExpr-peeling machinery of findBaseObject() that was previously utilized only by GlobalAlias prior to the introduction of GlobalIFunc. It is unclear to me whether all the ConstantExpr handlings in findBaseObject() are needed in the case of GlobalIFunc-s.

llvm/lib/Transforms/Utils/SplitModule.cpp

133

Same as above, will crash on bitcasts

[MaskRay]

add a strlen test
fix ConstantExpr issue

[MaskRay]

llvm/lib/Object/IRSymtab.cpp

289

Thanks for the example. Added strlen to llvm/test/LTO/Resolution/X86/ifunc.ll to catch the regression.

[MaskRay]

Ping @ibookstein

[ibookstein]

I've added one additional minor comment; other than that, I think this makes good progress in resolving the ambiguity currently in trunk.
I still think that to be able to express alias-to-ifunc situations (i.e. @foo1 = ifunc i32 (i32), i64 ()* @foo_resolver; @foo2 = alias i32 (i32), i32 (i32)* @foo1) we'll need to consider https://reviews.llvm.org/D108872, but that's a problem for another day (and I'd appreciate your opinion on that direction).

A couple of questions:

1. This effectively turns getBaseObject() into getAliaseeObject(), so it might make sense to perform that rename in a later commit - WDYT?
2. If I understand you correctly, calls to GlobalValue::getBaseObject() are exceedingly rare. Would it make sense to try and remove it from the base class so that only GlobalIFunc::getResolverFunction() and GlobalAlias::getAliaseeObject() remain? I like the explicitness that that would create, but I'm wondering whether that sacrifices utility too much.

llvm/lib/IR/Globals.cpp

467

Should also remove the declaration then, no?

[MaskRay]

Thanks:) Will push in two days unless someone objects.

In D109792#3013421, @ibookstein wrote:

I've added one additional minor comment; other than that, I think this makes good progress in resolving the ambiguity currently in trunk.
I still think that to be able to express alias-to-ifunc situations (i.e. @foo1 = ifunc i32 (i32), i64 ()* @foo_resolver; @foo2 = alias i32 (i32), i32 (i32)* @foo1) we'll need to consider https://reviews.llvm.org/D108872, but that's a problem for another day (and I'd appreciate your opinion on that direction).

Inheriting from GlobalObject makes sense to me, since we won't use any facility from GlobalIndirectSymbol.

A couple of questions:

1. This effectively turns getBaseObject() into getAliaseeObject(), so it might make sense to perform that rename in a later commit - WDYT?

Makes sense to me.

1. If I understand you correctly, calls to GlobalValue::getBaseObject() are exceedingly rare. Would it make sense to try and remove it from the base class so that only GlobalIFunc::getResolverFunction() and GlobalAlias::getAliaseeObject() remain? I like the explicitness that that would create, but I'm wondering whether that sacrifices utility too much.

Yes, I think removing GlobalValue::getBaseObject() will be worthwhile. My quick glance at the call sites suggest that they don't use the GlobalIFunc::getResolverFunction() facility.

llvm/include/llvm/IR/GlobalAlias.h

public:
   void setAliasee(Constant *Aliasee);
   const Constant *getAliasee() const {
     return getIndirectSymbol();
   }
   Constant *getAliasee() {
     return getIndirectSymbol();
   }
 
+  const GlobalObject *getBaseObject() const;
+  GlobalObject *getBaseObject() {
+    return const_cast<GlobalObject *>(
+        static_cast<const GlobalAlias *>(this)->getBaseObject());
+  }
+
   static bool isValidLinkage(LinkageTypes L) {
     return isExternalLinkage(L) || isLocalLinkage(L) ||
       isWeakLinkage(L) || isLinkOnceLinkage(L);
   }
 
   // Methods for support type inquiry through isa, cast, and dyn_cast:
   static bool classof(const Value *V) {
     return V->getValueID() == Value::GlobalAliasVal;
   }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_IR_GLOBALALIAS_H

llvm/include/llvm/IR/GlobalIFunc.h

public:
 
   /// This method unlinks 'this' from the containing module and deletes it.
   void eraseFromParent();
 
   /// These methods retrieve and set ifunc resolver function.
   void setResolver(Constant *Resolver) {
     setIndirectSymbol(Resolver);
   }
-  const Constant *getResolver() const {
-    return getIndirectSymbol();
-  }
-  Constant *getResolver() {
-    return getIndirectSymbol();
+  const Constant *getResolver() const { return getIndirectSymbol(); }
+  Constant *getResolver() { return getIndirectSymbol(); }
+
+  // Return the resolver function after peeling off potential ConstantExpr
+  // indirection.
+  const Function *getResolverFunction() const;
+  Function *getResolverFunction() {
+    return const_cast<Function *>(
+        static_cast<const GlobalIFunc *>(this)->getResolverFunction());
   }
 
   // Methods for support type inquiry through isa, cast, and dyn_cast:
   static bool classof(const Value *V) {
     return V->getValueID() == Value::GlobalIFuncVal;
   }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_IR_GLOBALIFUNC_H

llvm/lib/IR/Globals.cpp

if (isELF && !isDSOLocal())
     return false;
 
   return true;
 }
 
 const GlobalObject *GlobalValue::getBaseObject() const {
   if (auto *GO = dyn_cast<GlobalObject>(this))
     return GO;
-  if (auto *GA = dyn_cast<GlobalIndirectSymbol>(this))
+  if (auto *GA = dyn_cast<GlobalAlias>(this))
     return GA->getBaseObject();
   return nullptr;
 }
 
 bool GlobalValue::isAbsoluteSymbolRef() const {
   auto *GO = dyn_cast<GlobalObject>(this);
   if (!GO)
     return false;
case Instruction::GetElementPtr:
       return findBaseObject(CE->getOperand(0), Aliases);
     default:
       break;
     }
   }
   return nullptr;
 }
 
-const GlobalObject *GlobalIndirectSymbol::getBaseObject() const {
-  DenseSet<const GlobalAlias *> Aliases;
-  return findBaseObject(getOperand(0), Aliases);
-}
-
 //===----------------------------------------------------------------------===//
 // GlobalAlias Implementation
 //===----------------------------------------------------------------------===//
 
 GlobalAlias::GlobalAlias(Type *Ty, unsigned AddressSpace, LinkageTypes Link,
                          const Twine &Name, Constant *Aliasee,
                          Module *ParentModule)
     : GlobalIndirectSymbol(Ty, Value::GlobalAliasVal, AddressSpace, Link, Name,

 }
 
 void GlobalAlias::setAliasee(Constant *Aliasee) {
   assert((!Aliasee || Aliasee->getType() == getType()) &&
          "Alias and aliasee types should match!");
   setIndirectSymbol(Aliasee);
 }
 
+const GlobalObject *GlobalAlias::getBaseObject() const {
+  DenseSet<const GlobalAlias *> Aliases;
+  return findBaseObject(getOperand(0), Aliases);
+}
+
 //===----------------------------------------------------------------------===//
 // GlobalIFunc Implementation
 //===----------------------------------------------------------------------===//
 
 GlobalIFunc::GlobalIFunc(Type *Ty, unsigned AddressSpace, LinkageTypes Link,
                          const Twine &Name, Constant *Resolver,
                          Module *ParentModule)
     : GlobalIndirectSymbol(Ty, Value::GlobalIFuncVal, AddressSpace, Link, Name,

 
 void GlobalIFunc::removeFromParent() {
   getParent()->getIFuncList().remove(getIterator());
 }
 
 void GlobalIFunc::eraseFromParent() {
   getParent()->getIFuncList().erase(getIterator());
 }
+
+const Function *GlobalIFunc::getResolverFunction() const {
+  DenseSet<const GlobalAlias *> Aliases;
+  return cast<Function>(findBaseObject(getIndirectSymbol(), Aliases));
+}

llvm/lib/Object/IRSymtab.cpp

if (!GVar)
       return make_error<StringError>("Only variables can have common linkage!",
                                      inconvertibleErrorCode());
     Uncommon().CommonSize =
         GV->getParent()->getDataLayout().getTypeAllocSize(GV->getValueType());
     Uncommon().CommonAlign = GVar->getAlignment();
   }
 
   const GlobalObject *Base = GV->getBaseObject();
+  if (!Base) {
+    if (isa<GlobalIFunc>(GV))
+      Base = cast<GlobalIFunc>(GV)->getResolverFunction();
     if (!Base)
       return make_error<StringError>("Unable to determine comdat of alias!",
                                      inconvertibleErrorCode());
+  }
   if (const Comdat *C = Base->getComdat()) {
     Expected<int> ComdatIndexOrErr = getComdatIndex(C, GV->getParent());
     if (!ComdatIndexOrErr)
       return ComdatIndexOrErr.takeError();
     Sym.ComdatIndex = *ComdatIndexOrErr;
   }
 
   if (TT.isOSBinFormatCOFF()) {

llvm/lib/Object/ModuleSymbolTable.cpp

uint32_t ModuleSymbolTable::getSymbolFlags(Symbol S) const {
   if (GV->isDeclarationForLinker())
     Res |= BasicSymbolRef::SF_Undefined;
   else if (GV->hasHiddenVisibility() && !GV->hasLocalLinkage())
     Res |= BasicSymbolRef::SF_Hidden;
   if (const GlobalVariable *GVar = dyn_cast<GlobalVariable>(GV)) {
     if (GVar->isConstant())
       Res |= BasicSymbolRef::SF_Const;
   }
-  if (dyn_cast_or_null<Function>(GV->getBaseObject()))
+  if (isa_and_nonnull<Function>(GV->getBaseObject()) || isa<GlobalIFunc>(GV))
     Res |= BasicSymbolRef::SF_Executable;
   if (isa<GlobalAlias>(GV))
     Res |= BasicSymbolRef::SF_Indirect;
   if (GV->hasPrivateLinkage())
     Res |= BasicSymbolRef::SF_FormatSpecific;
   if (!GV->hasLocalLinkage())
     Res |= BasicSymbolRef::SF_Global;
   if (GV->hasCommonLinkage())

llvm/lib/Transforms/Utils/SplitModule.cpp

if (const Comdat *C = GV.getComdat()) {
       if (Member)
         GVtoClusterMap.unionSets(Member, &GV);
       else
         Member = &GV;
     }
 
     // For aliases we should not separate them from their aliasees regardless
     // of linkage.
-    if (auto *GIS = dyn_cast<GlobalIndirectSymbol>(&GV)) {
+    if (auto *GIS = dyn_cast<GlobalAlias>(&GV)) {
       if (const GlobalObject *Base = GIS->getBaseObject())
         GVtoClusterMap.unionSets(&GV, Base);
+    } else if (auto *GIS = dyn_cast<GlobalIFunc>(&GV)) {
+      GVtoClusterMap.unionSets(&GV, GIS->getResolverFunction());
     }
 
     if (const Function *F = dyn_cast<Function>(&GV)) {
       for (const BasicBlock &BB : *F) {
         BlockAddress *BA = BlockAddress::lookup(&BB);
         if (!BA || !BA->isConstantUsed())
           continue;
         addAllGlobalValueUsers(GVtoClusterMap, F, BA);
       }
static void externalize(GlobalValue *GV) {
   // Unnamed entities must be named consistently between modules. setName will
   // give a distinct name to each such entity.
   if (!GV->hasName())
     GV->setName("__llvmsplit_unnamed");
 }
 
 // Returns whether GV should be in partition (0-based) I of N.
 static bool isInPartition(const GlobalValue *GV, unsigned I, unsigned N) {
-  if (auto *GIS = dyn_cast<GlobalIndirectSymbol>(GV))
+  if (auto *GIS = dyn_cast<GlobalAlias>(GV)) {
     if (const GlobalObject *Base = GIS->getBaseObject())
       GV = Base;
+  } else if (auto *GIS = dyn_cast<GlobalIFunc>(GV)) {
+    GV = GIS->getResolverFunction();
+  }
 
   StringRef Name;
   if (const Comdat *C = GV->getComdat())
     Name = C->getName();
   else
     Name = GV->getName();
 
   // Partition by MD5 hash. We only need a few bits for evenness as the number

llvm/test/LTO/Resolution/X86/ifunc.ll

 ; RUN: opt -module-summary -o %t.bc %s
-; RUN: llvm-lto2 run %t.bc -r %t.bc,foo,pl -o %t2
+; RUN: llvm-lto2 run %t.bc -r %t.bc,foo,pl -r %t.bc,strlen,pl -o %t2
 ; RUN: llvm-nm %t2.1 | FileCheck %s
 ; CHECK: i foo
-; CHECK: t foo_ifunc
+; CHECK: t foo_resolver
+; CHECK: i strlen
+; CHECK: t strlen_resolver
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
-@foo = ifunc i32 (i32), i64 ()* @foo_ifunc
+@foo = ifunc i32 (i32), i64 ()* @foo_resolver
+@strlen = ifunc i64 (i8*), bitcast (i64 (i8*)* ()* @strlen_resolver to i64 (i8*)*)
 
-define internal i64 @foo_ifunc() {
+define internal i64 @foo_resolver() {
 entry:
   ret i64 0
 }
+
+define internal i64 (i8*)* @strlen_resolver() {
+entry:
+  ret i64 (i8*)* null
+}