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

[TG-1404] Bugfix incomplete coverage in virtual method dispatch #1755

Conversation

romainbrenguier
Copy link
Contributor

Before this PR block with an assume false was generated.
This was leading to report of incomplete coverage.
Instead of this we generate assumption that the class identifier are
different from the non-loaded type all in the same block.

@smowton
Copy link
Contributor

smowton commented Jan 19, 2018

Could you give an example of what's happening? This sounds like it should amount to the same assumption -- i.e. that we cannot possibly get to this virtual callsite.

@romainbrenguier
Copy link
Contributor Author

@smowton

Could you give an example of what's happening?

Yes I will try to add some regression tests to this PR

This sounds like it should amount to the same assumption -- i.e. that we cannot possibly get to this virtual callsite.

Yes the assumptions are the same. The difference is we don't generate extra blocks for this unreachable callsites. This makes a difference in the reported coverage with --cover location.

@peterschrammel peterschrammel changed the title [TG-1404] Bugfix incomplet coverage in virtual method dispatch [TG-1404] Bugfix incomplete coverage in virtual method dispatch Jan 21, 2018
@romainbrenguier romainbrenguier force-pushed the bugfix/coverage-dispatch#TG-1404 branch 2 times, most recently from 94d7ac4 to e1cbc33 Compare January 25, 2018 12:29
Before block with an assume false was generated.
This was leading to report of incomplete coverage.
Instead of we generate assumption that the class identifier are
different from the non-loaded type all in the same block.
Small variable names changes to make them more descriptive.
@romainbrenguier romainbrenguier force-pushed the bugfix/coverage-dispatch#TG-1404 branch from e1cbc33 to f8b5d7d Compare January 29, 2018 15:57
@romainbrenguier
Copy link
Contributor Author

@mgudemann This PR and #1731 have some conflicts, so I'm not sure which one should be merged first ?
I have a version of this branch rebased on @mgudemann 's PR https://github.com/romainbrenguier/cbmc/tree/bugfix/all_resolved_calls1931_coverage_dispatch1404

@mgudemann
Copy link
Contributor

@romainbrenguier is this relevant or was it obsoleted by #1830

@romainbrenguier
Copy link
Contributor Author

@mgudemann #1830 fixes the bug whose effect we were trying to minimize here so the other PR is more relevant. However this change could still be made, it can remove a few instruction from the goto-programs. I just don't know if it's worth it.

@romainbrenguier
Copy link
Contributor Author

The main coverage issue is fixed, and this changes are no longer necessary so I'm closing this PR.

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

Successfully merging this pull request may close these issues.

4 participants