Page MenuHomePhabricator

[Cloning] Fix to cloning DISubprograms.
ClosedPublic

Authored by akhuang on Aug 18 2020, 6:06 PM.

Details

Summary

When trying to enable -debug-info-kind=constructor there was an assert
that occurs during debug info cloning ("mismatched subprogram between
llvm.dbg.value variable and !dbg attachment").
It appears that during llvm::CloneFunctionInto, a DISubprogram could be
duplicated when MapMetadata is called, and then added to the MD map again
when DIFinder gets a list of subprograms. This results in two different
versions of the DISubprogram.

This patch switches the order so that the DIFinder subprograms are
added before MapMetadata is called.

I'm not sure how to create a small IR testcase for this; there's a long-ish
c++ repro in the bug.

Fixes https://bugs.llvm.org/show_bug.cgi?id=46784

Diff Detail

Event Timeline

akhuang created this revision.Aug 18 2020, 6:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2020, 6:06 PM
akhuang requested review of this revision.Aug 18 2020, 6:06 PM

There are unit tests for CloneFunction in llvm/unittests/Transforms/Utils/CloningTest.cpp by the looks of it - perhaps this bug/fix can be tested by a new unit test there?

Thanks! Having a test would be really important to document the expected functionality and prevent future regressions.
Every time we change something here, we tend to find new unexpected behavior in a very complex LTO build a couple months later ;-)

llvm/lib/Transforms/Utils/CloneFunction.cpp
193

Can you add a comment here explaining what this loop does?

akhuang updated this revision to Diff 286915.Aug 20 2020, 4:20 PM

Add unit test and comment.

akhuang marked an inline comment as done.Aug 20 2020, 4:23 PM
dblaikie added inline comments.Aug 20 2020, 4:47 PM
llvm/unittests/Transforms/Utils/CloningTest.cpp
740–744

This debug info metadata looks like it has two cycles (my_operator -> awaitables -> my_operator and test -> w -> test). Are they both necessary? do they both test different cases? Might be worth a few more words in the test comment above about what they're exercising/what aspects of them are relevant/necessary to reproduce the issue?

akhuang added inline comments.Aug 20 2020, 5:51 PM
llvm/unittests/Transforms/Utils/CloningTest.cpp
740–744

I don't think either is necessary, actually. The important part for reproducing is just that there's a path from test -> my_operator. Maybe I can get rid of some more stuff; this is just what the metadata happened to look like in the repro.

dblaikie added inline comments.Aug 20 2020, 6:46 PM
llvm/unittests/Transforms/Utils/CloningTest.cpp
740–744

Ah, so the critical path is "test" -> "w" -> anonymous type (!4) -> "my_operator" ?

I guess this could happen in real-world non-coroutine code with templates like this:

template<typename T>
void f1() {
  T t;
}
void f2() {
  struct x { };
  f1<x>();
}

(compiled with optimizations (well, might have to add noinline or optnone to 'f1' to get it through optimizations) that should produce 't' in 'f1's retained types list, etc)

In any case, yeah - documenting that chain & trimming down anything else (I guess removing "awaitables"/removing "my_operator"s retainedNodes list) would be good

akhuang added inline comments.Aug 20 2020, 6:55 PM
llvm/unittests/Transforms/Utils/CloningTest.cpp
740–744

I guess the test would be clearer if it didn't rely on the verifier check and directly checked the metadata. The only reason "awaitables" exists is to create the situation where its scope doesn't match the DILocation scope.

dblaikie added inline comments.Aug 21 2020, 10:50 AM
llvm/unittests/Transforms/Utils/CloningTest.cpp
740–744

Ah, yeah - if the output is wrong/problematic even when it's verifier-clean, might be better to test for that directly.

akhuang added inline comments.Aug 21 2020, 10:55 AM
llvm/unittests/Transforms/Utils/CloningTest.cpp
740–744

well, I guess the thing that causes the failure is that "awaitables" points to one duplicate of "my_operator" and the DILocation points to another duplicate of "my_operator". So this is probably about as simplified as the test can be (as in, I can't remove the retainedNodes = "awaitables" part of the metadata).

akhuang updated this revision to Diff 287065.Aug 21 2020, 10:55 AM

Simplify test and add comments.

dblaikie accepted this revision.Aug 21 2020, 11:29 AM

Sounds good - thanks!

This revision is now accepted and ready to land.Aug 21 2020, 11:29 AM
This revision was automatically updated to reflect the committed changes.