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

Fix review comments

• renamed
• reorder function arguments (CheckerContext last)

I believe https://reviews.llvm.org/D32164 is better

Or I can do it for you if you wish.

Please click "Done" on fixed review comments.

I would propose that I rename and cleanup RangeConstraintManager::uglyEval() and add it. When I tested it, the Z3 does not seem to handle this.

I would recommend that this is either fixed soon or that we commit my changes so it can be implemented more properly later. Right now users will see false positives.

Ping

Thanks! Looks like a valueable addition.

This is just work in progress!!

sorry ... I guess that should be something like "void *p = malloc(100);"

In D31650#717691, @NoQ wrote:

Is freeing function pointers always undefined?