Dwarf: Ensure tick parentage is preserved
ClosedPublic

Authored by bgamari on Oct 27 2015, 7:32 PM.

Details

Summary

Here we track tick parentage more carefully and introduce this information into
the produced DWARF DW_TAG_lexical_scope DIEs via a custom attribute.

Test Plan

Examine resulting DWARF structures

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bgamari updated this revision to Diff 4730.Oct 27 2015, 7:32 PM
bgamari retitled this revision from to [RFC] Dwarf: Ensure tick parentage is preserved.
bgamari updated this object.
bgamari edited the test plan for this revision. (Show Details)
bgamari added a reviewer: scpmw.
bgamari added a subscriber: scpmw.
scpmw edited edge metadata.EditedOct 29 2015, 5:35 AM

It's not quite as simple as this. It's probably best to look at the output of -ddump-debug to see what's going on, but here's roughly what we start with:

proc A [tick1, tick2]
  block B [tick3]
    proc C [tick4]

What debugSplitProcs does is reorganize this into separate trees by pulling out all debug blocks corresponding to CmmProcs to the top level, like follows:

proc A [tick1, tick2]
  block B [tick3]
proc C [tick4]

Now there are indeed two "correct" ways of going about this. Either we add the complete tick history of the parent:

proc A [tick1, tick2]
  block B [tick3]
proc C [tick1,tick2,tick3,tick4]

This is sort-of what you are doing - split gets called recursively, and therefore procedures floating up will get all ticks along the way (you also tick children that don't float, but that should be easy to correct). However

  • this might lead to quite a bit of tick duplication. That's a factor, given that debug information is already quite large. Plus GHC often generates very deeply nested CmmProcs, so this method might add dozens of ticks.
  • this loses information, because afterwards we cannot tell any more where the ticks were coming from.
  • (both issues become especially severe with CoreNotes. This was the nail in the coffin for me, but if we are willing to disregard this feature for the moment, it might not be as clean-cut.)

This is why my initial proposal was to add a parent relationship:

proc A [tick1, tick2]
  block B [tick3]
proc C [tick4] parent B

Which wouldn't be too hard to implement either - we add a dblScopeParent :: Maybe DebugBlock, have DWARF generation make a label for the B DIE, then reference it when generating C.

In D1387#39990, @scpmw wrote:

It's not quite as simple as this.

Thanks Peter! This is extremely helpful. I'll give it a try this weekend.

bgamari updated this revision to Diff 4806.Oct 30 2015, 3:55 PM
bgamari edited edge metadata.

Here we track tick parentage more carefully and introduce this information into
the produced DWARF DW_TAG_lexical_scope DIEs via a custom attribute.

bgamari updated this object.Oct 30 2015, 3:56 PM
bgamari edited edge metadata.

@scpmw, does this look better?

bgamari updated this revision to Diff 4824.Oct 31 2015, 3:58 PM
  • Dwarf: Emit tick parent attribute
scpmw added a comment.Oct 31 2015, 4:12 PM

Good work. Just two small suggestions, see inline comments.

compiler/nativeGen/Dwarf.hs
136–137

Note that you are setting the dblParent attribute on all blocks, even the ones that aren't floating. Not really a big problem, but slightly redundant. Simple fix would be to go:

dblParent = mfilter (/= prc) parent
compiler/nativeGen/Dwarf/Types.hs
170 ↗(On Diff #4824)

Might be an idea to make a new abbrev? With the change above this attribute should become fairly rare.

bgamari added inline comments.Oct 31 2015, 4:43 PM
compiler/nativeGen/Dwarf.hs
186

In order to make this work I needed to disable this behavior, including all blocks, even those optimized out, as they might have children. Intuitively I don't believe this should cost much space in "normal" programs, although I have yet to check this.

@scpmw, thoughts?

bgamari added inline comments.Oct 31 2015, 5:11 PM
compiler/nativeGen/Dwarf.hs
136–137

Fair point. Sounds like a plan.

compiler/nativeGen/Dwarf/Types.hs
170 ↗(On Diff #4824)

Sure, works for me.

bgamari updated this revision to Diff 4832.Oct 31 2015, 5:57 PM
bgamari edited edge metadata.

Address Peter's comments

bgamari marked 4 inline comments as done.Oct 31 2015, 5:57 PM

Done.

bgamari retitled this revision from [RFC] Dwarf: Ensure tick parentage is preserved to Dwarf: Ensure tick parentage is preserved.Oct 31 2015, 5:58 PM
bgamari edited edge metadata.

Unfortunately this will have some implications on the implementation of D1280, which will not longer be quite as simple a traversal as it is currently.

bgamari updated this revision to Diff 4864.Nov 1 2015, 11:07 AM
  • Dwarf: Ensure tick parentage is preserved
  • Dwarf: Emit tick parent attribute
bgamari added inline comments.Nov 2 2015, 6:04 AM
compiler/nativeGen/Dwarf.hs
186

It turns out that this is a bit more subtle than this. There are a few references that we need to ensure remain valid when we discard blocks,

  1. A DIE can refer to another as its dblParent
  2. A DIE's reference to its symbol

Keeping all of these valid turns out to be non-trivial for reasons I don't yet understand. At the moment I am trying to fix up the dblParent references in cmmDebugLink by choosing another parent for blocks whose parents end up being optimized out. It seems, however, that I am still somehow missing something as I still end up with undefined references to DIE symbols at link time.

bgamari added inline comments.Nov 2 2015, 6:07 AM
compiler/nativeGen/Dwarf.hs
186

Actually, the commit that I referenced doesn't even attempt to fix up the parent references but instead just throws away the parent relationship (for debugging purposes). Even in this extreme case I am still inexplicably seeing undefined references, hence my confusion.

bgamari updated this revision to Diff 4880.Nov 2 2015, 6:55 AM
bgamari edited edge metadata.

Finally correct?

As it turns out, the solution becomes quite straightforward if you just handle the reparenting in debugSplitProcs, which in hindsight is the right place for it to be anyways. This patch now implements this correctly.

compiler/nativeGen/Dwarf.hs
186

I believe this is now resolved.

As it turns out, the solution becomes quite straightforward if you just handle the reparenting in debugSplitProcs, which in hindsight is the right place for it to be anyways. This patch now implements this correctly.

bgamari updated this revision to Diff 4882.Nov 2 2015, 7:56 AM
bgamari edited edge metadata.
  • Dwarf: Emit tick parent attribute
scpmw added a comment.EditedNov 3 2015, 4:19 AM

Good point, optimised-out blocks might be a problem here... Especially because in theory they could have ticks (which are now lost). Maybe it would actually be better to let all blocks through, but omit code pointers for "empty" DIEs? I believe that's valid for DWARF.

In D1387#41173, @scpmw wrote:

Good point, optimised-out blocks might be a problem here... Especially because in theory they could have ticks (which are now lost). Maybe it would actually be better to let all blocks through, but omit code pointers for "empty" DIEs? I believe that's valid for DWARF.

I considered that but assumed this wouldn't be permitted by the spec. It seems, however, that you are right; the spec says subprograms "may" have {low,high}_pc attributes.

bgamari marked 4 inline comments as done.Nov 3 2015, 4:46 AM
bgamari updated this revision to Diff 5270.Nov 23 2015, 10:54 AM
bgamari edited edge metadata.

Emit all blocks

This revision was automatically updated to reflect the committed changes.