r241620 - Wrap clang modules and pch files in an object file container.

[Richard Smith]

On Wed, Jul 15, 2015 at 1:51 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> On Jul 15, 2015, at 1:16 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> On Wed, Jul 15, 2015 at 12:54 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
>> Here is a patch that implements a -fmodule-format=[obj,raw] option. I
>> have not yet implemented adding the module format to the module hash or
>> filename.
>>
>
> Generally, we only have a -cc1 flag to switch to the non-default state.
>
>
> Okay I think it’s better then to bake the default into cc1 rather than the
> driver.
>
>
>
>> The default setting is obj (based on the assumption that this is most
>> beneficial to platforms with a shared module cache) and the driver adds an
>> explicit -fmodule-format=raw on Linux.
>>
>
> I think this is backwards; I think putting more stuff into the precompiled
> module format should be opt-in rather than opt-out.
>
> I am not sure whether having the driver emit this option for Linux is a
>> good idea: Are explicit module builds a feature of the Google build system,
>> or are they a Linux platform feature?
>>
>
> Neither; they're a Clang feature. Implicit module builds are a
> compatibility feature for legacy build systems, and should be avoided
> wherever possible because they introduce a performance hit, interact poorly
> with distributed builds, behave badly if the cache gets cleared (especially
> once we put debug info in modules), and so on.
>
> Currently the only way to enable obj-wrapped modules on Linux is to pass a
>> cc1 option. Even with -fmodule-format=raw specified, clang can still read
>> obj-wrapped modules.
>>
>
> OK, but presumably we'll add -gmodules or similar at some point to resolve
> that issue.
>
>
> Yes.
>
> My updated plan is to make =raw the default. The driver expands
> “-gmodules" into something like "-dwarf-ext-typerefs[TBD]
> -fmodule-format=obj”. Having =raw be the default is safe as long as we
> encode the module format into the module hash so the raw and obj versions
> don’t conflict. Once we have a pass to translate module ast->obj as you
> would do for explicit builds, we can make more fine-grained adjustments to
> this.
>
>
> I’m not in love with the actual implementation, so suggestions and
>> feedback are very welcome!
>>
>
> I assume the "if (1 || ..." was not intentional?
>
>
> Yes :-) I had that in there to run the testsuite with the different
> default settings.
>
>
> It doesn't seem ideal to have the top-level driver create the
> wrapper-format handler, and then ignore that from the frontend code. That's
> also not going to scale to module formats other than obj and raw. Is there
> any other way we can deal with this without breaking the layering?
>
>
> That’s exactly what bothered me. The wrapper-format handler is primarily
> there so Frontend does not need to depend on CodeGen. Currently we’re
> passing it in to CompilerInstance at creation time (which happens at the
> very top level). It might be possible to solve this a little more elegantly
> by having a
>
> class PCHContainerOperationsRegistry {
>   void registerPCHContainerOperations(PCHContainerOperations *Handler);
>   shared_ptr<PCHContainerOperations> getPCHContainerOperations(PCHFormat
> Format);
> }
>
> have the toplevel register RawPCHContainerOperations and
> ObjectFilePCHContainerOperations and have
> CompilerInstance::getPCHContainerOperations() return the appropriate
> handler for the currently registered CodeGenOptions (since the caller
> usually doesn’t have access to the CodeGenOptions).
>

Yes, something along those lines would make sense to me. I was thinking of
a slightly different design, more like

  class ASTContainerFormatHandler {
    PCHContainerOperations *getFormat(StringRef Format) = 0;
  };

... but I'll leave the details up to you. Maybe this would also be a good
time to think about the writer / reader split (for consumers that want to
read object-file-wrapped ASTs but don't want to link against the LLVM code
generator)?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150715/ffe147f6/attachment.html>