ExprEngine: Handle pointer assignments in loops without bailout by jubnzv · Pull Request #2949 · cppcheck-opensource/cppcheck

jubnzv · 2020-12-14T17:19:20Z

The following source code:

void foo(int *a)
{
  while (1)
    *a = 42;
  *a == 42;
}

Before this commit generates a redundant "bailout" value on the assigment:
15:12: 0:memory:{a=($2,[$1],[:]=?,[0]=bailout,[0]=42)}

After this commit:
15:12: 0:memory:{a=($2,[$1],[:]=?,[0]=42)}

The added tests are redundant and can't detect this error, but they increase the number of similar cases to handle possible regressions in the future.

The following source code: ``` void foo(int *a) { while (1) *a = 42; *a == 42; } ``` Before this commit generates a redundant "bailout" value on the assigment: 15:12: 0:memory:{a=($2,[$1],[:]=?,[0]=bailout,[0]=42)} After this commit: 15:12: 0:memory:{a=($2,[$1],[:]=?,[0]=42)} The added tests are redundant and can't detect this error, but they increase the number of similar cases to handle possible regressions in the future.

danmar

great work! It is not easy to understand this code.

jubnzv · 2020-12-15T05:09:15Z

I'm not sure how this CI error can be related to my commits. Other github actions are green.

danmar · 2020-12-15T06:12:40Z

sorry I caused that CI error .. I've tried to fix it now with ecfabbc

danmar · 2020-12-15T06:28:13Z

After some more thinking.. I think it makes sense to assign a bailout value. I think we want to have something like this:

    while (1) {

        int x = 24;
0:memory:{a=($2,[$1],[:]=?,[0]=bailout) x=24}

        *a = 42;
0:memory:{a=($2,[$1],[:]=?,[0]=42) x=24}

    }

could you look at ArrayValue::assign.. if there is a matching index then it should be removed.

danmar

I would suggest that we drop this work path.. I changed my mind and don't think assigning a BailoutValue is bad. The assignment happens before the loop is executed.

danmar · 2020-12-15T06:41:04Z

+                        changedVariables.insert(varid);
+                        auto oldValue = data.getValue(varid, nullptr, nullptr);
+                        if (oldValue && oldValue->isUninit())
+                            call(data.callbacks, varToken, oldValue, &data);


We are not trying to execute the loop yet. We should call callbacks later when the loop is executed. We also do not know if the variable will still have the oldValue when this code is reached (it will if this is unconditionally executed).

Then why do we call them in variable assignments here?

danmar · 2020-12-15T06:44:33Z

+                            // Unknown pointer, array or struct
+                            auto arrayValue = std::dynamic_pointer_cast<ExprEngine::ArrayValue>(val);
+                            assert(arrayValue);
+                            data.assignValue(tok2, varid, val);


as far as I see this assignment is redundant.. it assigns the same value as we already have.

jubnzv · 2020-12-15T16:13:52Z

The assignment happens before the loop is executed.

That is, you suggest to replace bailout values later, when we assign a new value?

But how do we know that the loop has been executed? Do we want generate two equations after the loop, like for conditional statements?

For example:

int foo(int *arr) {
// memory:{arr=($2,[$1],[:]=?,[0]=bailout) a=2 b=$3}
    int b = 24;
    while (1) {
        *arr = 0;
// memory:{arr=($2,[$1],[:]=?,[0]=0) a=2 b=24}
    }
// memory:{arr=($2,[$1],[:]=?,[0]=0) b=24} constraints:{1}
// memory:{arr=($3,[$1],[:]=?,[0]=bailout,) b=24} constraints:{(1)==(0)}
}

Something like this, right?

danmar · 2020-12-15T19:18:36Z

I expanded a bit..

void foo(int *a, int x) {

    *a = 12;
0:memory:{a=($2,[$1],[:]=?,[0]=12) x=$3}

    while (x < 10) {

        x = 24;
0:memory:{a=($2,[$1],[:]=?,[0]=bailout) x=24} constraints:{$3 < 10}

        *a = 42;
0:memory:{a=($2,[$1],[:]=?,[0]=42) x=24} constraints:{$3 < 10}
    }

0:memory:{a=($2,[$1],[:]=?,[0]=12) x=$3} constraints:{$3 >= 10}
0:memory:{a=($2,[$1],[:]=?,[0]=42) x=24} constraints:{$3 < 10}
    return *a;
}

EDIT: I thought it was unfortunate that your code before the while loop had a bailout value. I guess your example is right but it's not clear where the "bailout" after the loop comes from.

danmar · 2020-12-15T21:30:56Z

One more thing.. if the loop condition is 1 then we should not split the execution path. The execution will obviously not skip the loop then. Please see how "alwaysTrue" is calculated in the handling of if.

danmar · 2020-12-15T21:32:57Z

It feels like we can find more and more issues :-) it's never ending.

I've tried to look into merging "same" execution paths this evening to speedup analysis.. I have a work in progress will see if I can finish that.

danmar · 2020-12-16T08:57:02Z

Please see how "alwaysTrue" is calculated in the handling of if.

Maybe we should create helper functions isAlwaysTrue and isAlwaysFalse. It is misleading that isTrue returns true whenever the value could be true, not sure how to rename it to make it less misleading..

jubnzv · 2020-12-16T16:33:49Z

Maybe we should create helper functions isAlwaysTrue and isAlwaysFalse

Yes, I implemented them while working on condition branches for the loops right now. This will take some more time.

jubnzv changed the title ~~ExprEngine: Remove redundant bailout value from pointer assignment~~ ExprEngine: Remove redundant bailout value from the pointer assignment Dec 14, 2020

danmar reviewed Dec 14, 2020

View reviewed changes

Comment thread lib/exprengine.cpp Outdated

Comment thread lib/exprengine.cpp

Comment thread test/testexprengine.cpp Outdated

jubnzv added 2 commits December 15, 2020 06:54

testexprengine: Fix code

040837b

ExprEngine: Handle pointers to a known variable

c39a860

jubnzv commented Dec 15, 2020

View reviewed changes

Comment thread lib/exprengine.cpp

jubnzv changed the title ~~ExprEngine: Remove redundant bailout value from the pointer assignment~~ ExprEngine: Handle pointer assignments in loops without bailout Dec 15, 2020

danmar reviewed Dec 15, 2020

View reviewed changes

jubnzv closed this Dec 16, 2020

Conversation

jubnzv commented Dec 14, 2020

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jubnzv commented Dec 15, 2020

Uh oh!

danmar commented Dec 15, 2020

Uh oh!

danmar commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

danmar Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

jubnzv Dec 16, 2020

Choose a reason for hiding this comment

Uh oh!

danmar Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

jubnzv commented Dec 15, 2020

Uh oh!

danmar commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmar commented Dec 15, 2020

Uh oh!

danmar commented Dec 15, 2020

Uh oh!

danmar commented Dec 16, 2020

Uh oh!

jubnzv commented Dec 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danmar commented Dec 15, 2020 •

edited

Loading

danmar commented Dec 15, 2020 •

edited

Loading