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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/CodeGen/LexicalScopes.cpp:87-94
+      // unit, but we could have instructions within a function body
+      // inlined from elsewhere in LTO mode).
+      auto *SP = dyn_cast<DISubprogram>(MIDL->getScope());
+      if (SP && SP->getUnit()->getEmissionKind() == DICompileUnit::NoDebug) {
+        PrevMI = &MInsn;
+        continue;
+      }
----------------
dblaikie wrote:
> Which test case/behavior does this address that the change in getOrCreateLexicalScope wasn't able to handle?
Good point, I think it doesn't add anything now (and now that I am looking at this I realize I forgot to change it to use getSubprogram() rather than use the dyn_cast). Let me try removing that.


================
Comment at: lib/CodeGen/LexicalScopes.cpp:182-184
+  assert(
+      I->second.getScopeNode()->getSubprogram()->getUnit()->getEmissionKind() !=
+      DICompileUnit::NoDebug);
----------------
dblaikie wrote:
> What about asserting in LexicalScope's ctor? 
> 
> This was what I tried & seemed to provide the checking desired?
> 
> diff --git include/llvm/CodeGen/LexicalScopes.h include/llvm/CodeGen/LexicalScopes.h
> index 7d7e48af2a0..b6d524fce44 100644
> --- include/llvm/CodeGen/LexicalScopes.h
> +++ include/llvm/CodeGen/LexicalScopes.h
> @@ -49,7 +49,11 @@ public:
>                 bool A)
>        : Parent(P), Desc(D), InlinedAtLocation(I), AbstractScope(A),
>          LastInsn(nullptr), FirstInsn(nullptr), DFSIn(0), DFSOut(0) {
> -    assert((!D || D->isResolved()) && "Expected resolved node");
> +    assert(D);
> +    assert(D->getSubprogram()->getUnit()->getEmissionKind() !=
> +               DICompileUnit::NoDebug &&
> +           "Don't build lexical scopes for non-debug locations");
> +    assert(D->isResolved() && "Expected resolved node");
>      assert((!I || I->isResolved()) && "Expected resolved node");
>      if (Parent)
>        Parent->addChild(this);
Good idea, will do that instead.


https://reviews.llvm.org/D29765