On Fri, Jun 19, 2015 at 3:15 PM, Adrian Prantl <[email protected]> wrote:
> > > On Jun 16, 2015, at 2:26 PM, Richard Smith <[email protected]> > wrote: > > > > On Tue, Jun 16, 2015 at 10:15 AM, Adrian Prantl <[email protected]> > wrote: > >> Hi Richard, > >> > >> based on an earlier discussion on cfe-commits, here is an NFC patch > that introduces a ModuleProvider interface to handle the physical storage > of clang modules. The primary motivation for this is module debugging. The > idea is that CodeGen will implement this interface to wrap clang modules in > an object file container. Other users of libclang that do not care about > debug info can use the default implementation provided in this patch and > won’t need to link against CodeGen and LLVM. > >> > >> The majority of the patch consists of threading the new interface > through everywhere, the interesting bits are in AST/ModuleProvider.h and > Lex/ModuleLoader.h. There are no changes necessary to clang-tools-extra. > >> > >> -- adrian > >> > >> > > Thanks for all the feedback! > > >> > >> Introduce a ModuleProvider interface (NFC). > >> > >> A ModuleProvider provides an interface handling the physical storage of > >> Clang modules. The default implementation is SimpleModuleProvider, which > >> uses a flat file for the output. > > > > I don't agree that that's what this patch does -- I think your > ModuleProvider provides an interface abstracting a wrapper file format > containing a serialized AST bitstream. > > Right — the ModuleProvider isn’t really providing anything and Module is > also a very overloaded term. What it does is provide a set of operations > that handle the wrapping of a serialized AST inside a container format. > Inside clang the most common short name for serialized AST bitstreams > appears to be PCH (CPCH is also the magic number of the bitstream format). > > I’m proposing to rename the abstract interface to: > > class PCHContainerOperations { > /// Return an ASTConsumer that can be chained with a PCHGenerator > /// that produces a wrapper file format containing a serialized AST > bitstream. > virtual std::unique_ptr<ASTConsumer> CreatePCHContainerGenerator(...); > > /// Initialize an llvm::BitstreamReader with the serialized AST > /// wrapped inside a PCH container. > virtual void ExtractPCH(...); > } > > and the concrete implementations to > > class RawPCHContainerOperations : public PCHContainerOperations {...} > class ObjectFilePCHContainerOperations : public PCHContainerOperations > {...} > > > > > > I think that doesn't fit very well in ModuleLoader, which should support > providing a module by means not involving an AST serialized in our normal > bitcode format (for instance, converting it from DWARF, or loading some > standardized serialized AST format, or converting from another language, or > whatever). Can you store the ModuleProvider at a higher level (somewhere > where we know we're dealing with a bitstream file, such as the > CompilerInstance)? > > I moved the ownership away from ModuleLoader and up to the > CompilerInstance. > > > > > > >> The main application for this interface will be an implementation that > >> uses LLVM to wrap the module in an ELF/Mach-O/COFF container to store > >> debug info alongside the module. > > > > +++ b/include/clang/AST/ModuleProvider.h > > > > Does this really belong in libAST? That seems like a poor fit to me. > (Also, I'm not really happy with the name, since this thing does not > provide modules in any meaningful sense.) > > I moved it into Frontend since it’s mostly owned by CompilerInstance which > also lives there. > > > + /// \brief Return an ASTconsumer that can be chained with a > > > > Miscapitalization. > > > Fixed. > > > > + virtual std::unique_ptr<ASTConsumer> CreateModuleContainerGenerator( > > + DiagnosticsEngine &Diags, const std::string &ModuleName, > > + const HeaderSearchOptions &HSO, const PreprocessorOptions &PPO, > > + const CodeGenOptions &CGO, const TargetOptions &TO, const > LangOptions &LO, > > + llvm::raw_pwrite_stream *OS, > > + std::shared_ptr<ModuleBuffer> Buffer) const = 0; > > > > This interface seems very specific to generating debug information for a > module. Should the computation of these options structs (when they're > needed) live inside the ModuleProvider subclass rather than being provided > by the caller of this function? > > > > The only two call sites of this function are in FrontendActions and there > we need to pass through most of the option structs from the compiler > invocation so debug info can be emitted. However, the computation of > CodeGenOpts (with the exception of the output file name) can really be > moved into the subclass. I removed it from the interface. I also renamed > ModuleName to InputFileName as that is what is really passed in there. > > > > > +++ b/include/clang/Lex/ModuleLoader.h > > - /// \brief Attempt to load the given module. > > +/// \brief Attempt to load the given module. > > > > Broken indentation here. > > > > > > +/// \brief A shared pointer to a ModuleProvider that is non-null. > > +class SharedModuleProvider : public std::shared_ptr<ModuleProvider> { > > + SharedModuleProvider() = delete; > > + SharedModuleProvider(ModuleProvider *MP); > > +public: > > + template<class T, typename... ArgTypes> > > + static SharedModuleProvider Create(ArgTypes &&... Args) { > > + return SharedModuleProvider(new T(std::forward<ArgTypes>(Args)...)); > > + } > > +}; > > > > Is it really worth having a separate class for this? Your Create > function seems worse than using make_shared directly (it'll result in two > allocations rather than one). > > > > Using just a shared_ptr now. > > > > > +++ b/include/clang/Serialization/GlobalModuleIndex.h > > + /// \param MP The Moduleprovider to be used to unwrap the modules. > > > > Miscapitalization. > > > Fixed. > > > > +++ b/lib/Frontend/FrontendActions.cpp > > + // The debug info emitted by ModuleContainerGenerator is not affected > by the > > + // optimization level. > > > > ... so why bother explicitly setting it? The default constructor already > set it to 0. > > > I was able to eliminate CodeGenOptions entirely. > > > GeneratePCHAction::CreateASTConsumer(CompilerInstance &CI, StringRef > InFile) { > > ... > > + > Consumers.push_back(CI.getModuleProvider().CreateModuleContainerGenerator( > > > > Are you intending to put debug information into PCH files too, not just > into modules? > > > Yes. > > > > +++ b/lib/Frontend/MultiplexConsumer.cpp > > > > Please commit these changes separately; it's sufficient to mention in > your commit message that your subsequent change will add test coverage. > > > Done. > > > > +++ b/tools/c-index-test/c-index-test.c > > > > Please commit this typo fix separately. > > > Done. > > The new patch is slightly larger because I ran clang-format on the patch. > That's introduced some bad formatting (mismatching function and comment indentation in ARCMT.h and Tooling.h), can you fix that up? Otherwise, LGTM, thanks! > -- adrian > > Introduce a PCHContainerOperations interface (NFC). > > A PCHContainerOperations abstract interface provides operations for > creating and unwrapping containers for serialized ASTs (precompiled > headers and clang modules). The default implementation is > RawPCHContainerOperations, which uses a flat file for the output. > > The main application for this interface will be an > ObjectFilePCHContainerOperations implementation that uses LLVM to wrap the > module in an ELF/Mach-O/COFF container to store debuginfo alongside the > AST. > > > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
