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

Thu, 16 Jul 2015 16:44:08 -0700


On Jul 15, 2015, at 2:02 PM, Richard Smith <[email protected]> wrote:

On Wed, Jul 15, 2015 at 1:51 PM, Adrian Prantl <[email protected]> wrote:

On Jul 15, 2015, at 1:16 PM, Richard Smith <[email protected]> wrote:

On Wed, Jul 15, 2015 at 12:54 PM, Adrian Prantl <[email protected]> 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

Attachment: fmodule-format2.diff
Description: Binary data

_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to