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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
