[PATCH] D86185: [Cloning] Fix to cloning DISubprograms.

[Amy Huang via Phabricator via llvm-commits]

akhuang added inline comments.


================
Comment at: llvm/unittests/Transforms/Utils/CloningTest.cpp:740-744
+    !3 = distinct !DISubprogram(name: "my_operator", scope: !1, retainedNodes: !{!5})
+    !4 = distinct !DICompositeType(tag: DW_TAG_structure_type, scope: !3)
+    !5 = !DILocalVariable(name: "awaitables", scope: !3)
+    !6 = distinct !DISubprogram(name: "test", scope: !1, retainedNodes: !{!7})
+    !7 = !DILocalVariable(name: "w", scope: !6, type: !4)
----------------
dblaikie wrote:
> akhuang wrote:
> > dblaikie wrote:
> > > akhuang wrote:
> > > > dblaikie wrote:
> > > > > 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?
> > > > 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. 
> > > 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
> > 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.
> > 
> > 
> Ah, yeah - if the output is wrong/problematic even when it's verifier-clean, might be better to test for that directly.
> 
> 
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86185/new/

https://reviews.llvm.org/D86185