Page MenuHomePhabricator

Fix for nasty LTO issue that's most obvious in ObjectiveC.
ClosedPublic

Authored by freik on Nov 5 2014, 3:52 PM.

Details

Summary

The LTO module linker is dropping the 'isExternallyInitialized' attribute from GlobalVariable's as it's copying them into the One Module to Rule Them All. This results in later phases believing that values can be copy-prop'ed or pre-evaluated. The second change is the one that actually fixes the problem, but the first change looks like a similar oversight, so I also changed it to include the attribute.

This bug prevents LTO from being usable by the Facebook iOS app.

Diff Detail

Event Timeline

freik updated this revision to Diff 15827.Nov 5 2014, 3:52 PM
freik retitled this revision from to Fix for nasty LTO issue that's most obvious in ObjectiveC..
freik updated this object.
freik edited the test plan for this revision. (Show Details)
freik added a reviewer: abdulras.
freik set the repository for this revision to rL LLVM.
freik added a subscriber: Unknown Object (MLST).
freik updated this revision to Diff 15836.Nov 5 2014, 3:56 PM
freik set the repository for this revision to rL LLVM.

Missed the new test file...

rnk added a subscriber: rnk.Nov 5 2014, 4:43 PM

This seems like an omission from GlobalVariable::copyAttributesFrom(), which the linker already calls.

freik added a comment.Nov 5 2014, 8:42 PM

@rnk: I considered both, and am new to the codebase, so the 'correct' place to fix this wasn't apparent. Does anyone else have an opinion, here? Either way is effective, and pretty straightforward as well.

In D6145#11, @freik wrote:

@rnk: I considered both, and am new to the codebase, so the 'correct' place to fix this wasn't apparent. Does anyone else have an opinion, here? Either way is effective, and pretty straightforward as well.

I think the idea is that copyAttributesFrom is intended to handle all attributes which are defaulted when a GlobalObject is created.

Now that I am reviewing GlobalObject::copyAttributesFrom, it looks like we forgot to handle Comdat there as well.

freik added a comment.Nov 6 2014, 9:22 AM

The reason I wasn't sure which was correct is because isExternallyIntialized is a default ctor parameter, so if someone's specifying a different value, then calling the copyAttributesFrom to fill in everything else, it's a bug. From a design perspective, having an API like copyAttributesFrom for a type with a ctor that has default parameters seems incredibly confusing. Perhaps I should eliminate the default parameter? And maybe this discussion should move over to llvmdev?

freik added a comment.Nov 6 2014, 12:44 PM

Fair enough. I'll go ahead & update the diff with the fix moved into copyAttributesFrom, and fix up handful of missing attributes in GlobalObject and GlobalValue, too.

freik updated this revision to Diff 15944.Nov 7 2014, 4:05 PM
freik set the repository for this revision to rL LLVM.

[Responding to feedback]
Updated the test case to be better directed (specifically testing externally_initialized and appending attributes). Moved the fix to be in GlobalVariable::copyAttributesFrom instead of at the copy site.

ab added a subscriber: ab.Nov 7 2014, 4:59 PM
rafael accepted this revision.Nov 10 2014, 7:25 AM
rafael edited edge metadata.

LGTM. Do you need me to commit it?

This revision is now accepted and ready to land.Nov 10 2014, 7:25 AM
freik added a comment.Nov 10 2014, 7:33 AM

Yup. Thanks!