[RFC][PATCH] Static analyser warning about identical inner condition

Clang Frontend
1

Hello!

This is just a request for comments.

The patch I attach improves the identicalexpr checker in the static analyser. It adds a warning when there is an identical inner condition inside a if.

Example code taken from a debian project:

        if(revint)
        {
            if(revint)

Testing:
I scanned 477 debian projects with this checker and got 2 warnings. Both are bugs as far as I see. Also, in both these cases the inner conditions does not have an 'else' so there is no unreachable code.

I think it would be a good idea to add a heuristic to avoid warnings when there is hidden code. For example:

    if (x)
    {
#if SOME_FALSE_CONDITION
        x = dostuff();
#endif
        if (x)

But other than that... this checker seems to be reliable imho.

A possible further improvement would be to warn also warn if the inner condition "overlaps". For instance:

    if (x == 15)
    {
        if (x > 0)
        ...

Any opinions / ideas?

Best regards,
Daniel Marjamäki

..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden

Mobile: +46 (0)709 12 42 62
E-mail: Daniel.Marjamaki@evidente.se

www.evidente.se

150505-IdenticalInnerCondition.diff (1.02 KB)

2

Hi!

Just a minor note: it might give false positives in case there is a side effect in the condition that is repeated.

Wouldn’t it be better to catch these kind of bugs using the constraint manager and checking for always true conditions?

Best Regards,

Gábor Horváth

3

Thanks!

Just a minor note: it might give false positives in case there is a side effect in the condition that is repeated.

Good thinking.

The isIdenticalStmt should return false then. Since the last argument IgnoreSideEffects is false.

Wouldn't it be better to catch these kind of bugs using the constraint manager and checking for always true conditions?

I have tried this approach too. But I did not see how to fix it. Problem is when constraintmanager can see that x is 0 in some execution path.. I do not see how I can tell that x is 0 also in all other execution paths.

For instance if I have this code:

    if (x==0)

then the visitor for the condition is called twice and first the constraintmanager tells me that x is 0, and then the constraintmanager tells me that x is 1.

I attach a patch where I tried that approach. It generates lots of false positives. Feel free to tell me how I fix this.

Best regards,
Daniel Marjamäki

..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden

Mobile: +46 (0)709 12 42 62
E-mail: Daniel.Marjamaki@evidente.se

www.evidente.se

150505-always-true.patch (4.26 KB)