Page MenuHomePhabricator

[Coverage] : introduce class hierarchy (funcRecordReader) to support multiple versions of coverage data
ClosedPublic

Authored by davidxl on Jan 12 2016, 10:59 PM.

Details

Reviewers
vsk
Summary

With the planned size reduction change, the coverage format version is expected to be bumped up. This patch adds necessary support such that backward compatibility can be kept with maximal code sharing. Reading different versions of coverage data just requires instantiating the reader according to the version.

No functional change is intended.

Diff Detail

Event Timeline

davidxl updated this revision to Diff 44711.Jan 12 2016, 10:59 PM
davidxl retitled this revision from to [Coverage] : introduce class hierarchy (funcRecordReader) to support multiple versions of coverage data.
davidxl updated this object.
davidxl added a reviewer: vsk.
davidxl added a subscriber: llvm-commits.
vsk edited edge metadata.Jan 13 2016, 10:46 AM

Thanks! Comments inline --

lib/ProfileData/CoverageMappingReader.cpp
309–341

This is a nice way to reduce the amount of template parameterization needed to pass around a reader object. However, since this class only used in this c++ file, maybe it would be better to get rid of this parent class and drop the 'Versioned' from 'VersionedCovMapFuncRecordReader'. Later, you can do auto Reader = make_unique(...).

433

I'm not comfortable with *&Buf as an input-output parameter. I think readFunctionRecords should have direct access to the buffer, without needing it passed in. Could you pass StringRef Data into the Reader constructor?

435

This should be lifted out of the loop. When the version bump happens, the line if (Version > coverage::CoverageMappingCurrentVersion) will start causing strange crashes :).

davidxl added inline comments.Jan 13 2016, 11:05 AM
lib/ProfileData/CoverageMappingReader.cpp
309–341

Auto does not work in this case -- because version value is read at reader runtime and we do not know what version's reader to instantiate at compile time. The parent class is introduced to hide the details.

433

In the coverage format design, there is no whole program level header, but instead the coverage map for the program is a concatination of module level coverage maps -- each one of the module level coverage data has a header. (On the other hand, the first module's cov map header can be considered the whole program's covmap header).

The CovMapFuncRecordReader is actually FuncRecord reader for a module. If we pass Data to the reader, we still need to keep track of current Data offset which is basically the same as 'Buf' here. Before the call to readFunctionRecords, 'Buf' points to the start of a module's coverage data.

435

Good idea -- since we don't expect Coverage Version to be different across different modules, lift it outside the loop can simplify the code.

Also note that -- the format of the covmap header format is pretty much 'locked' -- at least the location of the 'version' field -- no matter what version is in the future, it should not change.

davidxl updated this revision to Diff 44771.Jan 13 2016, 11:19 AM
davidxl edited edge metadata.

Address vsk's comment:

Hoist header read out of loop and add assert version is consistent across different modules.

vsk added a comment.Jan 13 2016, 1:11 PM

Thanks for the explanation, this lgtm.

lib/ProfileData/CoverageMappingReader.cpp
309–341

Got it.

433

I see, I misread this as a parser for a single module's worth of coverage mapping information.

vsk accepted this revision.Feb 24 2016, 6:23 PM
vsk edited edge metadata.

r257708

This revision is now accepted and ready to land.Feb 24 2016, 6:23 PM
vsk closed this revision.Feb 24 2016, 6:24 PM