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

Remove restriction that no loop unrolling is done when breaks are in the loop #1518

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

karoliineh
Copy link
Member

@karoliineh karoliineh commented Jun 19, 2024

In SV-COMP no-overflow tasks there are cases similar to the following example:

int counter = 0;
int main() {
    int  X = __VERIFIER_nondet_int();
    long long x = 0;

    while (counter++<10) {
        ...
        if (!(x <= X))
            break;
        ...
    }
    ...

Where we do not detect loops with fixed iterations due to the restriction:

There is one single break in an if statement where the condition compares a local variable v to a constant e.

This PR removes this restriction, as reaching these breaks should make the loop iterations smaller anyway.

However, all these tasks also have the increment counter++ in the loop condition, which we do not handle and which is very difficult to handle using only the syntax, as these statements are picked into pieces by CIL:

int counter = 0;
int main() {
    int  X = __VERIFIER_nondet_int();
    long long x = 0;

    tmp = counter;
    counter = counter + 1;
    while (tmp<10) {
        ...
        if (!(x <= X))
            break;
        ...
        tmp = counter;
        counter = counter + 1;
    }
    ...

Thus, I opted for another assumption that when we find a fixed loop and its start but cannot detect the increment within the loop, we assume the increment is 1.

This solved the tasks with the bounds being 5 and 10, but for bounds 20 and 50, I also had to apply the hack introduced in #1516 to just use the fixed loop size that was found by the heuristics if there are less than 10 loops in the program (the only stat I could find/access to estimate the size of the program (being small)).

@karoliineh karoliineh added sv-comp SV-COMP (analyses, results), witnesses precision labels Jun 19, 2024
@karoliineh karoliineh added this to the SV-COMP 2025 milestone Jun 19, 2024
@karoliineh karoliineh self-assigned this Sep 28, 2024
@karoliineh karoliineh marked this pull request as ready for review October 10, 2024 19:42
@@ -31,34 +31,25 @@ class checkNoBreakVisitor = object

end

let checkNoBreakStmt stmt =
let visitor = new checkNoBreakVisitor in
Copy link
Member

Choose a reason for hiding this comment

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

checkNoBreakVisitor itself is also unused now.

let checkNoBreakBlock block =
let visitor = new checkNoBreakVisitor in
ignore @@ visitCilBlock visitor block

class findBreakVisitor(compOption: exp option ref) = object
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this visitor now, I understand why it didn't want multiple breaks. It's main goal is to actually find the comparison for the loop, which will be an if with a break inside.
By only allowing one such break it guaranteed that the result is unambiguous: there couldn't be multiple such comparisons found.

Now, if there are multiple matching ifs with breaks inside, this will still find the first one because compOption is only assigned when it's None, so that can happen at most once.
Is the loop condition always in the first matching one though? If so, then why did the old version have a problem at all? Or maybe there are loops where the condition isn't actually the first one?

@karoliineh karoliineh modified the milestones: SV-COMP 2025, SV-COMP 2026 Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
precision sv-comp SV-COMP (analyses, results), witnesses
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants