[PATCH] Introduce a ModuleProvider interface (NFC)

[Richard Smith]

On Fri, Jun 19, 2015 at 3:15 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> > On Jun 16, 2015, at 2:26 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> >
> > On Tue, Jun 16, 2015 at 10:15 AM, Adrian Prantl <aprantl at apple.com>
> 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.
>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150619/e7d6d80a/attachment.html>