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

Wed Feb 15 08:31:10 PST 2017

On Wed, Feb 15, 2017 at 7:46 AM Teresa Johnson <tejohnson at google.com> wrote:

> On Tue, Feb 14, 2017 at 7:55 PM, Teresa Johnson <tejohnson at google.com>
> wrote:
>
>
>
> On Tue, Feb 14, 2017 at 2:36 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
> 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());
>
>
> I had to change the above line slightly, to:
>    return getOrCreateLexicalScope(IA);
>
> (which essentially ends up mapping to: "return
> getOrCreateLexicalScope(IA->getScope(), IA->getInlinedAt());")
>
> 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.
>
> Does my tweak make sense?
>
>
> I verified that your change + my tweak (along with the original fix in
> extractLexicalScopes) allows the entire internal app to build.
>

Cool - I'm trying to understand/think about whether the original fix in
extractLexicalScopes is avoidable - I'd like to try to consolidate/collapse
the state space as much as possible rather than having several pieces, if
possible.

> 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.
>

Ah, good idea!

>
> 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).
>
> 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.
>

f3 is the nodebug function, right? I imagine LexicalScopes should never be
built for f3. So I'd probably look at why they're built at all, rather than
trying to make building them work (& again, could have an assertion to
check that it never happens)

> 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?
>
> I fixed the above issue by adding the following before
> invoking getOrCreateRegularScope from getOrCreateLexicalScope:
>
>   if (Scope->getSubprogram()->getUnit()->getEmissionKind() ==
>       DICompileUnit::NoDebug)
>     return nullptr;
>
> 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.
>

Sure, sounds reasonable/good. Not sure which/where the best spot for the
asserts would be, but certainly sounds like suring up some invariants here
would be a good thing.

>
> Also, I noticed that one of the first things done in
> both getOrCreateInlinedScope and getOrCreateRegularScope is:
>   Scope = Scope->getNonLexicalBlockFileScope();
> 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. =(
>

LexicalBlockFile is for changes to the current file that don't change the
scope (eg: if a file is textually #included inside a function definition,
the file would change). Scope->getSubprogram() and
Scope->getNonLexicalBlockFileScope()->getSubprogram() should always be the
same, I believe.

- Dave

>
> Thanks,
> Teresa
>
>
> Teresa
>
>      // 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
>
>
>
>
>
>
> --
> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
> 408-460-2413 <(408)%20460-2413>
>
>
>
>
> --
> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
> 408-460-2413 <(408)%20460-2413>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170215/dbab3e65/attachment.html>