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

Wed Feb 15 07:46:43 PST 2017

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.

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.

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

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

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/h
>>> tml/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/e1c04603/attachment.html>