iains added a comment. 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). 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
