Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Coverage] getMaxBitmapSize: Scan max(BitmapIdx) instead of the last Decision #78963

Merged
merged 5 commits into from
Jan 23, 2024

Conversation

chapuni
Copy link
Contributor

@chapuni chapuni commented Jan 22, 2024

In CoverageMapping.cpp:getMaxBitmapSize(),
this assumed that the last Decision has the maxmum BitmapIdx.

Let it scan max(BitmapIdx).

Note that <= is used insted of <, because BitmapIdx == 0 is valid
and MaxbitmapID is unsigned. BitmapIdx is unique in the record.

Fixes #78922

Note that `<=` is used insted of `<`, because `BitmapIdx == 0` is valid
and `MaxbitmapID` is `unsigned`. `BitmapIdx` is unique in the record.
@chapuni
Copy link
Contributor Author

chapuni commented Jan 22, 2024

Let me push the testcase in advance of the fix, to confirm failure.

@chapuni
Copy link
Contributor Author

chapuni commented Jan 22, 2024

The record of this testcase is:

sub:
  Decision,File 0, 5:11 -> 5:59 = M:1, C:4
  Branch,File 0, 5:12 -> 5:20 = #5, ((#0 - #1) - #5) [1,3,2]
  Branch,File 0, 5:24 -> 5:32 = #6, (#5 - #6) [3,0,2]
  Branch,File 0, 5:38 -> 5:46 = #7, (#4 - #7) [2,4,0]
  Branch,File 0, 5:50 -> 5:58 = #8, (#7 - #8) [4,0,0]
  Decision,File 1, 1:23 -> 1:47 = M:0, C:2
  Branch,File 1, 1:23 -> 1:33 = #2, (#0 - #2) [1,2,0]
  Branch,File 1, 1:37 -> 1:47 = #3, (#2 - #3) [2,0,0]

The current implementation picks up M:0 C:2.

@chapuni
Copy link
Contributor Author

chapuni commented Jan 22, 2024

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:232: Expected<BitVector> llvm::coverage::CounterMappingContext::evaluateBitmap(const CounterMappingRegion *) const: Assertion `ID + SizeInBytes <= BitmapBytes.size() && "BitmapBytes overrun"' failed. in https://buildkite.com/llvm-project/github-pull-requests/builds/31563

Copy link

@ornata ornata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chapuni chapuni merged commit c193bb7 into llvm:main Jan 23, 2024
3 of 4 checks passed
@chapuni
Copy link
Contributor Author

chapuni commented Jan 23, 2024

https://lab.llvm.org/buildbot/#/builders/16/builds/60018

error: failed to load coverage: 'llvm/test/tools/llvm-cov/Inputs/mcdc-maxbs.o': profile uses zlib compression but the profile reader was built without zlib support

I'll rework.

@ornata
Copy link

ornata commented Jan 23, 2024

Will the new test fail when the profile reader is built with zlib support? :(

@chapuni
Copy link
Contributor Author

chapuni commented Jan 23, 2024

Should be fixed in d386c40

@ornata It was failing on zlib-unsupported hosts since I enabled zlib locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Coverage][MC/DC] Wrong BitmapIdx calculation in getMaxBitmapSize
3 participants