Page MenuHomePhabricator

[DWARF] Revert sharing subprograms across CUs
ClosedPublic

Authored by jmorse on Jul 29 2021, 7:25 AM.

Details

Summary

Over in D70350, DW_TAG_subprograms were permitted to be referenced across CU boundaries, to improve stack trace construction using call site information. Unfortunately, as documented in PR48790 [0], the way that subprograms are "owned" by dwarf units is sufficiently complicated that subprograms end up in unexpected units, invalidating cross-unit references.

I tried to fix this in D94976, however it uncovered additional problems with unit ownership of subprograms. @kyulee reports running into illegal DWARF references too, thus we're left with the choices of:

  • Knowingly emitting illegal DWARF in certain circumstances,
  • Damaging stack traces in certain circumstances.

Of which I would pick the latter: it's better to sometimes have a worse debugging experience, than to have scenarios where debugging is completely impossible. Opinions may vary; feedback most appreciated, especially @vsk @aprantl who'll suffer the backtrace damage. The intention is to cherry-pick this into llvm-13 too.

This patch is a revert of e08f205f5c2c, with some light conflicts around the declaration of constructCallSiteEntryDIE, nothing especially interesting. Three tests change in addition to the reversion:

  • dbgcall-site-indirect-param.mir , has an additional function and DW_TAG_formal_parameter placed ahead of the targeted function in dwarfdump output,
  • callsite-stack-value.mir , same problem as dbgcall-site-indirect-param.mir
  • convert-loclist.ll : the DWO_id number changes, which I don't think is a suprise.

In addition I've added subprogram-across-cus.ll from D94976 as a regression test, it compiles some IR and then runs llvm-dwarfdump -verify on it.

[0] https://bugs.llvm.org/show_bug.cgi?id=48790

Diff Detail

Event Timeline

jmorse created this revision.Jul 29 2021, 7:25 AM
jmorse requested review of this revision.Jul 29 2021, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2021, 7:25 AM

I'm generally in favor of this direction forward - I don't think it should fall to you to fix the debug info and if it isn't at the top of @vsk's list for a bit, let's fallback to known good-ish state for now.

@vsk - you might want to talk to @dexonsmith and @aprantl about the IR representation of these extra DISubprogram declarations that're being used for call site metadata, their lack of appropriate deduplication I think means you/we should consider some significant representational changes/fixes to make this work more reliably/consistently. See https://reviews.llvm.org/D94976#2508262 for more details.

vsk accepted this revision.Jul 29 2021, 10:55 AM

Hi @jmorse, thanks so much for putting this together. I agree that reverting does seem like the best option right now.

This revision is now accepted and ready to land.Jul 29 2021, 10:55 AM
This revision was landed with ongoing or failed builds.Aug 9 2021, 4:44 AM
This revision was automatically updated to reflect the committed changes.