<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Sep 10, 2013 at 2:17 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">>> Testcase? How did you see this?<br>
><br>
> This should not affect functionality :)<br>
<br>
</div>Oh good. I was hoping not. How did you come across this? I'm curious.<br>
<div class="im"><br>
> It basically swaps the order of creation of Scope DIE vs. creation of<br>
> children.<br>
><br>
<br>
</div>Yeah. It could use a lot more documentation.<br>
<div class="im"><br>
> In the process, we removed an early exit which only captures some cases of<br>
> creating dangling children.<br>
> - // Early return to avoid creating dangling variable|scope DIEs.<br>
> - if (!Scope->getInlinedAt() && DS.isSubprogram() &&<br>
> Scope->isAbstractScope() &&<br>
> - !TheCU->getDIE(DS))<br>
> - return NULL;<br>
><br>
> Creation of unused children then removing them are not efficient and it may<br>
> cause problems if a member<br>
> of DwarfDebug or CompileUnit holds a reference to the deleted DIE.<br>
><br>
<br>
</div>Makes sense, but I think only if you actually create them :)<br>
<div class="im"><br>
>><br>
>><br>
>> For some of the patch:<br>
>><br>
>> +bool DwarfDebug::isLexicalScopeDIENull(LexicalScope *Scope) {<br>
>> + if (Scope->isAbstractScope())<br>
>><br>
>> Needs a block comment.<br>
><br>
> I have comments at the header file, I will copy it over to the source file.<br>
<br>
</div>Please be more detailed than in the header file.<br>
<div class="im"><br>
>><br>
>><br>
>> + SmallVectorImpl<InsnRange>::const_iterator RI = Ranges.begin();<br>
>> + MCSymbol *End = getLabelAfterInsn(RI->second);<br>
>> + return !End;<br>
>><br>
>> Where does this show up?<br>
><br>
> I write up isLexicalScopeDIENull from constructLexicalScopeDIE.<br>
> constructLexicalScopeDIE returns null<br>
> when End is null. Is that what you are asking?<br>
><br>
<br>
</div>Yep. I'm still wondering where this shows up. :\<br></blockquote><div><br></div><div>Yeah, I'm rather curious about this. Given that we don't have a list of lexical scope children in their parent scope, the only reason a lexical scope even stays alive is by virtue of its children (instructions, variables, etc) staying alive within it - if all the children go away, the lexical scope should be dropped or otherwise never be seen/reached.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> Thanks,<br>
> Manman<br>
>><br>
>><br>
>> -eric<br>
>><br>
>><br>
>> On Tue, Sep 10, 2013 at 11:40 AM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
>> > Author: mren<br>
>> > Date: Tue Sep 10 13:40:41 2013<br>
>> > New Revision: 190421<br>
>> ><br>
>> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=190421&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=190421&view=rev</a><br>
>> > Log:<br>
>> > Debug Info: create scope children DIEs when the scope DIE is not null.<br>
>> ><br>
>> > We try to create the scope children DIEs after we create the scope DIE.<br>
>> > But<br>
>> > to avoid emitting empty lexical block DIE, we first check whether a<br>
>> > scope<br>
>> > DIE is going to be null, then create the scope children if it is not<br>
>> > null.<br>
>> > From the number of children, we decide whether to actually create the<br>
>> > scope DIE.<br>
>> ><br>
>> > This patch also removes an early exit which checks for a special<br>
>> > condition.<br>
>> > It also removes deletion of un-used children DIEs that are generated<br>
>> > because we used to generate children DIEs before the scope DIE.<br>
>> ><br>
>> > Deletion of un-used children DIEs may cause problem because we sometimes<br>
>> > keep<br>
>> > created DIEs in a member variable of a CU.<br>
>> ><br>
>> > Modified:<br>
>> > llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp<br>
>> > llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h<br>
>> ><br>
>> > Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=190421&r1=190420&r2=190421&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=190421&r1=190420&r2=190421&view=diff</a><br>
>> ><br>
>> > ==============================================================================<br>
>> > --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)<br>
>> > +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Tue Sep 10 13:40:41<br>
>> > 2013<br>
>> > @@ -424,18 +424,34 @@ DIE *DwarfDebug::updateSubprogramScopeDI<br>
>> > return SPDie;<br>
>> > }<br>
>> ><br>
>> > +bool DwarfDebug::isLexicalScopeDIENull(LexicalScope *Scope) {<br>
>> > + if (Scope->isAbstractScope())<br>
>> > + return false;<br>
>> > +<br>
>> > + const SmallVectorImpl<InsnRange> &Ranges = Scope->getRanges();<br>
>> > + if (Ranges.empty())<br>
>> > + return true;<br>
>> > +<br>
>> > + if (Ranges.size() > 1)<br>
>> > + return false;<br>
>> > +<br>
>> > + SmallVectorImpl<InsnRange>::const_iterator RI = Ranges.begin();<br>
>> > + MCSymbol *End = getLabelAfterInsn(RI->second);<br>
>> > + return !End;<br>
>> > +}<br>
>> > +<br>
>> > // Construct new DW_TAG_lexical_block for this scope and attach<br>
>> > // DW_AT_low_pc/DW_AT_high_pc labels.<br>
>> > DIE *DwarfDebug::constructLexicalScopeDIE(CompileUnit *TheCU,<br>
>> > LexicalScope *Scope) {<br>
>> > + if (isLexicalScopeDIENull(Scope))<br>
>> > + return 0;<br>
>> > +<br>
>> > DIE *ScopeDIE = new DIE(dwarf::DW_TAG_lexical_block);<br>
>> > if (Scope->isAbstractScope())<br>
>> > return ScopeDIE;<br>
>> ><br>
>> > const SmallVectorImpl<InsnRange> &Ranges = Scope->getRanges();<br>
>> > - if (Ranges.empty())<br>
>> > - return 0;<br>
>> > -<br>
>> > // If we have multiple ranges, emit them into the range section.<br>
>> > if (Ranges.size() > 1) {<br>
>> > // .debug_range section has not been laid out yet. Emit offset in<br>
>> > @@ -460,8 +476,7 @@ DIE *DwarfDebug::constructLexicalScopeDI<br>
>> > SmallVectorImpl<InsnRange>::const_iterator RI = Ranges.begin();<br>
>> > MCSymbol *Start = getLabelBeforeInsn(RI->first);<br>
>> > MCSymbol *End = getLabelAfterInsn(RI->second);<br>
>> > -<br>
>> > - if (End == 0) return 0;<br>
>> > + assert(End && "End label should not be null!");<br>
>> ><br>
>> > assert(Start->isDefined() && "Invalid starting label for an inlined<br>
>> > scope!");<br>
>> > assert(End->isDefined() && "Invalid end label for an inlined<br>
>> > scope!");<br>
>> > @@ -540,19 +555,9 @@ DIE *DwarfDebug::constructInlinedScopeDI<br>
>> > return ScopeDIE;<br>
>> > }<br>
>> ><br>
>> > -// Construct a DIE for this scope.<br>
>> > -DIE *DwarfDebug::constructScopeDIE(CompileUnit *TheCU, LexicalScope<br>
>> > *Scope) {<br>
>> > - if (!Scope || !Scope->getScopeNode())<br>
>> > - return NULL;<br>
>> > -<br>
>> > - DIScope DS(Scope->getScopeNode());<br>
>> > - // Early return to avoid creating dangling variable|scope DIEs.<br>
>> > - if (!Scope->getInlinedAt() && DS.isSubprogram() &&<br>
>> > Scope->isAbstractScope() &&<br>
>> > - !TheCU->getDIE(DS))<br>
>> > - return NULL;<br>
>> > -<br>
>> > - SmallVector<DIE *, 8> Children;<br>
>> > - DIE *ObjectPointer = NULL;<br>
>> > +DIE *DwarfDebug::createScopeChildrenDIE(CompileUnit *TheCU,<br>
>> > LexicalScope *Scope,<br>
>> > + SmallVectorImpl<DIE*><br>
>> > &Children) {<br>
>> > + DIE *ObjectPointer = NULL;<br>
>> ><br>
>> > // Collect arguments for current function.<br>
>> > if (LScopes.isCurrentFunctionScope(Scope))<br>
>> > @@ -576,6 +581,20 @@ DIE *DwarfDebug::constructScopeDIE(Compi<br>
>> > for (unsigned j = 0, M = Scopes.size(); j < M; ++j)<br>
>> > if (DIE *Nested = constructScopeDIE(TheCU, Scopes[j]))<br>
>> > Children.push_back(Nested);<br>
>> > + return ObjectPointer;<br>
>> > +}<br>
>> > +<br>
>> > +// Construct a DIE for this scope.<br>
>> > +DIE *DwarfDebug::constructScopeDIE(CompileUnit *TheCU, LexicalScope<br>
>> > *Scope) {<br>
>> > + if (!Scope || !Scope->getScopeNode())<br>
>> > + return NULL;<br>
>> > +<br>
>> > + DIScope DS(Scope->getScopeNode());<br>
>> > +<br>
>> > + SmallVector<DIE *, 8> Children;<br>
>> > + DIE *ObjectPointer = NULL;<br>
>> > + bool ChildrenCreated = false;<br>
>> > +<br>
>> > DIE *ScopeDIE = NULL;<br>
>> > if (Scope->getInlinedAt())<br>
>> > ScopeDIE = constructInlinedScopeDIE(TheCU, Scope);<br>
>> > @@ -591,6 +610,12 @@ DIE *DwarfDebug::constructScopeDIE(Compi<br>
>> > ScopeDIE = updateSubprogramScopeDIE(TheCU, DS);<br>
>> > }<br>
>> > else {<br>
>> > + if (isLexicalScopeDIENull(Scope))<br>
>> > + return NULL;<br>
>> > + // We create children only when we know the scope DIE is not going<br>
>> > to be<br>
>> > + // null.<br>
>> > + ObjectPointer = createScopeChildrenDIE(TheCU, Scope, Children);<br>
>> > + ChildrenCreated = true;<br>
>> > // There is no need to emit empty lexical block DIE.<br>
>> > std::pair<ImportedEntityMap::const_iterator,<br>
>> > ImportedEntityMap::const_iterator> Range =<br>
>> > std::equal_range(<br>
>> > @@ -600,15 +625,19 @@ DIE *DwarfDebug::constructScopeDIE(Compi<br>
>> > if (Children.empty() && Range.first == Range.second)<br>
>> > return NULL;<br>
>> > ScopeDIE = constructLexicalScopeDIE(TheCU, Scope);<br>
>> > + assert(ScopeDIE && "Scope DIE should not be null.");<br>
>> > for (ImportedEntityMap::const_iterator i = Range.first; i !=<br>
>> > Range.second;<br>
>> > ++i)<br>
>> > constructImportedEntityDIE(TheCU, i->second, ScopeDIE);<br>
>> > }<br>
>> ><br>
>> > if (!ScopeDIE) {<br>
>> > - std::for_each(Children.begin(), Children.end(), deleter<DIE>);<br>
>> > + assert(Children.empty() &&<br>
>> > + "We create children only when the scope DIE is not null.");<br>
>> > return NULL;<br>
>> > }<br>
>> > + if (!ChildrenCreated)<br>
>> > + ObjectPointer = createScopeChildrenDIE(TheCU, Scope, Children);<br>
>> ><br>
>> > // Add children<br>
>> > for (SmallVectorImpl<DIE *>::iterator I = Children.begin(),<br>
>> ><br>
>> > Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h?rev=190421&r1=190420&r2=190421&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h?rev=190421&r1=190420&r2=190421&view=diff</a><br>
>> ><br>
>> > ==============================================================================<br>
>> > --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h (original)<br>
>> > +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h Tue Sep 10 13:40:41<br>
>> > 2013<br>
>> > @@ -469,6 +469,9 @@ private:<br>
>> > /// \brief Construct new DW_TAG_lexical_block for this scope and<br>
>> > /// attach DW_AT_low_pc/DW_AT_high_pc labels.<br>
>> > DIE *constructLexicalScopeDIE(CompileUnit *TheCU, LexicalScope<br>
>> > *Scope);<br>
>> > + /// A helper function to check whether the DIE for a given Scope is<br>
>> > going<br>
>> > + /// to be null.<br>
>> > + bool isLexicalScopeDIENull(LexicalScope *Scope);<br>
>> ><br>
>> > /// \brief This scope represents inlined body of a function.<br>
>> > Construct<br>
>> > /// DIE to represent this concrete inlined copy of the function.<br>
>> > @@ -476,6 +479,9 @@ private:<br>
>> ><br>
>> > /// \brief Construct a DIE for this scope.<br>
>> > DIE *constructScopeDIE(CompileUnit *TheCU, LexicalScope *Scope);<br>
>> > + /// A helper function to create children of a Scope DIE.<br>
>> > + DIE *createScopeChildrenDIE(CompileUnit *TheCU, LexicalScope *Scope,<br>
>> > + SmallVectorImpl<DIE*> &Children);<br>
>> ><br>
>> > /// \brief Emit initial Dwarf sections with a label at the start of<br>
>> > each one.<br>
>> > void emitSectionLabels();<br>
>> ><br>
>> ><br>
>> > _______________________________________________<br>
>> > llvm-commits mailing list<br>
>> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>