[cfe-dev] [RFC][PATCH] Static analyser warning about identical inner condition

[Daniel Marjamäki]

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 at evidente.se

www.evidente.se

________________________________________
Från: Gábor Horváth [xazax.hun at gmail.com]
Skickat: den 8 maj 2015 12:27
Till: Daniel Marjamäki
Cc: cfe-dev Developers
Ämne: Re: [cfe-dev] [RFC][PATCH] Static analyser warning about identical inner condition

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

On 8 May 2015 at 10:46, Daniel Marjamäki <Daniel.Marjamaki at evidente.se<mailto:Daniel.Marjamaki at evidente.se>> wrote:


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<tel:%2B46%20%280%29709%2012%2042%2062>
E-mail:                 Daniel.Marjamaki at evidente.se<mailto:Daniel.Marjamaki at evidente.se>

www.evidente.se<http://www.evidente.se>

_______________________________________________
cfe-dev mailing list
cfe-dev at cs.uiuc.edu<mailto:cfe-dev at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 150505-always-true.patch
Type: text/x-patch
Size: 4359 bytes
Desc: 150505-always-true.patch
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150508/2aa83d3f/attachment.bin>