[PATCH] D29765: Handle link of NoDebug CU with a CU that has debug emission enabled
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 15 08:49:41 PST 2017
On Wed, Feb 15, 2017 at 8:31 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> 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)
>
The check added previously in this patch was to the instruction's SP within
the function, and in this case we had an instruction from f2 (inlined into
f3). I just confirmed that I can also fix the new issue in the test case by
adding the following check so that we never extractLexicalScopes in a
NoDebug function:
/// initialize - Scan machine function and constuct lexical scope nest.
void LexicalScopes::initialize(const MachineFunction &Fn) {
+ if (Fn.getFunction()->getSubprogram()->getUnit()->getEmissionKind() ==
+ DICompileUnit::NoDebug)
+ return;
(this is just above the invocation of extractLexicalScopes). I.e. instead
of adding the new guard before invoking getOrCreateRegularScope. I can use
that fix instead, since you're right, it doesn't make sense to attempt any
lexical scope creation for this function.
>
>> 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.
>
I decided to add an assert before the returns of getOrCreateInlinedScope
and getOrCreateRegularScope when we have constructed a new LexicalScope
(presumably not needed before the earlier return if we found an existing
LexicalScope in the map).
I'll finish testing with the latest fixes and update the patch including
the test case.
Thanks,
Teresa
>
>>
>> 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>
>>
>
--
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/20170215/9ef2201f/attachment.html>
More information about the llvm-commits
mailing list