Page MenuHomePhabricator

nextsilicon-itay-bookstein (Itay Bookstein)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 10 2020, 10:51 AM (19 w, 6 d)

Recent Activity

Sep 7 2020

nextsilicon-itay-bookstein updated the diff for D81911: [ThinLTO] Work around getBaseObject returning null for alias-to-ifunc.

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 7 2020, 9:38 AM · Restricted Project
nextsilicon-itay-bookstein retitled D81911: [ThinLTO] Work around getBaseObject returning null for alias-to-ifunc from [IR] Fix getBaseObject for GlobalAlias-to-GlobalIFunc to [ThinLTO] Work around getBaseObject returning null for alias-to-ifunc.
Sep 7 2020, 9:34 AM · Restricted Project

Sep 3 2020

nextsilicon-itay-bookstein added a comment to D81911: [ThinLTO] Work around getBaseObject returning null for alias-to-ifunc.

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.

Sep 3 2020, 12:35 AM · Restricted Project

Aug 19 2020

nextsilicon-itay-bookstein added a comment to D81911: [ThinLTO] Work around getBaseObject returning null for alias-to-ifunc.

@tejohnson would simply not emitting an AliasSummary in computeAliasSummary when getBaseObject() returns nullptr be sufficient, then?

Aug 19 2020, 7:01 AM · Restricted Project

Jul 8 2020

nextsilicon-itay-bookstein added a comment to D81911: [ThinLTO] Work around getBaseObject returning null for alias-to-ifunc.

I did a bit of archeology and it turns out that getBaseObejct was part of moved from GlobalAlias to GlobalIndirectSymbol in https://github.com/llvm/llvm-project/commit/95549497ec8b5269f0439f12859537b7371b7c90
It looks like the simplest solution is to handle nullptr from getBaseObejct in computeAliasSummary...

Jul 8 2020, 12:40 AM · Restricted Project

Jul 7 2020

nextsilicon-itay-bookstein added a comment to D81911: [ThinLTO] Work around getBaseObject returning null for alias-to-ifunc.

How do we want to proceed, then? Does my analysis make sense?

Jul 7 2020, 12:18 PM · Restricted Project

Jun 24 2020

nextsilicon-itay-bookstein added a comment to D81911: [ThinLTO] Work around getBaseObject returning null for alias-to-ifunc.

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.

Jun 24 2020, 6:27 AM · Restricted Project
nextsilicon-itay-bookstein added a comment to D81911: [ThinLTO] Work around getBaseObject returning null for alias-to-ifunc.

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.

Jun 24 2020, 4:49 AM · Restricted Project
nextsilicon-itay-bookstein added a comment to D82433: [ELF] -r: don't parse @ (symbol versioning) for .symver inline asm in bitcode.

Used the exact same fix as a local workaround until further counsel on the bug report, so looks good to me.
Thank you!

Jun 24 2020, 12:29 AM · Restricted Project

Jun 17 2020

nextsilicon-itay-bookstein updated the diff for D81911: [ThinLTO] Work around getBaseObject returning null for alias-to-ifunc.

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.
Jun 17 2020, 3:44 AM · Restricted Project

Jun 16 2020

nextsilicon-itay-bookstein added a comment to D81911: [ThinLTO] Work around getBaseObject returning null for alias-to-ifunc.

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 16 2020, 7:09 AM · Restricted Project
nextsilicon-itay-bookstein created D81911: [ThinLTO] Work around getBaseObject returning null for alias-to-ifunc.
Jun 16 2020, 12:30 AM · Restricted Project

Jun 15 2020

nextsilicon-itay-bookstein added a comment to D81605: [IR] Add missing GlobalAlias copying of ThreadLocalMode attribute.

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 15 2020, 2:08 AM · Restricted Project

Jun 12 2020

nextsilicon-itay-bookstein updated the diff for D81605: [IR] Add missing GlobalAlias copying of ThreadLocalMode attribute.

Updated title and description.
Added description for test, created PR in bugzilla, corrected PR reference to point at it.

Jun 12 2020, 2:08 AM · Restricted Project
nextsilicon-itay-bookstein retitled D81605: [IR] Add missing GlobalAlias copying of ThreadLocalMode attribute from IR: Add missing GlobalAlias copying of ThreadLocalMode attribute to [IR] Add missing GlobalAlias copying of ThreadLocalMode attribute.
Jun 12 2020, 2:08 AM · Restricted Project

Jun 11 2020

nextsilicon-itay-bookstein updated the diff for D81605: [IR] Add missing GlobalAlias copying of ThreadLocalMode attribute.

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.

Jun 11 2020, 3:37 AM · Restricted Project

Jun 10 2020

nextsilicon-itay-bookstein created D81605: [IR] Add missing GlobalAlias copying of ThreadLocalMode attribute.
Jun 10 2020, 11:41 AM · Restricted Project