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

[Vedant Kumar via Phabricator via llvm-commits]

vsk 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
----------------
dblaikie wrote:
> Why is this only for the definition case?  Is there any chance of coalescing it with the declaration case?
The DIEs for declarations are created in `DwarfDebug::getOrCreateDwarfCompileUnit`. Those cannot be constructed lazily as they are here, because they may be needed by other CUs.


================
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:
> 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)
> 
> 
I see, I've gotten rid of the '(void)', dropped the dependence on main/ld64/-flto, and renamed the test functions so that they more clearly reflect the functionality being tested (& added some more localized comments).


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

https://reviews.llvm.org/D70350