Page MenuHomePhabricator

[ThinLTO] Fix bug when importing writeonly variables
ClosedPublic

Authored by evgeny777 on Fri, Nov 8, 6:04 AM.

Details

Summary

D69561introduced a bug which causes writeonly variable with non-trivial initializer (with references) to be internalized in source module and its declaration imported to destination module. This causes linker errors. To fix this bug I suggest importing such variable definitions, but convert them to zeroinitializer before doing so in order to avoid promotion of referenced objects which are not accessed anyway.

Diff Detail

Event Timeline

evgeny777 created this revision.Fri, Nov 8, 6:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 8, 6:04 AM

We hit a linker undef also because of D69561. I just confirmed this patch fixes the issue. It would be good to get this patch in asap or revert the original change. I have some questions on this patch though.

I'm a little confused about what causes the original failure mode, and whether this patch is an enhancement that essentially hides the underlying bug or is really fixing the underlying issue. Since D69561 only affected read only variables, how did it affect a write only variable?

llvm/lib/Transforms/IPO/FunctionImport.cpp
321–328

Needs comment

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
258

Needs comment

260

What if it isn't a struct or array? Is that the only situation where we can hit this problem?

I'm a little confused about what causes the original failure mode

Assuming we have writeonly variable foo with non-trivial initializer, the following happens after D69561

  1. foo is marked as writeonly due to change in canImportGlobalVar
  2. computeImportForReferencedGlobals refuses to import foo because !isReadOnly(GVS) && GVS->refs().size() evaluates to true. See HasReferencesPreventingImport
  3. Import of function which uses foo causes declaration of foo to be exported to destination module
  4. foo is internalized in source module because due to being "writeonly"
evgeny777 marked an inline comment as done.EditedFri, Nov 8, 8:06 AM
This comment has been deleted.
llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
260

Looks like you're right. I'll replace with Constant::getNullValue and update the patch

evgeny777 updated this revision to Diff 228460.Fri, Nov 8, 8:12 AM

Addressed

Ah, ok, I wasn't thinking about the part where the flag propagation was changed. I'm a little concerned that due to that change we are now required to import the variable as a definition. I guess there is no current way to disable importing of references while leaving on importing of functions (e.g. through some kind of internal option for debugging), but since such a mechanism might be useful at some point for debugging, my concern is that it will cause failures. Please add some very clear comments to the code where you are checking the write only flag as to why and what is going on. I.e. why needed for correctness and how we can ignore its references (because it will eventually be made zeroinit). Also, it would be good to add an assert somewhere to ensure that we don't export a reference but not a definition for a write only variable, to guard against any regression if someone does add a debugging mechanism to disable variable importing (which would need to affect the flag propagation). Maybe down at the bottom of ComputeCrossModuleImport where we prune the ExportLists.

A few suggestions for beefing up the comments

llvm/lib/IR/ModuleSummaryIndex.cpp
200–214

Please add comment about why refs on write only vars must be ignored for correctness, and that we must import their defs.

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
261

Please add a note that we have to do this as the variable importing code does not mark these references as exported.

I think this is probably the best fix I can think as well. Agree with Teresa says, please add comments for reasons why it is implemented this way.

evgeny777 updated this revision to Diff 228476.Fri, Nov 8, 9:31 AM

Addressed

tejohnson accepted this revision.Fri, Nov 8, 9:40 AM

Let's get this one in (I have a couple more comment nits below), to fix the immediate problem. But please send a follow on patch soon to do the assertion checking I suggested in my earlier comment, to make sure we catch any regressions to the force import of these write only variables.

llvm/lib/IR/ModuleSummaryIndex.cpp
204

Add that this is because we don't AnalyzeRefs when propagating the write only property.

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
260

Please add that this is because we do not export their references.

This revision is now accepted and ready to land.Fri, Nov 8, 9:40 AM

But please send a follow on patch soon to do the assertion checking I suggested in my earlier comment

Just pushed this in. It's getting late here, so feel free to revert if things go bad. If not, I'll send follow-up with assertion on Monday.

This revision was automatically updated to reflect the committed changes.