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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Romanenkov Kirill via cfe-dev
Hello!

I want to implement a new -Wsign-compare checker in the static analyser that is not noisy.

I like the idea of Wsign-compare. However the checker generates lots of noise:

    int x = 10;
    unsigned y = x;  // <- FP

The -Wsign-compare doesn't care that the code is 100% safe and warns anyway.

I only want to see a warning when there can be a real loss of precision or a real loss of sign (when the value can be negative).

I don't plan to remove the -Wsign-compare from Clang. People will have to use -Wno-sign-compare to get rid of 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?

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:                 [hidden email]

www.evidente.se

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

150901-sign.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [StaticAnalyser][RFC] New checker, -Wsign-compare without the noise

Romanenkov Kirill via cfe-dev

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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [StaticAnalyser][RFC] New checker, -Wsign-compare without the noise

Romanenkov Kirill via cfe-dev
>> 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:                 [hidden email][hidden email]                      

 

www.evidente.se


Från: Matthew Del Buono [[hidden email]]
Skickat: den 1 september 2015 17:47
Till: Daniel Marjamäki
Kopia: [hidden email]
Ä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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [StaticAnalyser][RFC] New checker, -Wsign-compare without the noise

Romanenkov Kirill via cfe-dev
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 <[hidden email]> 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:                 [hidden email][hidden email]                      

 

www.evidente.se


Från: Matthew Del Buono [[hidden email]]
Skickat: den 1 september 2015 17:47
Till: Daniel Marjamäki
Kopia: [hidden email]
Ä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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [StaticAnalyser][RFC] New checker, -Wsign-compare without the noise

Romanenkov Kirill 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:                 [hidden email][hidden email]                      

 

www.evidente.se


From: Gábor Horváth [[hidden email]]
Sent: 17 February 2017 17:18
To: Daniel Marjamäki
Cc: Matthew Del Buono; [hidden email]
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 <[hidden email]> 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:                 [hidden email][hidden email]                      

 

www.evidente.se


Från: Matthew Del Buono [[hidden email]]
Skickat: den 1 september 2015 17:47
Till: Daniel Marjamäki
Kopia: [hidden email]
Ä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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev