[PATCH] Introduce a ModuleProvider interface (NFC)

[Adrian Prantl]

> 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.

-- 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 --------------
A non-text attachment was scrubbed...
Name: 0001-Introduce-a-PCHContainerOperations-interface-NFC.patch
Type: application/octet-stream
Size: 116891 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150619/5defab49/attachment.obj>
-------------- next part --------------