v.g.vassilev added a comment. In https://reviews.llvm.org/D34444#812418, @rjmccall wrote:
> In https://reviews.llvm.org/D34444#795175, @v.g.vassilev wrote: > > > @rjmccall, thanks for the prompt and thorough reply. > > > > In https://reviews.llvm.org/D34444#793311, @rjmccall wrote: > > > > > Okay. In that case, I see two problems, one major and one potentially > > > major. > > > > > > > > > > This is a very accurate diagnosis which took us 5 years to discover on an > > empirical basis ;) > > > You could've asked at any time. :) True. I am not really sure I knew what to ask, though ;) > > >>> The major problem is that, as Richard alludes to, you need to make sure you >>> emit any on-demand definitions that Sema registered with CodeGen during the >>> initial CGM's lifetime but used in later CGMs. >> >> >> >> We bring the CGM state to the subsequent CGMs. See >> https://github.com/vgvassilev/clang/blob/cling-patches/lib/CodeGen/ModuleBuilder.cpp#L138-L160 > > That's quite brittle, because that code is only executed in a code path that > only you are using, and you're not adding any tests. I would greatly prefer > a change to IRGen's core assumptions, as suggested. I am open to changing this code as well. That should probably be another review. > > >>> The potentially major problem is that it is not possible in general to >>> automatically break up a single translation unit into multiple translation >>> units, for two reasons. The big reason is that there is no way to >>> correctly handle entities with non-external linkage that are referenced >>> from two parts of the translation unit without implicitly finding some way >>> to promote them to external linkage, which might open a huge can of worms >>> if you have multiple "outer" translation units. >> >> >> >> We do not have an multiple 'outer' translation units. We have just one >> ever growing TU (which probably invalidates my previous statement that we >> have a distinct TUs) which we send to the RuntimeDyLD allowing only JIT to >> resolve symbols from it. We aid the JIT when resolving symbols with >> internal linkage by changing all internal linkage to external (We haven't >> seen issues with that approach). > > Ah, okay. Yes, that is a completely different translation model from having > distinct TUs. > > IRGen will generally happily emit references to undefined internal objects; > instead of hacking the linkage, you could just clean that up as a post-pass. > Although hacking the linkage as post-pass is reasonable, too. In either > case, you can recognize uses of internal-linkage objects that haven't been > defined yet and report that back to the user. > >>> The lesser reason is that the prefix of a valid translation unit is not >>> necessarily a valid translation unit: for example, a static or inline >>> function can be defined at an arbitrary within the translation unit, i.e. >>> not necessarily before its first use. But if your use case somehow defines >>> away these problems, this might be fine. >> >> >> >> If we end up with a module containing no definition of a symbol and such >> is required, then we complain. So indeed we are defining away this issue. > > Ok. > >>> As a minor request, if you are going to make HandleTranslationUnit no >>> longer the final call on CodeGenerator, please either add a new method that >>> *is* a guaranteed final call or add a new method that does the whole "end a >>> previous part of the translation unit and start a new one" step. >> >> >> >> We can have this but it would be a copy of `HandleTranslationUnit`. The >> `StartModule` interface is the antagonist routine to `ReleaseModule`. If you >> prefer we could merge `StartModule` into `ReleaseModule` adding a flag (or >> reading the value of `isIncrementalProcessingEnabled`). > > I feel it is important that there be a way to inform an ASTConsumer that no > further requests will be made of it, something other than calling its > destructor. I would like you to make sure that the ASTConsumer interface > supports that and that that call is not made too soon in your alternate > processing mode. Do you have a preference of a name of this new interface? Repository: rL LLVM https://reviews.llvm.org/D34444 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
