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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
