Page MenuHomePhabricator

[DebugInfo][Docs] Explicitly document how dbg.value intrinsics are interpreted in optimized code
ClosedPublic

Authored by jmorse on Feb 27 2019, 9:45 AM.

Details

Summary

As discussed in D58453, the interpretation of dbg.value intrinsics and how they should be treated in optimized code isn't very well documented at the moment. This patch adds a section, ``Object lifetime in optimized code'' to the source level debugging documentation, that documents how such intrinsics are Supposed To (TM) be handled. This may be a can of worms because not everyone may agree that this is how they're interpreted.

Further documentation can be added, but what I wanted to get out there at this point is:

  • dbg.values terminate earlier locations and their position in the instruction stream is significant, and
  • that it's better to make a variable read ``optimized out'' than it is to present a state (set of variable valuations) that misrepresents the program,

which if we can agree on, will be Progress (TM).

Note that I haven't written any restructured text before :o

Diff Detail

Repository
rL LLVM

Event Timeline

jmorse created this revision.Feb 27 2019, 9:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2019, 9:45 AM

Thanks! Small nitpicks inside.

docs/SourceLevelDebugging.rst
401 ↗(On Diff #188572)

SSA values

403 ↗(On Diff #188572)

values ...

408 ↗(On Diff #188572)

for (any overlapping fragments of) the

419 ↗(On Diff #188572)
``llvm.dbg.value``
426 ↗(On Diff #188572)

and undermining their trust in the debugger.

429 ↗(On Diff #188572)

ditto

430 ↗(On Diff #188572)

... and let the debugger present ...

Some initial comments.

docs/SourceLevelDebugging.rst
394 ↗(On Diff #188572)

This is the label for the following section, so it needs to stay after your new section.

405 ↗(On Diff #188572)

dbg.value can also record a (typically immediate) value, that is not an SSA register, so you'll need to do some rephrasing.

408 ↗(On Diff #188572)

I think "terminates the effect of any preceding dbg.value" might be better.

410 ↗(On Diff #188572)

value or location

421 ↗(On Diff #188572)

I agree with Adrian, "valuations" is not a term of art and I'd rather not see us introduce it. Maybe s/variable valuations/values of variables/g, or some other similar phrasing.

bjope added inline comments.Feb 27 2019, 11:18 AM
docs/SourceLevelDebugging.rst
462 ↗(On Diff #188572)

Did you intend to leave out dbg.value in this example?
Now it looks identical to the "Finally.." version below which is a little bit confusing.

487 ↗(On Diff #188572)

I do not think that we "must" introduce the undef in this example. We might prefer to do it (because it is a simple solution).
But since there are no other context given here, and the value 0 is a valid state for the variable it isn't wrong to present the variable as having the value 0, up until it is incremented. Well, that is at least my opinion...

Although, if you for example add a function call like this:

define i32 @foo(i32 %bar, i1 %cond) {
  call @llvm.dbg.value(metadata i32 0, metadata !1, metadata !2)
  br i1 %cond, label %truebr, label %falsebr
truebr:
  %tval = add i32 %bar, 1
  call @dbg.value(metadata i32 %tval, metadata !1, metadata !2)
  call @gazonk()
  br label %exit
falsebr:
  %fval = add i32 %bar, 2
  call @dbg.value(metadata i32 %fval, metadata !1, metadata !2)
  call @gazonk()
  br label %exit
exit:
  %merge = phi [ %tval, %truebr ], [ %fval, %falsebr ]
  call @dbg.value(metadata i32 %merge, metadata !1, metadata !2)
  %toret = add i32 %merge, 10
  call @dbg.value(metadata i32 %toret, metadata !1, metadata !2)
  ret i32 %toret
}

then we should get

define i32 @foo(i32 %bar, i1 %cond) {
  call @llvm.dbg.value(metadata i32 0, metadata !1, metadata !2)
  call @llvm.dbg.value(metadata i32 undef, metadata !1, metadata !2)
  call @gazonk()
  %addoper = select i1 %cond, i32 11, i32 12
  %toret = add i32 %merge, %addoper
  call @dbg.value(metadata i32 %toret, metadata !1, metadata !2)
  ret i32 %toret
}

In my example I think it could be wise to mark !1 as undef before the call to gazonk. The value 0 isn't a valid state for !1 at the call. Still not sure if we must do it.

Although I guess the transformation in neither example is done in one single step. We will probably salvage the dbg.value inside the truebrand falsebrblocks along the way while sinking. So it might be likely that we end up with the undef as in your example. However, I can't see that the undef "must" be there given your example (at least it doesn't help me understand why/when it must be added).

jmorse updated this revision to Diff 188877.Mar 1 2019, 4:27 AM
jmorse marked 16 inline comments as done.

Apply review feedback, re-write the dbg.value undef example to better demonstrate the problem at hand.

docs/SourceLevelDebugging.rst
405 ↗(On Diff #188572)

I've added an additional bullet point below, as this is also a difference with dbg.declare.

462 ↗(On Diff #188572)

Curses, you're correct, now updated,

487 ↗(On Diff #188572)

I do not think that we "must" introduce the undef in this example. We might prefer to do it (because it is a simple solution).
[...]
I can't see that the undef "must" be there given your example (at least it doesn't help me understand why/when it must be added).

Sure; in retrospect the example didn't demonstrate the motivating issue at hand, i.e. variable re-ordering. I've refreshed the example using yours as a base, and added an additional source variable (so we now have !1 and !3), the point of the example being that combinations of variable values that did not appear in the unoptimised program should not appear in the optimised one.

Thanks for the worked example, hopefully the new one is more convincing.

dstenb added a subscriber: dstenb.Mar 1 2019, 4:44 AM

Some nitpicks.

docs/SourceLevelDebugging.rst
398 ↗(On Diff #188877)

nit: variable's

408 ↗(On Diff #188877)

nit: variable's

432 ↗(On Diff #188877)

nit: Withholding

474 ↗(On Diff #188877)

%merge -> %bar ?

jmorse updated this revision to Diff 188883.Mar 1 2019, 4:51 AM

Sprinkle some possessive apostrophes, speling, incorrect variable names

jmorse marked 5 inline comments as done.Mar 1 2019, 4:52 AM
jmorse added inline comments.
docs/SourceLevelDebugging.rst
474 ↗(On Diff #188877)

Rats, I did that elsewhere, and had it duplicated in a dbg.value a few lines up too (now fixed).

A few wording notes. Haven't worked through the examples yet.

docs/SourceLevelDebugging.rst
409 ↗(On Diff #188883)

"constant valued"? maybe "Operands can be constants, ..."?

433 ↗(On Diff #188883)

valuations -> values

479 ↗(On Diff #188883)

I'm happy to let you use dbg.value as a shorthand in most places, but when you specifically call them "intrinsics" I think you should use the full name.

vsk added a comment.Mar 1 2019, 4:31 PM

+1, thanks for codifying all of this!

docs/SourceLevelDebugging.rst
403 ↗(On Diff #188883)

variable's

413 ↗(On Diff #188883)

Can this sentence ('Recording ... to a much greater extent') be a bit more direct? It might work to simply move it after the next paragraph, saying e.g. 'Care must be taken to update dbg.value intrinsics when: ...'.

jmorse updated this revision to Diff 189134.Mar 4 2019, 5:57 AM
jmorse marked 5 inline comments as done.

Incorporate further wording and structure feedback

jmorse updated this revision to Diff 189136.Mar 4 2019, 6:02 AM
jmorse marked an inline comment as done.

Re-think opening of paragraph on the risks involved when moving/altering code.

jmorse marked an inline comment as done.Mar 4 2019, 6:05 AM
jmorse added inline comments.
docs/SourceLevelDebugging.rst
413 ↗(On Diff #188883)

I've folded that portion into one paragraph: hopefully now the reader will see:

  • Care is required when [things move],
  • The risk is that developers _observe_ such movements,
  • [middle sentence explaining why this is bad]...

Ping -- AFAIUI we're happy with the meaning in this patch, wording and presentation still needs review?

aprantl accepted this revision.Mar 12 2019, 2:52 PM
This revision is now accepted and ready to land.Mar 12 2019, 2:52 PM
This revision was automatically updated to reflect the committed changes.

Many thanks for the reviews!