This is an archive of the discontinued LLVM Phabricator instance.

Emit one CodeView S_COMPILE3 record per TU rather than per function
ClosedPublic

Authored by amccarth on Nov 1 2016, 3:55 PM.

Details

Reviewers
rnk
Summary

Most of the change is ripple effects in tests.

Diff Detail

Event Timeline

amccarth updated this revision to Diff 76644.Nov 1 2016, 3:55 PM
amccarth retitled this revision from to Emit one CodeView S_COMPILE3 record per TU rather than per function.
amccarth updated this object.
amccarth added a reviewer: rnk.
amccarth added a subscriber: llvm-commits.
rnk edited edge metadata.Nov 2 2016, 9:18 AM

Looks good, mostly a bunch of comments about how to make the tests less fragile.

test/DebugInfo/COFF/asm.ll
29

I think this test is really supposed to be about the line table. I think we should keep the .cv_loc directive matching above, but delete all this .debug$S directive matching to make the test less fragile.

See the FIXME comment below and http://llvm.org/PR18679. I think that explains why this test is here.

69–74

Let's delete the relocation matching. They aren't interesting in this test.

134–137

Ditto, I don't think we need x64 .debug$S assembly matching, just line table matching below.

test/DebugInfo/COFF/multifile.ll
35

This test is also entirely concerned with what happens to the line table when you use #line inside a function. I think we can do the same here: delete the .debug$S assembly matching portion and relocation entries.

The FilenameSegment checks below are the interesting part.

test/DebugInfo/COFF/multifunction.ll
58

This is supposed to test how we emit one symbol subsection per function, so I think it's worth matching the compile info subsection.

The names F1_START and F1_END were intended to refer to the symbol subsection for x, but this is actually the compile info subection now. Let's use F1_START / F1_END in place of SUBSEC_START / SUBSEC_END and use COMPILE_START / COMPILE_END here.

146–158

I don't think the relocation entries are interesting anymore, let's delete them.

test/DebugInfo/COFF/simple.ll
28

Let's update the names to COMPILE_START/END and use F1_START/END below.

rnk added a comment.Nov 2 2016, 9:21 AM

I was going to say, the old line table tests that Timur added when CodeView support was new (asm.ll, simple.ll, multfile.ll, and multifunction.ll in rL200340) overfit our output because we didn't have a lot of confidence in our understanding of the format.

amccarth marked 6 inline comments as done.Nov 2 2016, 2:01 PM
amccarth added inline comments.
test/DebugInfo/COFF/multifunction.ll
58

Yeah, duh. I realized that mistake earlier, but promptly forgot about it.

amccarth updated this revision to Diff 76783.Nov 2 2016, 2:02 PM
amccarth edited edge metadata.
amccarth marked an inline comment as done.

Made tests less fragile per Reid's suggestions.

rnk accepted this revision.Nov 2 2016, 2:20 PM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Nov 2 2016, 2:20 PM
amccarth closed this revision.Mar 14 2017, 4:39 PM

Committed in r285862