[PATCH] D119409: [C++20] [Modules] Remain dynamic initializing internal-linkage variables in module interface unit

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D119409#3432281 <https://reviews.llvm.org/D119409#3432281>, @iains wrote:

> In D119409#3410893 <https://reviews.llvm.org/D119409#3410893>, @ChuanqiXu wrote:
>
>> In D119409#3410868 <https://reviews.llvm.org/D119409#3410868>, @iains wrote:
>>
>>> In D119409#3410474 <https://reviews.llvm.org/D119409#3410474>, @ChuanqiXu wrote:
>>>
>>>> In D119409#3409806 <https://reviews.llvm.org/D119409#3409806>, @iains wrote:
>>>>
>>>>> I think that this problem might well be a consequence of the bug which is fixed by D122413 <https://reviews.llvm.org/D122413>.
>>>>>
>>>>> We have been generating code with module internal entities (always) given the special ModuleInternalLinkage (which means that, although the linkage is formally 'internal', the entities are made global when emitted.  We should only be doing this for fmodules-ts, not for regular standard modules.
>>>>>
>>>>> If you apply D122413 <https://reviews.llvm.org/D122413> (which I hope to land soon), then I would expect that iostream should work as expected (with one internal instance of std::__ioinit in each TU that includes iostream).
>>>>>
>>>>> IFF (after applying D122413 <https://reviews.llvm.org/D122413> ) you add to the command line -fmodules-ts, then the patch here (D119409 <https://reviews.llvm.org/D119409>) would, presumably, be needed to work around multiple instances of the globalised std::__ioinit.
>>>>
>>>> Sadly it wouldn't work after D122413 <https://reviews.llvm.org/D122413> applied. Since the <iostream> is lived in GlobalModuleFragment, the calculated linkage wouldn't affect them. So I met the same segfault as before.
>>>
>>> Is this because we are not creating an initialiser for a static entity in the GMF ?
>>>
>>> - I did a quick test and that seemed to be the case.
>>
>> I think we need this one finally, It would create the initialiser after the patch applied. And I think we couldn't do that without the patch. Since from the code we could see that the static variable wouldn't be generated in the current strategies.
>>
>>> (module initialisers need quite some work, it seems)
>>
>> The initialiser above I said is the initialiser in that TU. What you mean `module initializer` ? Do you mean the one module could have only module initializer?
>>
>>>>> addendum: note we still have work to do on the module initialisers - those are not correct yet (so probably some nesting of modules might not work).
>>>>
>>>> What does the nesting of modules mean?
>>>
>>> If we have an import of a module that imports another - then we should be running the module initializers for the imported stack (in the correct order) .. at present, we do not do this.
>>> As noted above, we have some work to do here.
>>
>> I am not familiar with the history here. But I found http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1874r1.html#solution. It says clang already has a simple fix. So I am wondering if this one is already fixed or we are not talking about the same thing?
>
> Sorry for slow response - was hoping to have a patch to show by now
> ... I am currently finishing off an implementation for 1874 + elision of unused GMF decls.  The "simple fix" in clang actually pulls all the initialisers into one list in the top-level module (which is slightly different from the intent of the paper, as implemented in GCC) - that implementation also does not fix the issue of a module interface where there is no explicit "import" for the sub-modules.
>
> The patch in progress will build a per module initialiser (which will handle sub-module initialisers and calling module initialisers for direct imports).  I think that will fix the iostreams problem (it is a motivating example from the paper).

Cool. BTW, I want to be sure that it would solve the problem this patch shows. I mean the problem presented in this patch is about named module, while the motivating example from that paper is about header unit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119409/new/

https://reviews.llvm.org/D119409