[cfe-dev] [StaticAnalyser][RFC] New checker, -Wsign-compare without the noise

[Daniel Marjamäki via cfe-dev]

It was committed: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp

I would say it's not finished.

I saw true positives with this checker in some open source projects. I assume the Wconversion had been deactivated because it's too noisy.

In a comparison I made last year between PC-Lint and Clang, PC-Lint wrote 100's of conversion warnings and Clang didn't write any. I did not see a single "real" issue where the code was actually dangerous so I classified all those warnings as false positives.

..................................................................................................................
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<mailto:Daniel.Marjamaki at evidente.se>@evidente.se<mailto:Daniel.Marjamaki at evidente.se>

www.evidente.se
________________________________
From: Gábor Horváth [xazax.hun at gmail.com]
Sent: 17 February 2017 17:18
To: Daniel Marjamäki
Cc: Matthew Del Buono; cfe-dev at lists.llvm.org
Subject: Re: [cfe-dev] [StaticAnalyser][RFC] New checker, -Wsign-compare without the noise

Hi!

Having a check with less false positives is certainly useful. What is the state of this? Did you try it on some projects? Did you get promising results?

Regards,
Gábor

On 1 September 2015 at 20:06, Daniel Marjamäki <cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>> wrote:
>> I attach a simple proof of concept checker. It will just warn if there is an assignment and RHS is a known negative value. Do you have opinions about the design? Should some alternative approach be used?

> You seem to have substituted a lot of false positives for a lot of false negatives.

Thanks for looking!

It is just a proof of concept patch. I wanted it to be accurate. The finished patch will have fewer false negatives for sure.

My primary goal is to avoid obvious FPs. When it is obvious that the signed value can't be negative there should be no warning about loss of sign.

If that means the amount of FPs will be acceptable on real code, I am fine with that.

I have been told that we shouldn't even warn for:

    void foo(int x) {
        unsigned y;
        y = x;
    }

.. as developers often know that some function parameters are always positive. if we want to properly warn about that we should lookup all "foo" function calls and see if the parameter is negative sometimes.

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<mailto:Daniel.Marjamaki at evidente.se>@evidente.se<mailto:Daniel.Marjamaki at evidente.se>

www.evidente.se<http://www.evidente.se>
________________________________
Från: Matthew Del Buono [mpdelbuono at gmail.com<mailto:mpdelbuono at gmail.com>]
Skickat: den 1 september 2015 17:47
Till: Daniel Marjamäki
Kopia: cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>
Ämne: Re: [cfe-dev] [StaticAnalyser][RFC] New checker, -Wsign-compare without the noise


I attach a simple proof of concept checker. It will just warn if there is an assignment and RHS is a known negative value. Do you have opinions about the design? Should some alternative approach be used?


You seem to have substituted a lot of false positives for a lot of false negatives.

Why not look to see if the value cannot be provably non-negative? That would resolve the false positive you showed above, but still capture cases where the case only *might* be negative (rather than being guaranteed to be negative). In fact, I think the most interesting case is those where an off-by-one error or some other hidden defect has caused a scenario where almost all cases are positive, but left a couple cases open that the developer did not realize could be negative. Warning in these scenarios that the conversion could be unsafe would be very powerful, I think.

- Matthew P. Del Buono


_______________________________________________
cfe-dev mailing list
cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170218/43d17998/attachment.html>