Fix #14717 (FP: same exprId for different expressions leads to wrong redundantAssignment)

[danmar]

No description provided.

[danmar]

The problem I saw was that followAllReferences() looked at ternary operator operands. The second operand was "1" for both expressions and then the parent "/" got the same exprId. I have the feeling we have tweaked some handling for ternary operators recently. But very strange that bisect was pointing at other commits..

[firewave]

But very strange that bisect was pointing at other commits..

As mentioned in the comment Visual Studio builds are non-deterministic and somehow Linux builds have consistent results but they might flip with each commit. So regular bisecting is completely impossible.

[firewave]

The fix feels too intrusive for such an subtle issue.

Looking at followingAllReferencesInternal() there is std::set<ReferenceToken, ReferenceTokenLess> result;. As that contains a pointer I wonder if this is causing the issue as the order might be unstable. And as all other cases only return a single result this is the only case where it could differ. I will give this a spin in Visual Studio later.

[firewave]

I have the feeling we have tweaked some handling for ternary operators recently

It wasn't that recently - the issue was introduced in the 2.19 dev cycle.

[firewave]

And this doesn't seem to address the potentially duplicated exprId pointed out in https://trac.cppcheck.net/ticket/14717#comment:12.

[danmar]

And this doesn't seem to address the potentially duplicated exprId pointed out in https://trac.cppcheck.net/ticket/14717#comment:12.

I am not sure I understand the debug output in comment:12 but doesn't it say that the operand1 for the > token is 1, And this expression 1 will get exprId 3. It's OK that all expressions 1 everywhere gets the same exprId 3 right?

[firewave]

Looking at followingAllReferencesInternal() there is std::set<ReferenceToken, ReferenceTokenLess> result;. As that contains a pointer I wonder if this is causing the issue as the order might be unstable.

That is actually an issue.

correct:

op1 - b - 2
op2 - 1 - 3
op2 - [ - 5
? - astParent: = - key.operand1: 2 (b) - key.operand2: 5 (?) - not found - exprId: 9

incorrect:

op1 - b - 2
op2 - [ - 5
op2 - 1 - 3
? - astParent: = - key.operand1: 2 (b) - key.operand2: 3 (?) - not found - exprId: 9

[firewave]

That is actually an issue.

setParentExprId() breaks after the first results entry from followAllReferences() and since that doesn't have a stable order when it returns more than one results that will lead to the non-determinism.

That needs to be fixed inside followAllReferences() by using stable sorting via ReferenceTokenLess (using a pointer for sorting is always bad). The location comes to mind assomething fixed but I have no idea if that would work out with macros.

[firewave]

And this doesn't seem to address the potentially duplicated exprId pointed out in https://trac.cppcheck.net/ticket/14717#comment:12.

That appears to be a non-issue. The logging was just confusing as I was printing the op the exprId was looked up from and not the actual token belonging to the exprId.

[danmar]

The location comes to mind assomething fixed but I have no idea if that would work out with macros.

I am not sure if the Token::index() has been assigned yet. but if it has that would be more stable. The location will not work well there are many simplifications that will not give tokens unique locations.

No matter if we make the order fixed or not we need to ensure that we don't take the expression ids of the ternary operator operands..

When a ternary operator expression is x?y:z then it seems OK to me that followAllReferences can return both y and z. But neither of these results are OK to use here in this context when setting the expression id for the ? or its parent. An alternative fix could be to skip ref.token if its parent is a :

[firewave]

No matter if we make the order fixed or not we need to ensure that we don't take the expression ids of the ternary operator operands..

As a first step we need to make the order stable regardless of the ternary issues.

[danmar]

An alternative fix could be to skip ref.token if its parent is a :

I don't think that suggestion would work well.

As a first step we need to make the order stable regardless of the ternary issues.

It's not clear to me why? the important thing is that we pick the proper ref.token right? if the ternary operator says x?y:z then picking y or z are both wrong. There is no strong reason to pick the first result from followAllReferences.

[danmar]

As a first step we need to make the order stable regardless of the ternary issues.

My intention with my fix is that the order does not matter. My goal was to pick the expression id from the same ref token no matter if refs are order (ref1,ref2) or (ref2,ref1).

[firewave]

My intention with my fix is that the order does not matter. My goal was to pick the expression id from the same ref token no matter if refs are order (ref1,ref2) or (ref2,ref1).

There are two distinct issues.

The non-determinism affects all callers and you hide the underlying issue by introducing code which gets rid of one known symptom (we don't even know if there are any other issues unless you fix it (we should probably add a debug message to sniff out cases in daca) which is extremely bad and short-sighted - in that case I would prefer to have the code that might at least show the issue. As seen by the test this does not even manifests consistently on Linux so we don't even know what other issue we might have by keeping the inconsistent behavior.

[danmar]

My motivation why I feel this will be robust:

The last thing that happens in Tokenizer::simplifyTokenList1() is that list.front()->assignIndexes() is called. This will give all tokens a determinate index value. The intention of Token::index() is that it will be a counter value that checkers can rely on - it should be reliable otherwise various checkers will not be reliable. So after that call we should be able to count on that.

As far as I can tell we do not call followAllReferences neither directly or indirectly before assignIndexes() has been called.

I am not sure what test we should write for this. followAllReferences is used quite a lot in the tests so there is a basic level of testing.

[pfultz2]

When a ternary operator expression is x?y:z then it seems OK to me that followAllReferences can return both y and z.

That is correct. In valueflow, when followAllReferences returns multiple references we then treat the lifetime as inconclusive.