[llvm] r204107 - DwarfDebug: Only unique retained types instead of all types.

Thu Mar 27 09:39:42 PDT 2014

On Mar 26, 2014, at 11:15 PM, WenHan Gu (谷汶翰) <wenhan.gu at gmail.com> wrote:

> Hi Adrian,
> 
> After this commit, there's sometimes assert fail on large bitcodes on my environment.

Is there any chance you can provide me with a reduced example that reproduces this?

> According to my result, two types seems the same, and I take out the assert fail can still build and run.
> Is any possible that the contained MDNode is not the same (since it is a pointer comparison indeed) but this is reasonable?
> Thanks!

Since the MDNodes are kept in a FoldingSet there should be no two identical nodes. What may happen in your case is that the source code contains two conflicting definitions of the same type in two compilation units. This is illegal C++ (violation of the ODR), but the compiler currently has no way to diagnose such a situation.

On a related note, this made me realize that this assertion should only be enabled for C++ compilation units; I will fix this soon. This will not fix your problem, unfortunately.

> 
> 
> Below is the detail information:
> 
> I try to dump, and it shows: �(The contents are the same)
> 
> Ty:
> [ DW_TAG_class_type ] [Vector2<int>] [line 12, size 64, align 32, offset 0] [def] [from ]
> resolve(Ty.getRef()):
> [ DW_TAG_class_type ] [Vector2<int>] [line 12, size 64, align 32, offset 0] [def] [from ]
> 
> and I dump the MDNode
> 
> !833 = metadata !{i32 786434, metadata !834, metadata !59, metadata !"list<Vector2<int>, std::allocator<Vector2<int> > >", i32 438, i64 128, i64 64, i32 0, i32 0, null, metadata !835, i32 0, null, metadata !970, metadata !"_ZTSSt4listI7Vector2IiESaIS1_EE"} ; [ DW_TAG_class_type ] [list<Vector2<int>, std::allocator<Vector2<int> > >] [line 438, size 128, align 64, offset 0] [def] [from ]
> 
> _ZTSSt4listI7Vector2IiESaIS1_EE is "typeinfo name of ..."
> 
> In other cases, the error is on the "typeinfo name of �..." either.
> 

Can you dump both MDNodes and see what the difference between them is? This would help us rule out or diagnose an ODR violation. If the two definitions are not in conflict, we have a bug.

thanks!
Adrian

> 
> 
> 2014-03-25 5:39 GMT+08:00 Adrian Prantl <aprantl at apple.com>:
> 
> On Mar 24, 2014, at 2:08 PM, Eric Christopher <echristo at gmail.com> wrote:
> 
> > On Mon, Mar 17, 2014 at 7:53 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >> On Mon, Mar 17, 2014 at 7:35 PM, Adrian Prantl <aprantl at apple.com> wrote:
> >>> Author: adrian
> >>> Date: Mon Mar 17 21:35:03 2014
> >>> New Revision: 204107
> >>>
> >>> URL: http://llvm.org/viewvc/llvm-project?rev=204107&view=rev
> >>> Log:
> >>> DwarfDebug: Only unique retained types instead of all types.
> >>> This is a follow-up to r203983 based on feedback from dblaikie and mren (Thanks!)
> >>> No functionality change.
> >>
> >> Thanks!
> >>
> >>>
> >>> Modified:
> >>> � �llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
> >>> � �llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
> >>>
> >>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
> >>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=204107&r1=204106&r2=204107&view=diff
> >>> ==============================================================================
> >>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
> >>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Mon Mar 17 21:35:03 2014
> >>> @@ -846,8 +846,13 @@ void DwarfDebug::beginModule() {
> >>> � � for (unsigned i = 0, e = EnumTypes.getNumElements(); i != e; ++i)
> >>> � � � CU->getOrCreateTypeDIE(EnumTypes.getElement(i));
> >>> � � DIArray RetainedTypes = CUNode.getRetainedTypes();
> >>> - � �for (unsigned i = 0, e = RetainedTypes.getNumElements(); i != e; ++i)
> >>> - � � �CU->getOrCreateTypeDIE(RetainedTypes.getElement(i));
> >>> + � �for (unsigned i = 0, e = RetainedTypes.getNumElements(); i != e; ++i) {
> >>> + � � �DIType Ty(RetainedTypes.getElement(i));
> >>> + � � �// The retained types array by design contains pointers to
> >>> + � � �// MDNodes rather than DIRefs. Unique them here.
> >>> + � � �DIType UniqueTy(resolve(Ty.getRef()));
> >>> + � � �CU->getOrCreateTypeDIE(UniqueTy);
> >>
> >> One thought I've had here: should we, rather than resolving the
> >> references and emitting them, avoid emitting the type if the resolved
> >> version isn't from this compilation unit?
> >>
> >> I'm not sure there's a good reason here, but I'm just tossing it
> >> around in my head - I wonder if we could get into confusing situations
> >> (with types as you've described in other recent threads - where two
> >> ODR-correct definitions, written in different files, may end up
> >> emitted under the "wrong" compilation unit). I'm not sure if this
> >> would actually be bad - but, for example, with my line table work it
> >> might be a tiny bit more verbose to emit a type from the wrong
> >> compilation unit since it wouldn't take advantage of the comp_dir as
> >> much (if the type was actually in a header in that directory) or might
> >> introduce an extra file name in the line table that would �otherwise
> >> have been avoided.
> >>
> >> This is ultra low priority - just sanity checking to see if maybe
> >> there are issues with emitting the type from the wrong unit.
> >>
> >
> > I think you're correct here. The way the code is written we're adding
> > retained types as we go over the compile unit. We should probably do a
> > check if the current compile unit is the same as the compile unit for
> > the type that we're about to emit since the type merging would have
> > put it in doubt which compile unit a retained type actually belonged
> > to.
> >
> >>> + � �}
> >>> � � // Emit imported_modules last so that the relevant context is already
> >>> � � // available.
> >>> � � for (unsigned i = 0, e = ImportedEntities.getNumElements(); i != e; ++i)
> >>>
> >>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
> >>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp?rev=204107&r1=204106&r2=204107&view=diff
> >>> ==============================================================================
> >>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp (original)
> >>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp Mon Mar 17 21:35:03 2014
> >>> @@ -971,6 +971,8 @@ DIE *DwarfUnit::getOrCreateTypeDIE(const
> >>>
> >>> � DIType Ty(TyNode);
> >>> � assert(Ty.isType());
> >>> + �assert(*&Ty == resolve(Ty.getRef()) &&
> >>
> >> Why's the "*&Ty" required? That seems a bit strange/subtle...
> >>
> >
> > Agreed. Adrian?
> 
> Absolutely. Removed in r204673.
> 
> -- adrian
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> 
> -- 
> Best Regards,
> WenHan Gu (Nowar)