- User Since
- Jun 10 2020, 10:51 AM (19 w, 6 d)
Sep 7 2020
I have updated the diff to include the changes you suggested. However, as you said, for the test to have a chance of passing, D82745 is required. I will send out a mail to discuss the design issue on llvm-dev soon.
Sep 3 2020
The problem with what @DmitryPolukhin suggested (which he later acknowledged in an additional comment), is that getBaseObject() returns a GlobalObject, but GlobalIFunc does not inherit from GlobalObject, so that can't compile.
Aug 19 2020
@tejohnson would simply not emitting an AliasSummary in computeAliasSummary when getBaseObject() returns nullptr be sufficient, then?
Jul 8 2020
Jul 7 2020
How do we want to proceed, then? Does my analysis make sense?
Jun 24 2020
As far as I can tell, GlobalIndirectSymbol is there to share code between GlobalAlias and GlobalIFunc.
This shared code is mostly a common getBaseObject() implementation, which just delegates to getIndirectSymbol() (== getOperand(0)).
For GlobalAliases, the indirect symbol is the aliasee. For GlobalIFuncs, the indirect symbol is the resolver function.
Doesn't that mean that GlobalIFunc shouldn't really inherit from GlobalIndirectSymbol (and then GlobalIndirectSymbol loses the justification for its existence)?
I get the feeling like there's an expectation for getBaseObject to be idempotent (since it pierces through ConstantExprs and GlobalAliases), but GlobalIndirectSymbol::getBaseObject takes the resolver (getOperand(0)) and performs the lookup on it.
Used the exact same fix as a local workaround until further counsel on the bug report, so looks good to me.
Jun 17 2020
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:
- getBaseObject() is currently unable to indirect through GlobalIFuncs (as the current fix tries to address).
- 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.
- 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.
Jun 16 2020
Wondering what the most appropriate place for the test case is.
I can hit the crash by passing --module-summary to opt, but the crash is a bit removed from the underlying issue itself, I think.
Jun 15 2020
Alright, looks like it's settled and ready then :)
I cannot commit the patch myself because I don't have commit access, so might I trouble one of you with that?
Jun 12 2020
Updated title and description.
Added description for test, created PR in bugzilla, corrected PR reference to point at it.
Jun 11 2020
Changed the fix to push the ThreadLocalMode copying from derived class GlobalVariable down to base class GlobalValue.
Added an appropriate test under llvm/test/Linker.