This is an archive of the discontinued LLVM Phabricator instance.

[llvm-cov] Fix illegal cast from uint64_t to int64_t
ClosedPublic

Authored by sajjadm on Nov 13 2019, 12:18 PM.

Details

Summary

Counters are stored as uint64_t in the coverage mapping, but
exporting in JSON requires signed integers. Clamp the values to the
smaller range to make the conversion safe.

Diff Detail

Event Timeline

sajjadm created this revision.Nov 13 2019, 12:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 13 2019, 12:18 PM

Discovered this because we were getting negative numbers in some of our coverage data when we had counter overflows.

vsk added a comment.Nov 13 2019, 1:26 PM

Thanks, lgtm with a comment.

llvm/tools/llvm-cov/CoverageExporterJson.cpp
65

Please leave a comment explaining why the format makes clamping necessary.

sajjadm updated this revision to Diff 229205.Nov 13 2019, 4:39 PM
  • Added comment to clamp function.
sajjadm marked an inline comment as done.Nov 13 2019, 4:40 PM
Dor1s accepted this revision.Nov 14 2019, 7:27 AM

Ha, nice! LGTM. Is there any test you could easily update or add to reproduce this corner case?

llvm/tools/llvm-cov/CoverageExporterJson.cpp
72

nit: can return std::min(u, std::numeric_limits<int64_t>::max())

This revision is now accepted and ready to land.Nov 14 2019, 7:27 AM

Ha, nice! LGTM. Is there any test you could easily update or add to reproduce this corner case?

I tried to write a loop that would execute INT64_MAX + 10 times, but that takes way too long. Not sure how to test it outside of the case where we have counter overflows.

sajjadm updated this revision to Diff 229414.EditedNov 14 2019, 2:37 PM

Changed to using std::min

sajjadm marked 2 inline comments as done.Nov 14 2019, 2:39 PM
sajjadm added inline comments.
llvm/tools/llvm-cov/CoverageExporterJson.cpp
72

FYI we still needed the static_cast to make template deduction work

sajjadm marked an inline comment as done.Nov 14 2019, 2:39 PM
sajjadm retitled this revision from Fix illegal cast from uint64_t to int64_t to [llvm-cov] Fix illegal cast from uint64_t to int64_t.Nov 14 2019, 3:24 PM
This revision was automatically updated to reflect the committed changes.