<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jul 15, 2015, at 2:02 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" class="">richard@metafoo.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: 14px; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 15, 2015 at 1:51 PM, Adrian Prantl<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:aprantl@apple.com" target="_blank" class="">aprantl@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Jul 15, 2015, at 1:16 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank" class="">richard@metafoo.co.uk</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 15, 2015 at 12:54 PM, Adrian Prantl<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:aprantl@apple.com" target="_blank" class="">aprantl@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">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.</div></div></blockquote><div class=""><br class=""></div><div class="">Generally, we only have a -cc1 flag to switch to the non-default state.</div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">Okay I think it’s better then to bake the default into cc1 rather than the driver.</div><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">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.</div></div></blockquote><div class=""><br class=""></div><div class="">I think this is backwards; I think putting more stuff into the precompiled module format should be opt-in rather than opt-out.</div><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">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?</div></div></blockquote><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">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. </div></div></blockquote><div class=""><br class=""></div><div class="">OK, but presumably we'll add -gmodules or similar at some point to resolve that issue.</div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">Yes.</div><div class=""><br class=""></div><div class="">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.</div><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class="">I’m not in love with the actual implementation, so suggestions and feedback are very welcome!</div></div></blockquote><div class=""><br class=""></div><div class="">I assume the "if (1 || ..." was not intentional?</div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">Yes :-) I had that in there to run the testsuite with the different default settings.</div><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><div class="">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?</div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">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</div><div class=""><br class=""></div><div class="">class PCHContainerOperationsRegistry {</div><div class=""> void registerPCHContainerOperations(PCHContainerOperations *Handler);</div></div> shared_ptr<PCHContainerOperations> getPCHContainerOperations(PCHFormat Format);<div class="">}</div><div class=""><br class=""></div><div class="">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).</div></div></blockquote><div class=""><br class=""></div><div class="">Yes, something along those lines would make sense to me. I was thinking of a slightly different design, more like</div><div class=""><br class=""></div><div class=""> class ASTContainerFormatHandler {</div><div class=""> PCHContainerOperations *getFormat(StringRef Format) = 0;</div><div class=""> <span class="Apple-converted-space"> </span>};</div><div class=""><br class=""></div><div class="">... 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)?</div></div></div></div></div></blockquote><br class=""></div><div>This updated patch</div><div><div>- introduces -fmodule-format=[format]</div><div>- defaults to raw containers</div><div class="">- supports arbitrary module container formats that libclang is agnostic to</div><div class="">- adds the format to the module hash</div><div class="">- splits the old PCHContainerOperations into PCHContainerWriter and PCHContainerReader.</div><div class="">The module format string is now part of HeaderSearchOptions instead of CodeGenOptions.</div><div class=""><br class=""></div><div class="">-- adrian</div><div class=""><br class=""></div></div></body></html>