[PATCH] D29765: Handle link of NoDebug CU with a CU that has debug emission enabled

[David Blaikie via llvm-commits]

I think this is probably the right thing (patch attached, and text here for
readability):

diff --git lib/CodeGen/LexicalScopes.cpp lib/CodeGen/LexicalScopes.cpp
index fc84ddb1f6b..31203689b3a 100644
--- lib/CodeGen/LexicalScopes.cpp
+++ lib/CodeGen/LexicalScopes.cpp
@@ -127,6 +127,9 @@ LexicalScope *LexicalScopes::findLexicalScope(const
DILocation *DL) {
 LexicalScope *LexicalScopes::getOrCreateLexicalScope(const DILocalScope
*Scope,
                                                      const DILocation *IA)
{
   if (IA) {
+    if (Scope->getSubprogram()->getUnit()->getEmissionKind() ==
+        DICompileUnit::NoDebug)
+      return getOrCreateLexicalScope(IA->getScope());
     // Create an abstract scope for inlined function.
     getOrCreateAbstractScope(Scope);
     // Create an inlined scope for inlined function.

And here's my test case:

$ cat a.cpp
void f1();
__attribute__((always_inline)) void f2() {
  f1();
}
void f3();
void f4() {
  f3();
}
$ cat b.cpp
void f2();
__attribute__((always_inline)) void f3() {
  f2();
}
$ clang++ -flto a.cpp -g -c
$ clang++ -flto b.cpp -Rpass=inline -c
$ llvm-link {a,b}.o -o - | opt -O2 - -o ab.bc
$ llc ab.bc


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.

But it's probably good as-is/without worrying too much, I think.

- Dave

On Tue, Feb 14, 2017 at 9:48 AM David Blaikie <dblaikie at gmail.com> wrote:

> On Tue, Feb 14, 2017 at 9:41 AM Teresa Johnson via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> tejohnson added inline comments.
>
>
> ================
> Comment at: lib/CodeGen/LexicalScopes.cpp:82
> +      // Ignore locations linked from a NoDebug compile unit.
> +      auto *SP = dyn_cast<DISubprogram>(MIDL->getScope());
> +      if (SP && SP->getUnit()->getEmissionKind() ==
> DICompileUnit::NoDebug) {
> ----------------
> dblaikie wrote:
> > 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.
> >
> > It should be an invariant/guaranteed that a DISubprogram is reached (&
> is the end of the scope chain), so something like:
> >
> > oh, wait, no, here it is:
> http://llvm.org/docs/doxygen/html/classllvm_1_1DILocalScope.html#a747dd1690fc477a8d9638fa37a30ced8
> >
> > so, I think, maybe:
> >
> >   if
> (cast<DILocalScope>(MIDL->getScope())->getSubprogram()->getUnit()->getEmissionKind()
> == DICompileUnit::NoDebug) {
> >     ...
> 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:
>
>       // Ignore locations linked from a NoDebug compile unit.
>       auto *SP = MIDL->getScope()->getSubprogram();
>       if (SP->getUnit()->getEmissionKind() == DICompileUnit::NoDebug) {
>         PrevMI = &MInsn;
>         continue;
>       }
>
> (Don't need to cast MIDL->getScope() since it returns a DILocalScope).
>
>
> Ah, handy!
>
>
> But I am *still* getting the original seg fault. Any idea what else I
> could be missing?
>
>
> 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.
>
>
>
>
> ================
> Comment at: test/DebugInfo/Generic/debug_and_nodebug_CUs.ll:4
> +
> +; RUN: llc %s -o - | FileCheck %s
> +; CHECK-DAG: # debug.c:
> ----------------
> dblaikie wrote:
> > 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.
> Will do
>
>
> https://reviews.llvm.org/D29765
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170214/58457b36/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nodebug.diff
Type: text/x-patch
Size: 739 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170214/58457b36/attachment-0001.bin>