[cfe-commits] r139358 - /cfe/trunk/lib/Serialization/ASTReader.cpp

[Douglas Gregor]

On Sep 9, 2011, at 11:13 AM, Argyrios Kyrtzidis wrote:

> 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

Oh, my.  We shouldn't be setting those unless we see a record that actually indicates that we have lexical or visible storage :(

	- Doug