<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 Jan 27, 2015, at 5:12 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" class=""><div class="gmail_extra"><div class="gmail_quote">On Tue, Jan 27, 2015 at 9:29 AM, Adrian Prantl <span dir="ltr" class=""><<a href="mailto:aprantl@apple.com" target="_blank" class="">aprantl@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br class="">
> On Jan 26, 2015, at 5:34 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" class="">richard@metafoo.co.uk</a>> wrote:<br class="">
><br class="">
> On Mon, Jan 26, 2015 at 4:05 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" class="">aprantl@apple.com</a>> wrote:<br class="">>> Here is the final version of the patch that should address all the points<br class="">
>> raised so far in this thread. Any final comments/objections/suggestions<br class="">
>> before this can go into trunk?<br class=""></div></div></blockquote></div></div></div></div></blockquote><div><br class=""></div><div>Hi all,</div><div><br class=""></div><div>Here’s the upgraded version of the patch, which addresses all of your suggestions. The second patch enables debug info output, so you can see the first one in context and know where this is heading.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
><br class="">
> Using a fixed string literal as a key for the PCH data seems a bit<br class="">
> weird to me. I'm also not sure a module flag is the best way to model<br class="">
> this: can we instead emit this as a normal global, and keep the<br class="">
> knowledge of its special section name within Clang?<br class="">
</div></div>That sounds like a good idea — I wasn’t aware that globals could be annotated with a section name.<br class="">
I’ll give it a shot.<br class=""></blockquote></div></div></div></div></blockquote><div><br class=""></div><div>This works really great for ELF and Mach-O; using a global symbol with a custom section allowed me to get rid of the LLVM patch entirely! The big drawback is that I can no longer influence the order in which the sections are emitted, which turns out to be a problem for COFF.</div><div><div>Although COFF sections are marked with the correct alignment characteristics, they are not actually emitted aligned on disk. This is hopefully jus a bug in our COFF backend. Since the DWARF sections are of arbitrary size and come before the serialized AST, this messes up the alignment for OnDiskHashTable. Until I figure out a solution for this problem, I’ve disabled module debug info ouput for COFF.</div></div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="">><br class="">
> I would imagine that you'll want to generate debug information via<br class="">
> Clang's normal ASTConsumer -> CodeGen pipeline, which suggests that<br class="">
> building a custom LLVM code generator and running it from within<br class="">
> PCHGenerator::HandleTranslationUnit is the wrong choice. This should<br class="">
> probably be handled by a FrontendAction that passes the bitcode<br class="">
> generated by the PCHGenerator to CodeGen. That'd also avoid you<br class="">
> needing to plumb a CompilerInstance into Serialization, which seems<br class="">
> like a layering violation (Frontend depends on Serialization, not vice<br class="">
> versa).<br class="">
<br class="">
</span>Thanks for the pointer. In this case both the code generator and the output buffer for the serialized AST is owned by a FrontendAction which then wraps it in a global and passes it on to CodeGen.<br class="">
In order to wire up the debug information, since I can only have one ASTConsumer per FrontendAction, I will probably need to have a second FrontendAction for the debug info generator?<br class=""></blockquote><div class=""><br class=""></div><div class="">If you need it, it shouldn't be hard to write an ASTConsumer implementation that forwards calls to multiple other consumers. You'll probably need to ensure that PCHGenerator is first in the sequence to ensure that it can emit the PCH data before we finalize the IR.</div></div></div></div></div></blockquote><div><br class=""></div><div>I found ASTMultiplexConsumer, which does exactly what I needed (after I added all the missing forwarding methods).</div><div><br class=""></div><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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I’ll check out how clang works and how to communicate between FrontendActions.<br class="">
I might be back with more questions :-)<br class="">
<span class=""><br class="">
><br class="">
> For the section name (repeating a comment from before): I don't like<br class="">
> "cfe"; we should indicate which compiler is involved here. I don't<br class="">
> like "pch"; this is used for all kinds of AST files, not just PCH<br class="">
> files. Can you pick a better name for the section?<br class="">
<br class="">
</span>That particular name was a suggestion by David Majnemer who pointed out that COFF section names are restricted to 8 characters. I chose PCH because the magic number in .pcm files is CPCH.<br class="">
The full list of constraints is:<br class="">
- 8 characters (COFF)<br class="">
- starts with __ (MachO)<br class=""></blockquote><div class=""><br class=""></div><div class="">How about clangast on COFF and ELF, and __clangast on MachO? Or do we need to keep the name the same in all cases?</div></div></div></div></div></blockquote><div><br class=""></div>Since the name is only used in two locations, I think having a platform-specific one isn’t too bad. I’ve implemented your suggestion.</div><div><br class=""></div><div>thanks for all the feedback so far!</div><div>-- adrian</div><div></div></body></html>