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

[Richard Smith]

Thanks! LGTM

+           "Supported options are `raw' and `obj'.">;

We don't typically use backticks for quotation.

On Thu, Jul 16, 2015 at 4:41 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> On Jul 15, 2015, at 2:02 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> 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)?
>
>
> This updated patch
> - introduces -fmodule-format=[format]
> - defaults to raw containers
> - supports arbitrary module container formats that libclang is agnostic to
> - adds the format to the module hash
> - splits the old PCHContainerOperations into PCHContainerWriter and
> PCHContainerReader.
> The module format string is now part of HeaderSearchOptions instead of
> CodeGenOptions.
>
> -- adrian
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150716/ea8b65a4/attachment.html>