<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 14, 2017 at 7:55 PM, Teresa Johnson <span dir="ltr"><<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="gmail-m_5490049026779165918gmail-">On Tue, Feb 14, 2017 at 2:36 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br></span><span class="gmail-m_5490049026779165918gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">I think this is probably the right thing (patch attached, and text here for readability):<br><br><div>diff --git lib/CodeGen/LexicalScopes.cpp lib/CodeGen/LexicalScopes.cpp</div><div>index fc84ddb1f6b..31203689b3a 100644</div><div>--- lib/CodeGen/LexicalScopes.cpp</div><div>+++ lib/CodeGen/LexicalScopes.cpp</div><div>@@ -127,6 +127,9 @@ LexicalScope *LexicalScopes::findLexicalSco<wbr>pe(const DILocation *DL) {</div><div> LexicalScope *LexicalScopes::getOrCreateLex<wbr>icalScope(const DILocalScope *Scope,</div><div> const DILocation *IA) {</div><div> if (IA) {</div><div>+ if (Scope->getSubprogram()->getUn<wbr>it()->getEmissionKind() ==</div><div>+ DICompileUnit::NoDebug)</div><div>+ return getOrCreateLexicalScope(IA->ge<wbr>tScope());</div></div></blockquote><div><br></div></span><div>I had to change the above line slightly, to:</div><div> return getOrCreateLexicalScope(IA);</div><div><br></div><div>(which essentially ends up mapping to: "return getOrCreateLexicalScope(IA->ge<wbr>tScope(), IA->getInlinedAt());")</div><div><br></div><div>Otherwise, with the source file I tried, I the new recursive call to getOrCreateLexicalScope (where the IA->getScope() passed it was also in the NoDebug CU) ended up calling getOrCreateRegularScope, which asserted because Parent==nullptr and CurrentFnLexicalScope != nullptr.</div><div><br></div><div>Does my tweak make sense?</div></div></div></div></blockquote><div><br></div><div>I verified that your change + my tweak (along with the original fix in extractLexicalScopes) allows the entire internal app to build.</div><div><br></div><div>However, I went back and tried your below test case with these changes + some extra assertion checking. I added asserts in getOrCreateLexicalScope on the new LexicalScope returned by both getOrCreateInlinedScope and getOrCreateRegularScope, to ensure that neither is from a NoDebug CU.</div><div><br></div><div>Without the above fix, we as expected hit one of these asserts when we have called getOrCreateInlinedScope for f3, when extracting lexical scopes for f4 (which has f3 and f2 inlined into it).</div><div><br></div><div>However, with the above fix, we hit the assert when checking the return of getOrCreateRegularScope. This happens when extracting lexical scopes for f3, which has f2 inlined into it. In that case when we call getOrCreateLexicalScope for the Scope f3 (invoked from getOrCreateInlinedScope for f2), it does not have an inlinedAt so we instead invoke getOrCreateRegularScope, which did not have the new check. Interestingly, this does not hit the original AsmPrinter failure I saw when trying to generate dwarf DIEs. However, I assume we want to be consistent and not have the LexicalScope generated ever for a NoDebug CU's SPs, right?</div><div><br></div><div>I fixed the above issue by adding the following before invoking getOrCreateRegularScope from getOrCreateLexicalScope:</div><div><br></div><div><div> if (Scope->getSubprogram()->getUnit()->getEmissionKind() ==</div><div> DICompileUnit::NoDebug)</div><div> return nullptr;</div></div><div><br></div><div>I think it may be useful to add in some NDEBUG assertion checking like the kind I had that caught the above issue, to ensure we never create a LexicalScope for a NoDebug CU - does that sound reasonable? I am going to rebuild the internal app with the above fix and assertion checking added.</div><div><br></div><div>Also, I noticed that one of the first things done in both getOrCreateInlinedScope and getOrCreateRegularScope is:</div><div> Scope = Scope->getNonLexicalBlockFileScope();<br></div><div>Do we need to check this resulting Scope, or will it always be the case that both the original and new Scope above will come from the same CU? I am not familiar with LexicalBlockFileScope and there aren't any comments about this class in DebugInfoMetadata.h. =(</div><div><br></div><div>Thanks,</div><div>Teresa</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="gmail-m_5490049026779165918gmail-HOEnZb"><font color="#888888"><div><br></div><div>Teresa</div></font></span><div><div class="gmail-m_5490049026779165918gmail-h5"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div> // Create an abstract scope for inlined function.</div><div> getOrCreateAbstractScope(Scop<wbr>e);</div><div> // Create an inlined scope for inlined function.<br><br>And here's my test case:<br><br><div><font face="monospace">$ cat a.cpp</font></div><div><font face="monospace">void f1();</font></div><div><font face="monospace">__attribute__((always_inline)) void f2() {</font></div><div><font face="monospace"> f1();</font></div><div><font face="monospace">}</font></div><div><font face="monospace">void f3();</font></div><div><font face="monospace">void f4() {</font></div><div><font face="monospace"> f3();</font></div><div><font face="monospace">}</font></div><div><font face="monospace">$ cat b.cpp</font></div><div><font face="monospace">void f2();</font></div><div><font face="monospace">__attribute__((always_inline)) void f3() {</font></div><div><font face="monospace"> f2();</font></div><div><font face="monospace">}</font></div></div><div><font face="monospace">$ clang++ -flto a.cpp -g -c</font></div><div><font face="monospace">$ clang++ -flto b.cpp -Rpass=inline -c</font></div><div><font face="monospace">$ llvm-link {a,b}.o -o - | opt -O2 - -o ab.bc</font></div><div><font face="monospace">$ llc ab.bc</font></div><br><br>It might be worth testing with more than one level of nodebug function in the middle (f2_1, f2_2) - my first pass I called "getOrCreateRegularScope" which would've been incorrect in that case, I think. (you could change it back to that, demonstrate that it breaks, and that using getOrCreateLexicalScope fixes it - if you like) - I'm not sure I'd bother committing such test cases - this one seems to suffice, but just for some general confidence in the change.<br><br>But it's probably good as-is/without worrying too much, I think.<br><br>- Dave<div><div class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-h5"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Feb 14, 2017 at 9:48 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr" class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"><div class="gmail_quote gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"><div dir="ltr" class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">On Tue, Feb 14, 2017 at 9:41 AM Teresa Johnson via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"></div><blockquote class="gmail_quote gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">tejohnson added inline comments.<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
================<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
Comment at: lib/CodeGen/LexicalScopes.cpp:<wbr>82<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
+ // Ignore locations linked from a NoDebug compile unit.<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
+ auto *SP = dyn_cast<DISubprogram>(MIDL->g<wbr>etScope());<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
+ if (SP && SP->getUnit()->getEmissionKind<wbr>() == DICompileUnit::NoDebug) {<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
----------------<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
dblaikie wrote:<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
> This probably won't catch all cases - if the location was in a subscope within the function (eg: try adding a {} to the function which should add an extra scope). Walking the scope chain until a DISubprogram is probably needed here - I forget if we have a nice wrapped up utility for that. Don't see one at a glance.<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
><br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
> It should be an invariant/guaranteed that a DISubprogram is reached (& is the end of the scope chain), so something like:<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
><br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
> oh, wait, no, here it is: <a href="http://llvm.org/docs/doxygen/html/classllvm_1_1DILocalScope.html#a747dd1690fc477a8d9638fa37a30ced8" rel="noreferrer" class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg" target="_blank">http://llvm.org/docs/doxygen/h<wbr>tml/classllvm_1_1DILocalScope.<wbr>html#a747dd1690fc477a8d9638fa3<wbr>7a30ced8</a><br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
><br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
> so, I think, maybe:<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
><br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
> if (cast<DILocalScope>(MIDL->getS<wbr>cope())->getSubprogram()->getU<wbr>nit()->getEmissionKind() == DICompileUnit::NoDebug) {<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
> ...<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
I had just found the DILocalScope::getSubprogram() helper, since I was still getting the original seg fault when I tried the original failing internal code with this change. So now I have:<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
// Ignore locations linked from a NoDebug compile unit.<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
auto *SP = MIDL->getScope()->getSubprogra<wbr>m();<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
if (SP->getUnit()->getEmissionKin<wbr>d() == DICompileUnit::NoDebug) {<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
PrevMI = &MInsn;<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
continue;<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
}<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
(Don't need to cast MIDL->getScope() since it returns a DILocalScope).</blockquote><div class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"><br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"></div></div></div><div dir="ltr" class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"><div class="gmail_quote gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"><div class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">Ah, handy!</div></div></div><div dir="ltr" class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"><div class="gmail_quote gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"><div class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"> </div><blockquote class="gmail_quote gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> But I am *still* getting the original seg fault. Any idea what else I could be missing?<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"></blockquote><div class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"><br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"></div></div></div><div dir="ltr" class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"><div class="gmail_quote gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"><div class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">Not sure, no, sorry :/ I know reducing LTO test cases is difficult, but if this works for the simple test case and doesn't in the original failing situation it'll be good to understand why.</div></div></div><div dir="ltr" class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"><div class="gmail_quote gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"><div class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg"> </div><blockquote class="gmail_quote gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
================<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
Comment at: test/DebugInfo/Generic/debug_a<wbr>nd_nodebug_CUs.ll:4<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
+<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
+; RUN: llc %s -o - | FileCheck %s<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
+; CHECK-DAG: # debug.c:<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
----------------<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
dblaikie wrote:<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
> Include the steps for reproducing this test case (basically the RUN lines, and the original C source/steps for producing the IR, from the first version of the test) - helps to visualize/document/reproduce the situation.<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
Will do<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
<a href="https://reviews.llvm.org/D29765" rel="noreferrer" class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg" target="_blank">https://reviews.llvm.org/D2976<wbr>5</a><br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
<br class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail-m_8251086938961588333gmail_msg">
</blockquote></div></div></blockquote></div></div></div></div>
</blockquote></div></div></div><br><br clear="all"><span class="gmail-m_5490049026779165918gmail-"><div><br></div>-- <br><div class="gmail-m_5490049026779165918gmail-m_5704362587694747824gmail_signature"><span style="font-family:times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top:2px solid rgb(213,15,37)">Teresa Johnson |</td><td nowrap style="border-top:2px solid rgb(51,105,232)"> Software Engineer |</td><td nowrap style="border-top:2px solid rgb(0,153,57)"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top:2px solid rgb(238,178,17)"> <a href="tel:(408)%20460-2413" value="+14084602413" target="_blank">408-460-2413</a></td></tr></tbody></table></span></div>
</span></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail-m_5490049026779165918gmail_signature"><span style="font-family:times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top:2px solid rgb(213,15,37)">Teresa Johnson |</td><td nowrap style="border-top:2px solid rgb(51,105,232)"> Software Engineer |</td><td nowrap style="border-top:2px solid rgb(0,153,57)"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top:2px solid rgb(238,178,17)"> <a href="tel:(408)%20460-2413" value="+14084602413" target="_blank">408-460-2413</a></td></tr></tbody></table></span></div>
</div></div>