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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:758
+
+        if (CalleeSP->isDefinition()) {
+          // Ensure that a subprogram definition for the callee and a containing
----------------
Why is this only for the definition case?  Is there any chance of coalescing it with the declaration case?


================
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();
 }
----------------
vsk wrote:
> 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).
> 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.

Yeah, cross-CU inlining makes the cross-CU choice in DwarfDebug::constructAbstractSubprogramScopeDIE here: https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L523

re: history/lack of testing - yeah, think we'll just have to accept that. Thanks for looking into it a bit further.


================
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();
----------------
vsk wrote:
> 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.
Still got a few "(void)" in here - please replace those with "()", I think (or make them all "(void)")? (though I guess we don't have an authoritative documentation on C style formatting, etc)

Ah, sorry - what I meant about "main" was basically "why main specifically?" why not another arbitrary function name with no parameters/return value? The code doesn't need to link into a valid executable - it can be only two IR modules linked together into one, that's how I've usually tested IR linking behavior in the past since it has fewer constraints like this, and being able to call undefined external functions is a nice hard-stop to the optimizer without needing optnone attributes (though the attributes can be useful in some cases, for sure)

Perhaps some comments at the top of the file, or within the source code comments explaining which cases are being tested would be helpful? (and/or renaming the functions in the source to self-document their purpose) or splitting each test case, while within the same two files, into separate function groups that don't interact with each other so they're more isolated/easier to read (eg: f1_* functions are all related to testing one situation, f2_* functions are for another situation, etc)




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

https://reviews.llvm.org/D70350