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

Wed Jun 17 09:08:45 PDT 2020

tejohnson added a subscriber: DmitryPolukhin.
tejohnson added a comment.

In D81911#2097766 <https://reviews.llvm.org/D81911#2097766>, @nextsilicon-itay-bookstein wrote:

> 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.

My knowledge of IFunc details is pretty thin. Adding @DmitryPolukhin who ported over ifunc support from gcc and might be able to provide some guidance. I agree it makes sense to have the use in computeAliasSummary check for the null with an assert. And if the usage you describe in 3 is illegal, it would be good to have the verifier check for this.

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

https://reviews.llvm.org/D81911