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.
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?
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
- foo is marked as writeonly due to change in canImportGlobalVar
- computeImportForReferencedGlobals refuses to import foo because !isReadOnly(GVS) && GVS->refs().size() evaluates to true. See HasReferencesPreventingImport
- Import of function which uses foo causes declaration of foo to be exported to destination module
- foo is internalized in source module because due to being "writeonly"
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
Please add comment about why refs on write only vars must be ignored for correctness, and that we must import their defs.
Please add a note that we have to do this as the variable importing code does not mark these references as exported.
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.
Add that this is because we don't AnalyzeRefs when propagating the write only property.
Please add that this is because we do not export their references.
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.