Page MenuHomePhabricator

[Debugify] Diagnose mis-sized dbg.values
ClosedPublic

Authored by vsk on Jun 20 2018, 5:31 PM.

Details

Summary

Report an error in -check-debugify when the size of a dbg.value operand
doesn't match up with the size of the variable it describes.

Eventually this check should be moved into the IR verifier. For the
moment, it's useful to include the check in -check-debugify as a means
of catching regressions and finding existing bugs.

Here are some instances of bugs the new check finds in the -O2 pipeline:

  1. A float is used where a double is expected:

ERROR: dbg.value operand has size 32, but its variable has size 64:
call void @llvm.dbg.value(metadata float %expf, metadata !12, metadata
!DIExpression()), !dbg !15

  1. An i8 is used where an i32 is expected:

ERROR: dbg.value operand has size 8, but its variable has size 32:
call void @llvm.dbg.value(metadata i8 %t4, metadata !14, metadata
!DIExpression()), !dbg !24

  1. A <4 x i32> is used where something twice as large is expected

(perhaps a <4 x i64>, I haven't double-checked):

ERROR: dbg.value operand has size 128, but its variable has size 256:
call void @llvm.dbg.value(metadata <4 x i32> %4, metadata !40, metadata
!DIExpression()), !dbg !95

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Jun 20 2018, 5:31 PM
davide added a subscriber: davide.Jun 20 2018, 6:22 PM
vsk updated this revision to Diff 152538.Jun 22 2018, 1:49 PM
vsk edited the summary of this revision. (Show Details)
  • Simplify the commit description.
  • Use a helper function to clean up the code.
  • Remove the exception for integer values which are larger than their described variables, as it's not clear whether debuggers can always truncate to display the right value.
vsk updated this revision to Diff 152539.Jun 22 2018, 1:54 PM
  • Mark a variable as missing only if the mis-sized diagnostic fires.

I remember there being a difference of opinion about the "correct" size of an 80-bit float in a 128-bit container. How would that case be handled here?

vsk added a comment.Jun 22 2018, 2:40 PM

I remember there being a difference of opinion about the "correct" size of an 80-bit float in a 128-bit container. How would that case be handled here?

That's a good question. Currently, we emit IR that would not trigger the diagnostic this patch introduces (https://godbolt.org/g/7DXyJy):

call void @llvm.dbg.value(metadata x86_fp80 %10, metadata !14, metadata !DIExpression())
!10 = !DIBasicType(name: "long double", size: 128, encoding: DW_ATE_float)
!14 = !DILocalVariable(name: "x", scope: !7, file: !1, line: 2, type: !10)

getSizeInBits() reports back 128 for an x86_fp80.

Hm. DataLayout is willing to report back one of three possible sizes: the size of a value; the size written by a store; and the stride of an array. getTypeAllocSize() returns the third of these, which would include alignment padding as needed.
Is that really the value size we expect from dbg.value?

vsk planned changes to this revision.Jun 26 2018, 12:38 PM

I think I need to bring back the exception for integers. Adrian pointed out (in https://reviews.llvm.org/D48331) that DI variables with a smaller size than a dbg.value operand implicitly use the lower bits. This works for the integer case.

Hm. DataLayout is willing to report back one of three possible sizes: the size of a value; the size written by a store; and the stride of an array. getTypeAllocSize() returns the third of these, which would include alignment padding as needed.
Is that really the value size we expect from dbg.value?

Oh I see, thanks for pointing this distinction out.

In the example I shared above, at -O0, the stack slot for the x86_fp80 is 128 bits in size (https://godbolt.org/g/B89MwC). LLVM generates a ST_FP80m to convert the fp80 into "double extended-precision floating-point format", which it then stores into this slot. So I think 128 bits makes sense as the size: anything else could confuse the debugger about what's on the stack. I'm not sure how we'd support debugging fp80's which are "in" the FPU, i.e which are not spilled. FWIW, we tend to spill (-O2, https://godbolt.org/g/uwpMYY).

In general then, I think getAllocSizeInBits is the appropriate size to use, because it captures the fact that conversions may be needed to place the value in a stack slot.

vsk updated this revision to Diff 152952.Jun 26 2018, 1:56 PM
  • Bring back the exception for an integer dbg.value operand which is wider than the DILocalVariable it's associated with. We're relying on the debugger being able to simply use the low bits of the integer in this scenario in a few different places, and it appears to work.
probinson accepted this revision.Jun 26 2018, 2:59 PM

A couple of naming suggestions but LGTM.

tools/opt/Debugify.cpp
43 ↗(On Diff #152952)

Given the discussion about sizes, maybe getAllocSizeInBits()?

187 ↗(On Diff #152952)

Maybe "ValueSizeInBits" and "FragmentSizeInBits" to be more descriptive than judgmental.

This revision is now accepted and ready to land.Jun 26 2018, 2:59 PM
This revision was automatically updated to reflect the committed changes.