int f(int nr) {
int x;
for (int i = 0; i < nr; ++i) {
x = i;
}
return x;
}
Theoretically, if "nr" is less than 0 then the return value will be uninitialized.
As far as I have seen we know that such loops are executed at least once very often. I can't remember a TP where it was possible that such loop code would not be executed.
Can we try to fix this so the analyzer will be silent for the above code? I would like that if the variable is unconditionally written in the loop code there should be no warning.
Best regards,
Daniel Marjamäki
..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
I’ve seen the analyzer find real problems where loops are not executed and think this is very useful. Personally I’d rather see the warnings and add asserts, which makes it clearer that ‘nr’ must be greater than 1, making it less likely someone will add an incorrect call in the future. While that is a preference for avoiding false negatives over false positives, I think it is reasonable in this case given there is no analysis across translation units.
You tend to get false positives when the Analyzer can’t see any calls to the function, so doesn’t know ‘nr’ can never be less than 1. You can get false negatives when the analyzer can only see some of the calls.
I totally agree. If you have such constrain then you should express it in code, for example using do-while or assert, or why just not initialize the value?
I am not against that those warnings are available also. Imho the heuristic is too noisy for me right now so I would be happier if I could personally disable that for now. Move the heuristic to alpha or something.
I’ve seen the analyzer find real problems where loops are not executed and think this is very useful. Personally I’d rather see the warnings and add asserts, which makes it clearer that ‘nr’ must be greater than 1, making it less likely someone will add an incorrect call in the future. While that is a preference for avoiding false negatives over false positives, I think it is reasonable in this case given there is no analysis across translation units.
I am trying to introduce Clang analyzer on an existing large code base and they don’t want to see any false positives. I would ideally like to have very restrictive checking for uninitialized variables right now.
It is acceptable to add some assertions and annotations to achieve no false positives. But I’m afraid it looks bad that most of the warnings are wrong right now.
Best regards,
Daniel Marjamäki
…
Daniel Marjamäki Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
That's actually wrong. The loop won't be executed for 0 either. That is
a good example of why I consider the analysis completely valid -- humans
are surprisingly bad at ensuring that complex unenforced pre-conditions
are actually true.
I am trying to introduce Clang analyzer on an existing large code base
and they don't want to see any false positives. I would ideally like to
have very restrictive checking for uninitialized variables right now.
If they don't want to see any false positives, they shouldn't even ask
the compiler for warnings. It is a completely absurd constraint to put
on any analysis system. The trick for tools like Coverity and where the
majority of the research budget goes is to develop heuristics on what
false positives should be silently dropped.
It is acceptable to add _some_ assertions and annotations to achieve
no false positives. But I'm afraid it looks bad that most of the
warnings are wrong right now.
But the example you have given is *not* wrong. It is completely correct.
It might be wrong for an internal function where you can reason about
all possible call sites and the restrictions of the argument ranges.
But that's not the case in the example you gave.
As far as I know, having few false positives is a major goal for the Clang static analyzer.
Yes I understand that false positives are inevitable. So a few fixes in my code to hide them are expected.
However if most warnings for a particular use case are in practice false positives I say these should be dropped. So I think the question should be if most warnings are false positives? Do you think your true positive ratio in your own code is good? Is it higher than 50%?
But the example you have given is *not* wrong. It is completely correct.
Imho it is a false positive if I can easily see with human intelligence that the warning is wrong.
Best regards,
Daniel Marjamäki
..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
As far as I know, having few false positives is a major goal for the Clang static analyzer.
Yes I understand that false positives are inevitable. So a few fixes in my code to hide them are expected.
A good example of unavoidable false positives is the format string
checker. There is exactly one situation where passing format string
and a separate va_list is valid: if you expand some custom format string
that doesn't take an argument (consider %m for non-syslog functions) and
you expand it in a way that correctly quotes any potential %. The
compiler can't know that and that's why it is a false positive.
The workaround in the code is to explicitly disable the warning/check
for that specific case.
However if most warnings for a particular use case are in practice false positives I say these should be dropped. So I think the question should be if most warnings are false positives? Do you think your true positive ratio in your own code is good? Is it higher than 50%?
> But the example you have given is *not* wrong. It is completely correct.
Imho it is a false positive if I can easily see with human intelligence that the warning is wrong.
But you can't without adding additional assumptions that are not part of
the code. In this case "nr must be positive". That's why it is not a
false positive -- you have to add external knowledge. If the constraint
can't be handled by the analyzer, that's again a different situation.
That's e.g. a problem when the initialisation happens in a different
function and the analyzer can't or doesn't want to see it. Another case
is if the constraint depends on a non-trivial computation and the
analyzer isn't smart enough to figure it out. All those are examples of
false positives: the information is available, but the system doesn't
use them. It is not a false positive if information is missing in first
place.
While false positives are obviously inevitable (there are various well-known reasons for the clang static analyzer's technique to have false positives; even outside the reach of halting problem), there are reasons why false positives are destructive:
(1) If a new user takes the tool, picks 3-4 positives and finds that they're all false, she may never give the tool another chance.
(2) If you have 1% false positives on your codebase, it means that there's a pattern that the tool fails upon; but there might be another codebase on which that pattern is popular and you'd get thousands of warnings with 100% false positive rate.
So yeah, we inevitably have to treat every false positive as carefully as possible, much more carefully than false negatives. That said, ugly heuristics are rarely the best choice, yeah.
Suppose we analyzed the program from the very main(), scanned all call sites in all translation units, and proven that the path is never reached in this program. Then the positive would have been gone. So perhaps this positive is not unavoidable, and i don't think this information is missing - just the analyzer was not taught to use it. If it was a library interface function, then yeah, the user is known to be able to throw arbitrary stuff into it. But perhaps the library documentation will suggest that "if nr is non-positive, the behavior is undefined"... no longer sure if it's true or false this time.
But i'd also consider classifying positives as "useful" and "useless", rather than "true" or "false". For example, when the code needs to be fixed, this positive is good. When it requires documenting (documenting contracts through asserts included), it's also probably useful, but perhaps less useful - there was no bug in the program, so the user wouldn't notice the change. This particular positive, depending on the circumstances, may or may not be very useful. Assertion is worth adding, yeah. But the software's end user may not benefit from it, so the manager may avoid supporting the usage of such tool in the project, if most positives need to be suppressed rather than fixed.
Couldn’t there be a “mode” where the analyzer would only report when it finds a patch where the condition happens.
I.e. instead of considering “I don’t know what is the possible value for nr so I complain about a possible error”, having the alternate behavior of “I know that there is a path where nr can be <= 0 and the loop not executed so I complain”.
That may suppress a large number of "true positives” indeed, but if you start on a “dirty” codebase that may be useful to focus on “real” issues.
That must be possible, and i think i'm also throwing in a generalization of this idea in http://lists.llvm.org/pipermail/cfe-dev/2016-August/050550.html (paths on which the condition certainly happens, i.e. because all values are concrete, are totally-"realistic"; branch conditions based on SymbolRegionValue of static function parameters must have less realism compared to branches based on externally visible function parameters, and so on).
However if most warnings for a particular use case are in practice
false positives I say these should be dropped. So I think the
question should be if most warnings are false positives? Do you think
your true positive ratio in your own code is good? Is it higher than
50%?
But the example you have given is *not* wrong. It is completely
correct.
Imho it is a false positive if I can easily see with human
intelligence that the warning is wrong.
But you can't without adding additional assumptions that are not part of
the code. In this case "nr must be positive". That's why it is not a
false positive -- you have to add external knowledge. If the constraint
can't be handled by the analyzer, that's again a different situation.
That's e.g. a problem when the initialisation happens in a different
function and the analyzer can't or doesn't want to see it. Another case
is if the constraint depends on a non-trivial computation and the
analyzer isn't smart enough to figure it out. All those are examples of
false positives: the information is available, but the system doesn't
use them. It is not a false positive if information is missing in first
place.
Suppose we analyzed the program from the very main(), scanned all call
sites in all translation units, and proven that the path is never
reached in this program. Then the positive would have been gone. So
perhaps this positive is not unavoidable, and i don't think this
information is missing - just the analyzer was not taught to use it. If
it was a library interface function, then yeah, the user is known to be
able to throw arbitrary stuff into it. But perhaps the library
documentation will suggest that "if nr is non-positive, the behavior is
undefined"... no longer sure if it's true or false this time.
But i'd also consider classifying positives as "useful" and "useless",
rather than "true" or "false". For example, when the code needs to be
fixed, this positive is good. When it requires documenting (documenting
contracts through asserts included), it's also probably useful, but
perhaps less useful - there was no bug in the program, so the user
wouldn't notice the change. This particular positive, depending on the
circumstances, may or may not be very useful. Assertion is worth adding,
yeah. But the software's end user may not benefit from it, so the
manager may avoid supporting the usage of such tool in the project, if
most positives need to be suppressed rather than fixed.
I think OP's particular case, the assertion *is* the correct suppression.
Couldn’t there be a “mode” where the analyzer would only report when
it finds a patch where the condition happens. I.e. instead of
considering “I don’t know what is the possible value for nr so I
complain about a possible error”, having the alternate behavior of “I
know that there is a path where nr can be <= 0 and the loop not
executed so I complain”.
I think it would be very useful, even if only to explain what assumptions the analyzer made when it came to that conclusion. I bet that would make it easier to "correct" those assumptions via assertions, when you see that it thinks something could happen that you happen to know is never true.
The analyzer does - or at least tries its best to - explain all the assumptions it takes (in -analyzer-output=text or -analyzer-output=html or -analyzer-output=plist, which includes all known/sensible use cases). This system is being improved, but it's already pretty good. Also, the analyzer tries to skip some assuming-path-pieces that are probably useless, but even that can be avoided with "-analyzer-config prune-paths=false".
Couldn’t there be a “mode” where the analyzer would only report when it finds a patch where the condition happens.
Yes that sounds very useful.
That may suppress a large number of "true positives” indeed, but if you start on a “dirty” codebase that may be useful to focus on “real” issues.
Yes exactly.
Best regards,
Daniel Marjamäki
..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
