r249157 - Module debugging: Don't emit forward declarations in module scopes.
David Blaikie via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 5 11:39:01 PDT 2015
On Mon, Oct 5, 2015 at 11:37 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Mon, Oct 5, 2015 at 8:55 AM, Adrian Prantl <aprantl at apple.com> wrote:
>
>>
>> On Oct 2, 2015, at 3:24 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Fri, Oct 2, 2015 at 3:21 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>
>>>
>>> On Oct 2, 2015, at 3:01 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>
>>>
>>> On Fri, Oct 2, 2015 at 3:00 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>>
>>>>
>>>> On Oct 2, 2015, at 2:58 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On Fri, Oct 2, 2015 at 2:40 PM, Adrian Prantl <aprantl at apple.com>
>>>> wrote:
>>>>
>>>>>
>>>>> On Oct 2, 2015, at 2:18 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>
>>>>> This seems a little curious, so you'll have code like this:
>>>>>
>>>>> decl foo
>>>>> module
>>>>> def bar
>>>>> member foo pointer (references the declaration of foo outside
>>>>> the module, in the CU?)
>>>>>
>>>>>
>>>>> Right.
>>>>>
>>>>> Why is that preferable to DWARF that looks more like the AST where the
>>>>> declaration of foo appears in the first module that references it, rather
>>>>> than, in a curiously circular situation, in the CU that references the
>>>>> module?
>>>>>
>>>>>
>>>>> The problem I had with this was that a forward declaration would end
>>>>> up in a DeclContext that is not the DeclContext of the definition. Looking
>>>>> at this through the dsymutil goggles, the different DeclContexts
>>>>> effectively prevent the forward declaration from being uniqued with the
>>>>> type's definition.
>>>>>
>>>>
>>>> Putting it in the CU doesn't seem to make the decl context match
>>>> either, does it? In that case the decl context may still be "some other
>>>> compile unit" (as with most declarations)
>>>>
>>>> Shouldn't dsymutil be treating modules more like CUs as "another top
>>>> level scope”?
>>>>
>>>>
>>>> The CU is not part of the DeclContext (for dsymutil’s interpretation of
>>>> a DeclContext). Otherwise cross-CU type uniquing would never work.
>>>>
>>>
>>> perhaps the module shouldn't be either, then?
>>>
>>>
>>> The module has to be, for correctness. (None of this applies to C++.
>>> Oversimplified: because of the ODR we can omit the parent DW_TAG_module
>>> from all CXXRecordDecls).
>>> For C and ObjC it is perfectly legal to have two modules with
>>> conflicting definitions for a type, so we need the DW_TAG_module as part of
>>> the DeclContext to differentiate them if we want to be able to resolve
>>> forward declarations to the correct definition.
>>>
>>
>> But you have to do the same thing for the things in the CU as well,
>> right? So you can't ignore the CU in the same way you can't ignore the
>> module, yes?
>>
>>
>> You’ve got a point there. In a non-ODR language, you still wouldn’t be
>> allowed to resolve a forward declaration to a specific type.
>> The one thing that I don’t like about emitting “real" forward
>> declarations into the DW_TAG_module is that they are indistinguishable from
>> the forward declarations that are emitted for external type references, for
>> which there is an actual definition in the module itself. This is more of
>> an aesthetic problem
>>
>
> Yeah, I agree it's a bit weird & not sure what the nicest solution is, but
> I think this is a bit more consistent with the way the modules actually
> work (that the type appears in the first module it's used from, even if
> it's just a declaration, then other modules do actually reference the type
> from the prior one)
>
> Remind me why we need these other types in the non-defining module anyway?
> Since it only contains type declarations, when does it need to contain
> types that aren't defined in the module? It just happens to be that its a
> type declaration that comes from the module, I guess? But the type will
> never be referenced from anything inside the non-defining DW_TAG_module?
>
(to clarify further - if nothing in the non-defining DW_TAG_module
references the type declaration, and the type definition won't be found in
the module, yeah - I think aesthetically, it makes sense to put the type
declaration in the CU - it's just the way the code to handle that falls out
that feels weird, I think)
>
> (& I know this is revisiting decisions we've made - but again, I wonder if
> this would all fall out more naturally if modularized types were treated
> more traditionally, but with an extra attribute in their declaration if
> they happen to be defined in a module we know about at the moment - so the
> code would look more like -flimit-debug-info just "if this definition is
> from a module, just emit a declaration" (& the "if we're building a
> declaration for a type defined in a module and we're targetting lldb, add
> the extra attribute) - it does seem a little (but not massively so) more
> complicated to be having to deal with these different context chains)
>
>
>> than a technical one, but it does make dsymutils job of resolving type
>> references in a single pass a little harder since dsymutil can no longer
>> assume that all forward declarations in a non-defining TAG_module can be
>> resolved.
>>
>
> Wouldn't dsymutil have a pretty generic algorithm for resolving
> declarations to definitions that would have to cope with missing
> definitions for declarations in CUs? So I would imagine this case would
> just "fall out" of that resolution code without need for any special cases.
> (so long as you visit modules in reverse order of dependencies - so you
> visit the dependencies before the modules they depend on (or the other way
> around, if that works best)).
>
>
>>
>> -- adrian
>>
>>
>>
>>>
>>> -- adrian
>>>
>>>
>>>
>>>>
>>>> -- adrian
>>>>
>>>>
>>>>
>>>>>
>>>>> -- adrian
>>>>>
>>>>> On Fri, Oct 2, 2015 at 10:36 AM, Adrian Prantl via cfe-commits <
>>>>> cfe-commits at lists.llvm.org> wrote:
>>>>>
>>>>>> Author: adrian
>>>>>> Date: Fri Oct 2 12:36:14 2015
>>>>>> New Revision: 249157
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=249157&view=rev
>>>>>> Log:
>>>>>> Module debugging: Don't emit forward declarations in module scopes.
>>>>>> A forward declaration inside a module header does not belong to the
>>>>>> module.
>>>>>>
>>>>>> Modified:
>>>>>> cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
>>>>>> cfe/trunk/test/Modules/Inputs/DebugObjC.h
>>>>>> cfe/trunk/test/Modules/ModuleDebugInfo.m
>>>>>>
>>>>>> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=249157&r1=249156&r2=249157&view=diff
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
>>>>>> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Fri Oct 2 12:36:14 2015
>>>>>> @@ -2172,6 +2172,9 @@ ObjCInterfaceDecl *CGDebugInfo::getObjCI
>>>>>> }
>>>>>>
>>>>>> llvm::DIModule *CGDebugInfo::getParentModuleOrNull(const Decl *D) {
>>>>>> + // A forward declaration inside a module header does not belong to
>>>>>> the module.
>>>>>> + if (isa<RecordDecl>(D) && !cast<RecordDecl>(D)->getDefinition())
>>>>>> + return nullptr;
>>>>>> if (DebugTypeExtRefs && D->isFromASTFile()) {
>>>>>> // Record a reference to an imported clang module or precompiled
>>>>>> header.
>>>>>> auto *Reader = CGM.getContext().getExternalSource();
>>>>>>
>>>>>> Modified: cfe/trunk/test/Modules/Inputs/DebugObjC.h
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/DebugObjC.h?rev=249157&r1=249156&r2=249157&view=diff
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- cfe/trunk/test/Modules/Inputs/DebugObjC.h (original)
>>>>>> +++ cfe/trunk/test/Modules/Inputs/DebugObjC.h Fri Oct 2 12:36:14 2015
>>>>>> @@ -5,6 +5,7 @@
>>>>>> }
>>>>>> + classMethod;
>>>>>> - instanceMethodWithInt:(int)i;
>>>>>> +- (struct OpaqueData*) getSomethingOpaque;
>>>>>> @property int property;
>>>>>> @end
>>>>>>
>>>>>>
>>>>>> Modified: cfe/trunk/test/Modules/ModuleDebugInfo.m
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/ModuleDebugInfo.m?rev=249157&r1=249156&r2=249157&view=diff
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- cfe/trunk/test/Modules/ModuleDebugInfo.m (original)
>>>>>> +++ cfe/trunk/test/Modules/ModuleDebugInfo.m Fri Oct 2 12:36:14 2015
>>>>>> @@ -41,3 +41,6 @@
>>>>>> // MODULE-CHECK: !DICompositeType(tag: DW_TAG_structure_type,
>>>>>> // MODULE-CHECK-SAME: name: "ObjCClass",
>>>>>> // MODULE-CHECK-SAME: scope: ![[MODULE]],
>>>>>> +
>>>>>> +// The forward declaration should not be in the module scope.
>>>>>> +// MODULE-CHECK: !DICompositeType(tag: DW_TAG_structure_type, name:
>>>>>> "OpaqueData", file
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> cfe-commits mailing list
>>>>>> cfe-commits at lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151005/f54a9859/attachment-0001.html>
More information about the cfe-commits
mailing list