In D92634#2484404, @steakhal wrote:

Here is a link for our results on a few more projects. It might be useful for you.
https://codechecker-demo.eastus.cloudapp.azure.com/Default/runs?run=D92634&items-per-page=50&sort-by=name&sort-desc=false
Note: use the diff to filter only for the new reports.

Typically in such cases bug visitors should be added/improved until it is clear from the user-facing report why does the analyzer think so. They'd highlight the important events, prevent path pruning, and potentially suppress reports if the reason is discovered to not be valid.

I have run clang static analysis on random open source projects. The very first finding that I look at seems (to me) to be a false positive. :-(

I also agree with @NoQ's D92634#2478703 comment.

BTW, I cannot optimize function f to returning zero directly with GCC-10.2.1 and Clang-10.0.1 under -O3. Should I add any other flags? Or it is version specific?

However, the mainstream compilers like GCC and Clang implement this as the overflowed value, and some programmers also use this feature to do some tricky things.

Besides, the return value should be the exact value computed from the two integers, even unknown, rather than undefined. As the developers may overflow an integer on purpose.

ok.. thanks for the reviews. I will see if I can make some new check.

No reviews => I will not contribute.

Erik and I will not continue working on this. Feel free to take over the patch or write a new patch.

I will not continue working on this. Feel free to take over the patch or write a new patch.

I will not continue working on this. Feel free to take over the patch or write a new patch.

I will not continue working on this. Feel free to take over the patch or write a new patch.

I will not continue working on this. Feel free to take over the patch or write a new patch.

> So what are the arguments that are passed to getSimplifiedOffset() in that case? 0? That does not seem to be correct.

Could you do a similar analysis that I did above to check why does this not work for the multidimensional case? (I.e.: checking what constraints are generated and what the analyzer does with them.)

I have updated the patch so it uses evalBinOpNN. This seems to work properly.

Do you mind writing some tests with multidimensional arrays to check what do we lose if we remove that code?

As suggested, use a ProgramState trait to detect VLA overflows.

I think it is much better when the assert failure tells the developer _what_ value is failing, rather than saying "oops we are dead".

I like this patch overall.. here are some stylistic nits.

Stylistically this looks pretty good to me. Just a minor nit.

LGTM

LGTM.. however I would like approval from somebody else also.

ping

Track modification of global static variables in CallGraph construction

In D37897#892667, @dcoughlin wrote:

Apologies for the delay reviewing! As I noted inline, I'm pretty worried about the performance impact of this. Is it possible to do the analysis in a single traversal of the translation unit?

ping

LGTM! However I would like to see a review from somebody else also.

I think a test for -Wtautological-pointer-compare should be added that shows that the bug is fixed.

In D38675#891912, @xazax.hun wrote:

In D38675#891750, @danielmarjamaki wrote:

However, the checker seems to work with a low false positive rate. (<15 on the LLVM, 6 effectively different)

This does not sound like a low false positive rate to me. Could you describe what the false positives are? Is it possible to fix them?

Note that the unique findings are 6. I think there are non-alpha checks with more false positives.

LGTM

However, the checker seems to work with a low false positive rate. (<15 on the LLVM, 6 effectively different)

Fixes according to review comments. Reuse ast matchers in LoopUnrolling.cpp. Avoid some recursion (however the isChanged() is still recursive but it is very small and simple).

ping

ping

fixed review comments

ping

In D37897#871858, @xazax.hun wrote:

Out of curiosity, does the false positive disappear after making the static variables const?

Minor cleanups. Changed names. Updated comments.

ping

ping

This is not committed as far as I see.. do you have write permission or do you want that I commit it?

minor code cleanup

ping

small nits

ping

ping

LGTM. But others should approve.

LGTM. I let others approve this.

Should evalAPSInt() have machinery to do standard sign/type promotions? I suggest that I add one more argument bool promote = false, do you think that sounds good?

Refactoring, use BasicValueFactory::evalAPSInt

In D36471#835410, @xazax.hun wrote:

Can't you reuse somehow some machinery already available to evaluate the arithmetic operators? Those should already handle most of your TODOs and overflows.

A minor code cleanup. No functional change.

Cleaned up the patch a little. Thanks Gabor for telling me about SValBuilder::getKnownValue()

Fix review comments

I will not continue working on this checker

ping

ping

Fix review comments

renamed exprComparesTo to svalComparesTo

minor tweak

If you have svn write permission then please do it.

Ping.

Ping. Any comments?

Ping

I am thinking about making my check more strict so it only warns in allocations. I believe the example code is much more motivating when there is allocation.

Fixed review comments. Made code examples and documentation more motivational.