<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Apr 9, 2009, at 10:52 PM, Chris Lattner wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div><blockquote type="cite">URL: <a href="http://llvm.org/viewvc/llvm-project?rev=68732&view=rev">http://llvm.org/viewvc/llvm-project?rev=68732&view=rev</a><br></blockquote><blockquote type="cite">Log:<br></blockquote><blockquote type="cite">Implementation of pre-compiled headers (PCH) based on lazy<br></blockquote><blockquote type="cite">de-serialization of abstract syntax trees.<br></blockquote><br>Awesome Doug! Thank you for splitting out the astcontext-everwhere part of the patch, it definitely makes it easier to review.<br><br><blockquote type="cite">+++ cfe/trunk/include/clang/AST/DeclContextInternals.h Thu Apr 9 17:27:44 2009<br></blockquote><blockquote type="cite"> /// VectorTy - When in vector form, this is what the Data pointer points to.<br></blockquote><blockquote type="cite">+ typedef llvm::SmallVector<uintptr_t, 4> VectorTy;<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ /// \brief The stored data, which will be either a declaration ID, a<br></blockquote><blockquote type="cite">+ /// pointer to a NamedDecl, or a pointer to a vector.<br></blockquote><blockquote type="cite">+ uintptr_t Data;<br></blockquote><br>Why the custom bitmangling?<br></div></blockquote><div><br></div><div>It's an unsigned, a NamedDecl*, or a VectorTy*... IIRC, there's no variant type for that, and of course the VectorTy* can be interpreted in two different ways (vector-of-IDs or vector-of-NamedDecl*s).</div><br><blockquote type="cite"><div><blockquote type="cite">+ void setFromDeclIDs(const llvm::SmallVectorImpl<unsigned> &Vec) {<br></blockquote><blockquote type="cite">+ if (Vec.size() > 1) {<br></blockquote><blockquote type="cite">+ VectorTy *Vector = getAsVector();<br></blockquote><blockquote type="cite">+ if (!Vector) {<br></blockquote><blockquote type="cite">+ Vector = new VectorTy;<br></blockquote><blockquote type="cite">+ Data = reinterpret_cast<uintptr_t>(Vector) | DK_Decl_Vector;<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ Vector->resize(Vec.size());<br></blockquote><blockquote type="cite">+ std::copy(Vec.begin(), Vec.end(), Vector->begin());<br></blockquote><blockquote type="cite">+ return;<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ if (VectorTy *Vector = getAsVector())<br></blockquote><blockquote type="cite">+ delete Vector;<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ if (Vec.size() == 0)<br></blockquote><br>Vec.empty() ?<br></div></blockquote><div><br></div>Oh, yeah. Duh.</div><div><br><blockquote type="cite"><div><blockquote type="cite">+++ cfe/trunk/include/clang/Frontend/PCHBitCodes.h Thu Apr 9 17:27:44 2009<br></blockquote><blockquote type="cite">@@ -0,0 +1,239 @@<br></blockquote><br><blockquote type="cite"><br></blockquote><blockquote type="cite">+#ifndef LLVM_CLANG_FRONTEND_PCHBITCODES_H<br></blockquote><blockquote type="cite">+#define LLVM_CLANG_FRONTEND_PCHBITCODES_H<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+#include "llvm/Bitcode/BitCodes.h"<br></blockquote><blockquote type="cite">+#include "llvm/Support/DataTypes.h"<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+namespace clang {<br></blockquote><blockquote type="cite">+ namespace pch {<br></blockquote><blockquote type="cite">+ const int IDBits = 32;<br></blockquote><br>IDBits is dead?<br></div></blockquote><div><br></div><div>Yup, thanks.</div><div><br></div><br><blockquote type="cite"><div><blockquote type="cite">+++ cfe/trunk/include/clang/Frontend/PCHWriter.h Thu Apr 9 17:27:44 2009<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">+ /// The ID numbers of declarations are consecutive (in order of<br></blockquote><blockquote type="cite">+ /// discovery) and start at 2. 1 is reserved for the translation<br></blockquote><blockquote type="cite">+ /// unit, while 0 is reserved for NULL.<br></blockquote><br>Please repeat or move this comment to the definition of the pch::ID typedef.<br><br><blockquote type="cite">+ /// \brief Map that provides the ID numbers of each type within the<br></blockquote><blockquote type="cite">+ /// output stream.<br></blockquote><blockquote type="cite">+ ///<br></blockquote><blockquote type="cite">+ /// The ID numbers of types are consecutive (in order of discovery)<br></blockquote><blockquote type="cite">+ /// and start at 1. 0 is reserved for NULL. When types are actually<br></blockquote><blockquote type="cite">+ /// stored in the stream, the ID number is shifted by 3 bits to<br></blockquote><blockquote type="cite">+ /// allow for the const/volatile/restrict qualifiers.<br></blockquote><br>Okay, maybe not. Would it make sense to have two different typedefs for TypeID vs DeclID? If that happened, you could have an appropriate comment for each.<br></div></blockquote><div><br></div><div>This is better, thanks!</div><br><blockquote type="cite"><div><blockquote type="cite">+++ cfe/trunk/lib/Frontend/PCHReader.cpp Thu Apr 9 17:27:44 2009<br></blockquote><br><blockquote type="cite">+void PCHDeclReader::VisitVarDecl(VarDecl *VD) {<br></blockquote><blockquote type="cite">+ VisitValueDecl(VD);<br></blockquote><blockquote type="cite">+ VD->setStorageClass((VarDecl::StorageClass)Record[Idx++]);<br></blockquote><blockquote type="cite">+ VD->setThreadSpecified(Record[Idx++]);<br></blockquote><blockquote type="cite">+ VD->setCXXDirectInitializer(Record[Idx++]);<br></blockquote><blockquote type="cite">+ VD->setDeclaredInCondition(Record[Idx++]);<br></blockquote><br>This is a very cute idiom. One potential problem to think about is that a malformed pch file could crash the reader by having a record with too-few fields: this would cause invalid indexes of Record to be used.<br></div></blockquote><div><br></div><div>Good point. As part of turning the lame Error()s into real diagnostics, I'll pre-test the record size and give a FATAL_ERROR if there is a problem.</div><div><br></div><blockquote type="cite"><div><blockquote type="cite"><font class="Apple-style-span" color="#000000"><br></font></blockquote><blockquote type="cite">+/// \brief Read the type-offsets block.<br></blockquote><blockquote type="cite">+bool PCHReader::ReadTypeOffsets() {<br></blockquote><br>Would it make sense to just make TYPE_OFFSETS be a record in the top-level block instead of a block that only contains one record type? Likewise with DECL_OFFSETS. That would get rid of the boilerplate to read a block.<br></div></blockquote><div><br></div><div>Yep, that makes sense. </div><br><blockquote type="cite"><div><blockquote type="cite">+/// \brief Read and return the type at the given offset.<br></blockquote><blockquote type="cite">+///<br></blockquote><blockquote type="cite">+/// This routine actually reads the record corresponding to the type<br></blockquote><blockquote type="cite">+/// at the given offset in the bitstream. It is a helper routine for<br></blockquote><blockquote type="cite">+/// GetType, which deals with reading type IDs.<br></blockquote><blockquote type="cite">+QualType PCHReader::ReadTypeRecord(uint64_t Offset) {<br></blockquote><blockquote type="cite">+ Stream.JumpToBit(Offset);<br></blockquote><blockquote type="cite">+ RecordData Record;<br></blockquote><blockquote type="cite">+ unsigned Code = Stream.ReadCode();<br></blockquote><blockquote type="cite">+ switch ((pch::TypeCode)Stream.ReadRecord(Code, Record)) {<br></blockquote><blockquote type="cite">+ case pch::TYPE_FIXED_WIDTH_INT: {<br></blockquote><blockquote type="cite">+ assert(Record.size() == 2 && "Incorrect encoding of fixed-width int type");<br></blockquote><br>Instead of asserts, should these emit an error somehow and return something bogus? Having an invalid PCH file crash the compiler is acceptable, but suboptimal.<br></div></blockquote><div><br></div><div>Yeah, these should be FATAL_ERRORS some some recovery strategy.</div><br><blockquote type="cite"><div><blockquote type="cite">+ // FIXME: Several other kinds of types to deserialize here!<br></blockquote><blockquote type="cite">+ default:<br></blockquote><blockquote type="cite">+ assert("Unable to deserialize this type");<br></blockquote><br>0 && "unable...<br></div></blockquote><div><br></div>Oops, thanks.</div><div><br><blockquote type="cite"><div><blockquote type="cite">+void PCHDeclWriter::VisitDecl(Decl *D) {<br></blockquote><blockquote type="cite">+ Writer.AddDeclRef(cast_or_null<Decl>(D->getDeclContext()), Record);<br></blockquote><blockquote type="cite">+ Writer.AddDeclRef(cast_or_null<Decl>(D->getLexicalDeclContext()), Record);<br></blockquote><blockquote type="cite">+ Writer.AddSourceLocation(D->getLocation(), Record);<br></blockquote><blockquote type="cite">+ Record.push_back(D->isInvalidDecl());<br></blockquote><blockquote type="cite">+ // FIXME: hasAttrs<br></blockquote><br>Interesting question: would it be better to unique attribute lists and output an attr ID # with each decl, or would it be better to emit a "has attr" bit and then emit a separate declid->attrlist datastructure independent of this?<br></div></blockquote><div><br></div><div>I think I'd like to output a "has attr" bit, and then have a (not-yet-implemented) DECL_ATTRIBUTE record directly after the decl record. So, when the reader sees that the decl has an attribute, it builds the decl and then reads the next record to get its attributes.</div><div><br></div><div>I don't know if it's worth uniquing attributes or not.</div><div><br></div><blockquote type="cite"><div><blockquote type="cite">+/// \brief Write a block containing all of the types.<br></blockquote><blockquote type="cite">+void PCHWriter::WriteTypesBlock(ASTContext &Context) {<br></blockquote><blockquote type="cite">+ // Enter the types block<br></blockquote><blockquote type="cite">+ S.EnterSubblock(pch::TYPES_BLOCK_ID, 2);<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ // Emit all of the types in the ASTContext<br></blockquote><blockquote type="cite">+ for (std::vector<Type*>::const_iterator T = Context.getTypes().begin(),<br></blockquote><blockquote type="cite">+ TEnd = Context.getTypes().end();<br></blockquote><blockquote type="cite">+ T != TEnd; ++T) {<br></blockquote><blockquote type="cite">+ // Builtin types are never serialized.<br></blockquote><blockquote type="cite">+ if (isa<BuiltinType>(*T))<br></blockquote><blockquote type="cite">+ continue;<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ WriteType(*T);<br></blockquote><blockquote type="cite">+ }<br></blockquote><br>Why not just emit all types referred to by decls? Is there a difference?<br></div></blockquote><div><br></div><div>We need to start somewhere, and once we've emitted the block for decls or types, we can't go back and add to that block when the processing of the other block uncovers a new decl/type. There are circular dependencies here: types store decls and decls store types.</div><div><br></div><div>By iterating over the list of types, and writing the types block first, we know that we've handled *all* of the types. I guess we should assert that the number of types hasn't changed once we're done writing the PCH file... (lazily de-serializing one PCH file to write another? hrm)</div><div><br></div><div>Now, we could just iterate until we're done, emitting multiple decl and type blocks as needed. It would be slightly unfortunate to have to do so, because it could make it a little tougher to cope with abbreviations when reading the PCH file. However, we might end up writing fewer types (especially in C++ with template instantiation, where lots of types will be built and then thrown away).</div><br><blockquote type="cite"><div><blockquote type="cite">+/// \brief Write the block containing all of the declaration IDs<br></blockquote><blockquote type="cite">+/// lexically declared within the given DeclContext.<br></blockquote><blockquote type="cite">+///<br></blockquote><blockquote type="cite">+/// \returns the offset of the DECL_CONTEXT_LEXICAL block within the<br></blockquote><blockquote type="cite">+/// bistream, or 0 if no block was written.<br></blockquote><blockquote type="cite">+uint64_t PCHWriter::WriteDeclContextLexicalBlock(ASTContext &Context,<br></blockquote><blockquote type="cite">+ DeclContext *DC) {<br></blockquote><blockquote type="cite">+ if (DC->decls_begin(Context) == DC->decls_end(Context))<br></blockquote><blockquote type="cite">+ return 0;<br></blockquote><br>How about a new DC->decls_empty(Context) method?<br></div></blockquote><div><br></div>Sure.</div><div><br><blockquote type="cite"><div><blockquote type="cite">+++ cfe/trunk/lib/Sema/SemaDecl.cpp Thu Apr 9 17:27:44 2009<br></blockquote><blockquote type="cite">@@ -524,6 +524,12 @@<br></blockquote><blockquote type="cite"> return false;<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">+ // __builtin_va_list gets redeclared in the built-in definitions<br></blockquote><blockquote type="cite">+ // buffer when using PCH. Don't complain about such redefinitions.<br></blockquote><blockquote type="cite">+ if (Context.getExternalSource() &&<br></blockquote><blockquote type="cite">+ strcmp(SourceMgr.getBufferName(New->getLocation()), "<built-in>") == 0)<br></blockquote><blockquote type="cite">+ return false;<br></blockquote><br>Eww. We should really just fix sema to not do this. Can you add a verbose fixme explaining the issue and the fix we discussed?<br></div></blockquote><div><br></div><div>Done.</div><br><blockquote type="cite"><div>Overall, this is great Doug!<br></div></blockquote><div><br></div><div>Thanks for the review!</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug<br></div></div></body></html>