On Sep 9, 2011, at 11:09 AM, Douglas Gregor wrote: > > On Sep 9, 2011, at 11:08 AM, Argyrios Kyrtzidis wrote: > >> On Sep 9, 2011, at 11:05 AM, Douglas Gregor wrote: >> >>> >>> On Sep 9, 2011, at 10:40 AM, Argyrios Kyrtzidis wrote: >>> >>>> On Sep 9, 2011, at 10:30 AM, Douglas Gregor wrote: >>>> >>>>> >>>>> On Sep 9, 2011, at 10:28 AM, Argyrios Kyrtzidis wrote: >>>>> >>>>>> On Sep 9, 2011, at 9:55 AM, Douglas Gregor wrote: >>>>>> >>>>>>> >>>>>>> On Sep 9, 2011, at 9:44 AM, Argyrios Kyrtzidis wrote: >>>>>>> >>>>>>>> >>>>>>>> On Sep 9, 2011, at 9:12 AM, Douglas Gregor wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> On Sep 9, 2011, at 9:05 AM, Argyrios Kyrtzidis wrote: >>>>>>>>> >>>>>>>>>> On Sep 9, 2011, at 8:15 AM, Douglas Gregor wrote: >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Sep 8, 2011, at 11:44 PM, Argyrios Kyrtzidis wrote: >>>>>>>>>>> >>>>>>>>>>>> Author: akirtzidis >>>>>>>>>>>> Date: Fri Sep 9 01:44:17 2011 >>>>>>>>>>>> New Revision: 139358 >>>>>>>>>>>> >>>>>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=139358&view=rev >>>>>>>>>>>> Log: >>>>>>>>>>>> [PCH] When loading the decls linked to an identifier, also make >>>>>>>>>>>> them visible >>>>>>>>>>>> in the translation unit. >>>>>>>>>>>> >>>>>>>>>>>> Modified: >>>>>>>>>>>> cfe/trunk/lib/Serialization/ASTReader.cpp >>>>>>>>>>>> >>>>>>>>>>>> Modified: cfe/trunk/lib/Serialization/ASTReader.cpp >>>>>>>>>>>> URL: >>>>>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=139358&r1=139357&r2=139358&view=diff >>>>>>>>>>>> ============================================================================== >>>>>>>>>>>> --- cfe/trunk/lib/Serialization/ASTReader.cpp (original) >>>>>>>>>>>> +++ cfe/trunk/lib/Serialization/ASTReader.cpp Fri Sep 9 01:44:17 >>>>>>>>>>>> 2011 >>>>>>>>>>>> @@ -4841,8 +4841,16 @@ >>>>>>>>>>>> return; >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> + ASTContext &Ctx = *getContext(); >>>>>>>>>>>> for (unsigned I = 0, N = DeclIDs.size(); I != N; ++I) { >>>>>>>>>>>> NamedDecl *D = cast<NamedDecl>(GetDecl(DeclIDs[I])); >>>>>>>>>>>> + >>>>>>>>>>>> + // In C++, translation unit's visible decls map gets emitted >>>>>>>>>>>> in the AST >>>>>>>>>>>> + // file, but not on C; make the decl visible so it can be >>>>>>>>>>>> looked up. >>>>>>>>>>>> + if (!Ctx.getLangOptions().CPlusPlus) >>>>>>>>>>>> + SetExternalVisibleDeclsForName(Ctx.getTranslationUnitDecl(), >>>>>>>>>>>> + DeclarationName(II), D); >>>>>>>>>>>> + >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> This is a bit unfortunate. Creating the lookup table in the >>>>>>>>>>> translation unit forces the AST to continually maintain that lookup >>>>>>>>>>> table (e.g., adding entries to it every time a visible declaration >>>>>>>>>>> is added to the translation unit), which is a cost we'd like to >>>>>>>>>>> avoid in C. I presume that you only made this change to support the >>>>>>>>>>> new lookup used in r139359? I have an alternative suggestion for >>>>>>>>>>> that... >>>>>>>>>> >>>>>>>>>> But adding entries always updated the lookup table, or the lookup in >>>>>>>>>> r139359 would not work in non-PCH mode, right ? >>>>>>>>>> In general I'd find it bad for the AST usefulness if you could not >>>>>>>>>> do a lookup in translation unit unless it is on C++. >>>>>>>>> >>>>>>>>> >>>>>>>>> It's more subtle than that. The lookup table for a DeclContext is >>>>>>>>> constructed lazily, when one performs the first lookup, by walking >>>>>>>>> the declarations lexically in that context and populating the table. >>>>>>>>> Once a table is there, it is maintained as declarations are added to >>>>>>>>> the DeclContext. >>>>>>>> >>>>>>>> I see the error of my ways now. Note however that, outside of this >>>>>>>> fix, it's very unfortunate that lookup does not work on a loaded C AST. >>>>>>> >>>>>>> >>>>>>> I think it should work, but it's super-inefficient because it will >>>>>>> deserialize the whole translation unit. >>>>>> >>>>>> Once a declaration is loaded through the identifier it can be found in >>>>>> lexical decls linked list right ? It's only of matter then to figure out >>>>>> that if no result is returned from looking up at the external ast >>>>>> source, it doesn't mean that no decl with that name exists, but that the >>>>>> source didn't store a map; in which case we recreate the map with only >>>>>> the lexical decls that were already loaded (and subsequent >>>>>> loaded-through-identifier decls will get added to the existing map) >>>>> >>>>> >>>>> Ahhh, you are correct. Which means this is already broken :( >>>>> >>>>> Perhaps we should just take the performance hit when building an AST file >>>>> for a C translation unit, generating the lookup table for the translation >>>>> unit as AST writing time so we can serialize it. >>>> >>>> Rough estimate: ObjC PCH for Cocoa.h is -20% compared to the ObjC++ PCH, >>>> is this acceptable hit ? >>> >>> >>> ObjC++ includes a bunch of extra stuff, so that will over-estimate the >>> cost… I went ahead and measured the actual cost of doing this. It's an 8% >>> increase in PCH size, which I think is unacceptable. >>> >>> Looking back at DeclContext::buildLookup, I don't actually see why it would >>> be broken. It walks the lexical context using decls_begin()/decls_end(), >>> which should load all of the lexical declarations on-demand from the PCH >>> file. Something is fishy here. >> >> buildLookup is not getting called if there is an external ast source >> present, looking up is delegated to the source; see DeclContext::lookup and >> the "if (hasExternalVisibleStorage()) {" check. > > … but there is no external visible storage for the TU in C. If there were, we > wouldn't be worrying about this.
But it "does"; see unconditional: // Note that the translation unit has external lexical and visible storage. TU->setHasExternalLexicalStorage(true); TU->setHasExternalVisibleStorage(true); in ASTReader::InitializeContext > > - Doug _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
