Page MenuHomePhabricator

Add Targeted LiveDebugValues Tests That Exercise Specific Expected Behaviours
ClosedPublic

Authored by TWeaver on Jan 10 2020, 8:42 AM.

Details

Summary

Within in this patch are 22 distinct test cases that exercise specific behaviours within the livedebug values pass.

After all the work that's been ongoing with the livedebugvalues pass its become clear that coverage for the expected behaviour was previously lacking.

Rather than risk there not being coverage for said cases I felt it was time to get them added to reduce the risk of a regression in the future.

The aim is to cover behaviour for a standard case with no moves or clobbers, cases with clobbers and cases with moves.

A mix of sequential, loop and diamond control flows are considered in the flavours described above.

happy reviewing.

Diff Detail

Event Timeline

TWeaver created this revision.Jan 10 2020, 8:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2020, 8:42 AM
vsk added a reviewer: jmorse.Jan 10 2020, 9:53 AM

This is great, thanks for doing this.

vsk added inline comments.Jan 10 2020, 10:19 AM
llvm/test/DebugInfo/MIR/X86/livedebugvalues_basic_diamond_one_move.mir
18

Have you considered running FileCheck with -implicit-check-not=DBG_VALUE? That might be less brittle (e.g. it'd survive small changes to how DBG_VALUE is printed out).

llvm/test/DebugInfo/MIR/X86/livedebugvalues_basic_loop.mir
6

nit, it's -> its

llvm/test/DebugInfo/MIR/X86/livedebugvalues_bb_to_bb.mir
5

nit, propogate -> propagate

6

This one isn't a loop, it seems. Just a chain of jumps?

llvm/test/DebugInfo/MIR/X86/livedebugvalues_bb_to_bb_clobbered.mir
11

Is the interesting thing here the undef DBG_VALUE? Could this be:

; CHECK-LABEL: bb.1.bb1:
; CHECK:       DBG_VALUE $ebx, $noreg, !16
; CHECK:       $ebx = MOV32ri 0
; CHECK:       DBG_VALUE $noreg, $noreg, !16

?

If so it might be another argument to have an implicit check-not for DBG_VALUE in these tests.

Thanks for this!

llvm/test/DebugInfo/MIR/X86/livedebugvalues_basic_diamond.mir
31

(Nit) Please update the version of the compiler.

TWeaver marked 6 inline comments as done.Jan 13 2020, 9:26 AM

thank you all for your valuable time.

added implicit-check-not=DBG_VALUE to all tests.
added extra coverage missing from one of the tests.
removed some trailing whitespace.
fixed few typo nits.

llvm/test/DebugInfo/MIR/X86/livedebugvalues_basic_diamond.mir
31

Hi Dj,

thanks for your review.

All of the tests have clang 8.0 as their versions as they're all based on some samples written about a year ago now.

Are you wanting me to change the strings in the debug info to the latest version of clang or regenerate these samples?

the samples are hand modified from one base sample that was trimmed down from a larger test case.

llvm/test/DebugInfo/MIR/X86/livedebugvalues_basic_diamond_one_move.mir
18

Hi VSK,

I wasn't fully aware of the -implicit-check feature of FileCheck until you mentioned it.

I've added it to all tests and it actually flushed out a lack of coverage in one of the tests.

thank you very much for pointing this out. Some of the test cases have been significantly reduced as a result.

TWeaver updated this revision to Diff 237712.Jan 13 2020, 9:26 AM
TWeaver marked an inline comment as done.

address feedback

aprantl accepted this revision.Jan 13 2020, 9:29 AM

Very nice!

This revision is now accepted and ready to land.Jan 13 2020, 9:29 AM

Very nice!

llvm/test/DebugInfo/MIR/X86/livedebugvalues_basic_diamond.mir
4

Wording: "debug value intrinsics" are call llvm.dbg.value in LLVM IR. In MIR they are DBG_VALUE *instructions*.

llvm/test/DebugInfo/MIR/X86/livedebugvalues_loop_within_loop_outer_moved.mir
4

propagated

vsk accepted this revision.Jan 13 2020, 9:37 AM

LGTM as well.

TWeaver marked 2 inline comments as done.Jan 13 2020, 10:33 AM

addressed further feedback

TWeaver updated this revision to Diff 237725.Jan 13 2020, 10:35 AM

updated diff

I'll land this tomorrow as I'm off home now and don't want to risk getting buildbot emails all night.

thanks for the reviews!

djtodoro added inline comments.Jan 13 2020, 11:27 PM
llvm/test/DebugInfo/MIR/X86/livedebugvalues_basic_diamond.mir
31

Hi Tom,

Yes I just wanted the "clang version 10" as a string field there, since we are testing the LLVM 10 and somewhere outhere might be a testing tool parsing the metadata...
I mean, it is a very minor thing, other than that, everything looks fine! Thanks!

This revision was automatically updated to reflect the committed changes.
vsk added a comment.Jan 16 2020, 5:05 PM

@TWeaver this is still causing a failure on some Darwin bots:

error: /Volumes/JenkinsData/workspace/apple-clang-stage1-configure-R_master/llvm-project/llvm/test/DebugInfo/MIR/X86/livedebugvalues_loop_within_loop_clobbered.mir:40:39: expected end of metadata node
  !12 = distinct !DISubprogram(name: "bb_to_bb", linkageName: "bb_to_bb", scope: !1, file: !1, line: 6, type: !13, scopeLine: 6, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !15)

Could you please take a look?