Page MenuHomePhabricator

[DebugInfo][Docs] Document that prologue/epilogue variable location changes are ignored
ClosedPublic

Authored by jmorse on Jun 10 2019, 10:00 AM.

Details

Summary

This patch documents that LLVM does not describe all changes in variable locations during the prologue and the epilogue. The debugger doesn't / shouldn't step through that portion of the function anyway, and describing every location through such stages would bloat location lists.

Perform some minor cleanup at the same time,

  • Fix an enumerated list
  • Document that dbg.declare intrinsics have their variable location recorded in a MachineFunction table, not with DBG_VALUE meta-insts
  • Adds frame-indexes to the list of things that can be operands to DBG_VALUEs.

~

This is a result of discussion in D61940, note that my understanding of the rationale behind LLVMs current behaviour is mostly guided by [0].

[0] https://bugs.llvm.org/show_bug.cgi?id=40188#c8

Diff Detail

Repository
rL LLVM

Event Timeline

jmorse created this revision.Jun 10 2019, 10:00 AM
jmorse set the repository for this revision to rL LLVM.
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2019, 10:00 AM
bjope accepted this revision.Jun 10 2019, 10:38 AM

Looks good to me (matching my assumptions about why prolog/epilog is special).
Just a minor nit inline.

docs/SourceLevelDebugging.rst
586 ↗(On Diff #203847)

I guess the formatting was choosen to avoid unnecessary diffs.
I usually end up reading the .rst files as plain text more often than I read the docs using some browser etc. which makes this look weird.
nit: Not sure what the common practice is, but I don't mind if line breaks are updated for the full sentence/paragraph.

This revision is now accepted and ready to land.Jun 10 2019, 10:38 AM
rnk added a subscriber: rnk.Jun 10 2019, 11:25 AM

My initial reaction to the commit message was to disagree with it. In optimized builds, the SDAG builder does a bunch of work to track variable locations of arguments starting at function entry before the prologue is through. I think you can say something more general, which is that variables whose location is described by dbg.declare only ever have one location. That memory location may not be valid during the prologue. This documents what happens when you try to mix dbg.declare with dbg.value. In that case, dbg.declare wins.

echristo added a subscriber: echristo.

I'm tagging in Roland here since he and I were just discussing this the other day. I'm not entirely sure what we want to document here as I'm torn on whether or not we should be doing more here.

rnk added a comment.Jun 10 2019, 1:36 PM
In D63083#1536577, @rnk wrote:

My initial reaction to the commit message was to disagree with it.

I should add, it's really just the commit message that I find confusing. I think the documentation as written clearly documents LLVM's existing behavior.

jmorse updated this revision to Diff 204012.Jun 11 2019, 4:21 AM
jmorse edited the summary of this revision. (Show Details)

Whitespace change to ease reading of .rst, split summary into being an actual commit message

jmorse marked 2 inline comments as done.Jun 11 2019, 4:35 AM
In D63083#1536776, @rnk wrote:

I should add, it's really just the commit message that I find confusing. I think the documentation as written clearly documents LLVM's existing behavior.

Curses -- I've been treating the phab summary more as a "message to reviewers" rather than a commit message (I should have clocked this would cause confusion). I've edited in an actual commit message, up to the tilde character.

I'm tagging in Roland here since he and I were just discussing this the other day. I'm not entirely sure what we want to document here as I'm torn on whether or not we should be doing more here.

Cool -- by "more", you mean describe more variable locations in prologues / epilogues? This isn't an area of deep experience for me, I'd be interested in the reasoning for extending that information (is the explanation this patch adds to the documentation wrong/incomplete?)

docs/SourceLevelDebugging.rst
586 ↗(On Diff #203847)

Indeed @ the formatting, I've updated the diff to normalise the formatting, keeping this uncommitted for the moment due to the extra interest.

jmorse marked an inline comment as done.Jun 17 2019, 1:39 AM

Semi-ping: while this is approved, the extra interest suggested I should delay committing in case there's more discussion. Without hearing anything else, I'll land this in the next couple of days.

I think this is good to go.

rnk accepted this revision.Jun 17 2019, 10:52 AM

lgtm

This revision was automatically updated to reflect the committed changes.