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?

Ping

Ping

Ping

In D31029#708567, @danielmarjamaki wrote:

In D31029#703428, @zaks.anna wrote:

Are there other cases where makeNull would need to be replaced?

There might be. As I understand it, this is the only known case at the moment.

Updated the patch so all the loss of precision are detected also

In D31029#703428, @zaks.anna wrote:

Are there other cases where makeNull would need to be replaced?

Added a testcase that will crash without the fix. Used the amdgcn target as that happens to use different pointer bit widths for different address spaces.

I added more testcases. There are several undetected "TODO: loss of precision" right now in the tests that I would like to fix. If this patch to fix FP is accepted I will commit it and continue working on the TODO tests. If it's not accepted I will continue investigating the TODO tests anyway..

Well.. feel free to provide an alternative fix. If the message is more specific and it must be enabled explicitly by an option then maybe it's good enough for me.

In D31097#707385, @baloghadamsoftware wrote:

Hi!

There is an option to disable the checking of widening casts. It is enabled by default. You can disable it any time. Or, if you find too much false positives, we can discuss about setting this option to disabled as default.

I am convinced that checking implicit widening casts are also necessary. We should probably change the error message in the implicit case from "misplaced" to "missing", and maybe also rename the checker itself. Separating it to two different checkers, which are almost copy of each other is huge code duplication.

I believe there is pointless code in relativeIntSizes etc. If there is for instance "a+b" then the result can't be a char type.

static int relativeIntSizes(BuiltinType::Kind Kind) {
  switch (Kind) {
  case BuiltinType::UChar:
    return 1;
  case BuiltinType::SChar:
    return 1;
  case BuiltinType::Char_U:
    return 1;
  case BuiltinType::Char_S:
    return 1;
  case BuiltinType::UShort:
    return 2;
  case BuiltinType::Short:
    return 2;
  case BuiltinType::UInt:
    return 3;
  case BuiltinType::Int:
    return 3;
  case BuiltinType::ULong:
    return 4;
  case BuiltinType::Long:
    return 4;
  case BuiltinType::ULongLong:
    return 5;
  case BuiltinType::LongLong:
    return 5;
  case BuiltinType::UInt128:
    return 6;
  case BuiltinType::Int128:
    return 6;
  default:
    return 0;
  }
}

Remove warnings for implicit casts.

Fix review comment. Made isShiftOverflow() static.

In D31097#704628, @alexfh wrote:

In D31097#704626, @alexfh wrote:

In D31097#704621, @xazax.hun wrote:

I wonder whether warning on implicit casts still makes sense for example in mission critical code. So maybe it is worth to have a configuration option with the default setting being less strict and chatty. What do you think?

But it's not about "misplaced casts", it's about implicit conversions and -Wconversion diagnostic can take care of this.

Actually, the diagnostics about implicit casts here might be useful (but maybe in a separate check). I have to look again at https://reviews.llvm.org/D17987.

In my opinion, we should stop warning about all implicit casts.

Fix review comments

I am not sure where to look. I heard somebody say OpenCL has different pointer widths.

Ping

Also, in your state dumps no information is actually lost. The fact that the value of variable sz is reg_$0<sz> is trivial: you could ask the Store what's the value of the variable sz and it'd say reg_$0<sz> if there are no bindings over it.

No the argument is not shown with tilde/column number.

Thanks! That sounds excellent to me.

Fix review comment

To me it seems that the extent is calculated properly in ArrayBoundsV2.

I am running this checker right now on various projects. Here are currently seen results.. https://drive.google.com/open?id=0BykPmWrCOxt2STZMOXZ5OGlwM3c

minor updates. Use llvm::getOrdinalNumber(). Use llvm::Twine.

Fixed review comment. Broke out function.

Making the error message more precise.

I will not work on this in the near future

I have updated the patch and want a new review.

It was reported in the bugzilla report that my first fix did not fix all crashes. A new example code was provided that triggered a new crash. I have updated the patch so both crashes are fixed.

Ping

In D28278#677905, @zaks.anna wrote:

Does the code you added detects array out of bounds cases without false positives? Is it an option to just have this checkers produce a more precise error message in the specific case.

A lot of work will probably need to be done to implement a proper array out of bounds checking and no-one is working on that.

I reverted the change because there were buildbot failures.