[PATCH] D70350: [DWARF] Allow cross-CU references of subprogram definitions

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a subscriber: manmanren.
vsk added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:201
     return false;
-  return (isa<DIType>(D) ||
-          (isa<DISubprogram>(D) && !cast<DISubprogram>(D)->isDefinition())) &&
-         !DD->generateTypeUnits();
+  return (isa<DIType>(D) || isa<DISubprogram>(D)) && !DD->generateTypeUnits();
 }
----------------
dblaikie wrote:
> Oh, hmm - seems this function isn't used for cross-CU inlining. Perhaps it should be refactored to go through here?
> 
> Also - any idea what the history was about the !Definition test in this function? What was that designed to avoid/handle?
Do you mean that the cross-CU inlining logic doesn't check that an abstract origin `isShareableAcrossCUs` before creating a cross-CU reference? It's possible some logic is duplicated there. I'll look into that.

Re: history, @manmanren introduced `isShareableAcrossCUs` in r193779. Based on the commit message, I'm not sure whether the goal was to share just types/declarations between CUs, and there's no mention of subprogram definitions. There's a reference to added test coverage, but I couldn't find any in that commit or in close-by ones. Certainly no tests broke when //removing// the `!cast<DISubprogram>(D)->isDefinition()` guard. I understand it would be more reassuring to know why the guard came into being in the first place, but I think the test case from this patch provides good justification (briefly: when generating a TAG_call_site for the call to `noinline_in_a` in b.c:main, the definition of `noinline_in_a` must be shareable across CUs for the `getDIE` call to succeed).


================
Comment at: llvm/test/DebugInfo/X86/lto-cross-cu-call-origin-ref.ll:5-26
+; // a.c
+; extern void helper_from_b(void);
+; __attribute__((optnone)) void bar() {}
+; __attribute__((optnone)) static void baz() {}
+; __attribute__((always_inline)) void foo() {
+;   bar();
+;   baz();
----------------
dblaikie wrote:
> This test case still seems a bit complicated - I'd probably avoid having "main" in it if not needed (because it has arguments and return values that then complicate the DWARF, etc - and extra semantics that probably aren't relevant to the test/readers trying to understand it)
> 
> Probably leave functions declared-but-not-defined (since you don't need to link the whole thing into an executable - juts to an object file, to look at the DWARF) rather than optnone, for one thing. & probably doesn't need to be compiled with optimizations enabled? (always_inline is probably enough to get the inlinings you're interested in) & some functions have "()" and others have "(void)" - prefer the former uniformly. You can use llvm-link to link together two llvm IR files, then run it back through clang (to go IR->bitcode-or-IR & just get the always_inline behavior at -O0), rather than needing save-temps, for what it's worth.
> 
> What are the cases this test is intended to exercise?
I think that the "main" in the test case adds some valuable coverage, as it lets us test that cross-CU references in call site tags are well formed. I'm not sure if that answers your last question fully, but that's the case the test is intended to exercise. The specific sub-cases tested by "main" are: same-CU ref, cross-CU ref to an external def, and cross-CU ref to a non-external def inlined via an external def. Additionally, the DWARF for `noinline_in_a` is checked to make sure cross-CU references can go in the other direction.

The arguments/return values in "main" do complicate the DWARF, but those bits of DWARF won't/shouldn't be checked here as that would be distracting. As for optimizations, clang won't emit call site info unless they are on, and they do tidy up the IR. However, `-Xclang -femit-debug-entry-values` can be dropped as that's not tested here -- I'll remove that.


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

https://reviews.llvm.org/D70350