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.

Thanks for all comments. I am working on fixing them. Updated patch will be uploaded soon.

you can ignore my comment ... LGTM

I don't have further comments except that I would personally rewrite:

// Get the value of the size argument.
SVal TotalSize = State->getSVal(Arg1, LCtx);
if (SuffixWithN) {
  const Expr *Arg2 = CE->getArg(2);
  TotalSize = evalMulForBufferSize(C, Arg1, Arg2);
}

to:

// Get the value of the size argument.
SVal TotalSize;
if (!SuffixWithN) {
  TotalSize = State->getSVal(Arg1, LCtx);
} else {
  TotalSize = evalMulForBufferSize(C, Arg1, CE->getArg(2));
}

I hold the view that I need to respect original developers' code, and it need a Global Patch for Capital variable, just like KDE's Use nullptr everywhere