[PATCH] D81911: [IR] Fix getBaseObject for GlobalAlias-to-GlobalIFunc

[Itay Bookstein via Phabricator via llvm-commits]

nextsilicon-itay-bookstein updated this revision to Diff 271316.
nextsilicon-itay-bookstein added a comment.
Herald added subscribers: dexonsmith, steven_wu.

I added a test case that exercises the crash before the fix.
However, your comment made me realize that there are two separate issues + one question here:

1. getBaseObject() is currently unable to indirect through GlobalIFuncs (as the current fix tries to address).
2. computeAliasSummary() assumes that getBaseObject() can never return nullptr, while it has potential flows that return nullptr. In Verifier::visitGlobalAlias() I see that aliases-to-ConstantExpr-s are valid, but in Verifier::visitAliaseeSubExpr I see that alias cycles are invalid. If we assume that the module for which we generate a summary is valid, this question amounts to whether it is valid for the ConstantExpr case of an alias to ever return nullptr there. I can add an assert(Aliasee) / fatal(..) in computeAliasSummary.
3. Is it valid for a GlobalIFunc to have an alias as its resolver function operand? It sounds contrived, but valid nonetheless (e.g. if the resolver is in a different translation unit and they end up getting merged in LTO?). A GlobalIFunc with a resolver that is itself a GlobalIFunc sounds like an invalid construction to me, though. I don't see references to GlobalIFunc in Verifier.cpp to use as guiding principles for what would be considered valid. So, it sounds to me that with respect to module validity, in an alias-...-alias-ifunc-alias-...-resolverfunc chain there should only ever be a single ifunc at most.

Regardless, changing the findBaseObject implementation to take a set of GlobalIndirectSymbols and uniformizing that handling sounds like a fine alternative to the currently proposed fix, depending (?) on the points I raised above.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81911/new/

https://reviews.llvm.org/D81911

Files:
  llvm/lib/IR/Globals.cpp
  llvm/test/Bitcode/thinlto-summary-alias-ifunc.ll


Index: llvm/test/Bitcode/thinlto-summary-alias-ifunc.ll
===================================================================
--- /dev/null
+++ llvm/test/Bitcode/thinlto-summary-alias-ifunc.ll
@@ -0,0 +1,19 @@
+; RUN: opt -module-summary %s -o - | llvm-bcanalyzer -dump | FileCheck %s
+
+; PR46340
+; Check that alias-to-ifunc is properly generated in the module summary.
+
+; CHECK: <GLOBALVAL_SUMMARY_BLOCK
+; CHECK: <ALIAS
+; CHECK: </GLOBALVAL_SUMMARY_BLOCK>
+
+ at bar = dso_local alias void (), void ()* @foo
+ at foo = dso_local ifunc void (), bitcast (i8* ()* @foo_ifunc to void ()*)
+
+define internal i8* @foo_ifunc() {
+  ret i8* bitcast (void ()* @foo_impl to i8*)
+}
+
+define internal void @foo_impl() {
+  ret void
+}
Index: llvm/lib/IR/Globals.cpp
===================================================================
--- llvm/lib/IR/Globals.cpp
+++ llvm/lib/IR/Globals.cpp
@@ -444,6 +444,8 @@
 findBaseObject(const Constant *C, DenseSet<const GlobalAlias *> &Aliases) {
   if (auto *GO = dyn_cast<GlobalObject>(C))
     return GO;
+  if (auto *GI = dyn_cast<GlobalIFunc>(C))
+    return findBaseObject(GI->getOperand(0), Aliases);
   if (auto *GA = dyn_cast<GlobalAlias>(C))
     if (Aliases.insert(GA).second)
       return findBaseObject(GA->getOperand(0), Aliases);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D81911.271316.patch
Type: text/x-patch
Size: 1276 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200617/31501461/attachment.bin>