On Mon, Oct 5, 2015 at 11:37 AM, David Blaikie <dblai...@gmail.com> wrote:
> > > On Mon, Oct 5, 2015 at 8:55 AM, Adrian Prantl <apra...@apple.com> wrote: > >> >> On Oct 2, 2015, at 3:24 PM, David Blaikie <dblai...@gmail.com> wrote: >> >> >> >> On Fri, Oct 2, 2015 at 3:21 PM, Adrian Prantl <apra...@apple.com> wrote: >> >>> >>> On Oct 2, 2015, at 3:01 PM, David Blaikie <dblai...@gmail.com> wrote: >>> >>> >>> >>> On Fri, Oct 2, 2015 at 3:00 PM, Adrian Prantl <apra...@apple.com> wrote: >>> >>>> >>>> On Oct 2, 2015, at 2:58 PM, David Blaikie <dblai...@gmail.com> wrote: >>>> >>>> >>>> >>>> On Fri, Oct 2, 2015 at 2:40 PM, Adrian Prantl <apra...@apple.com> >>>> wrote: >>>> >>>>> >>>>> On Oct 2, 2015, at 2:18 PM, David Blaikie <dblai...@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@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@lists.llvm.org >>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>> >>>>> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits