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

[Teresa Johnson via llvm-commits]

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?

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170214/36670098/attachment.html>