Page MenuHomePhabricator

[analyzer] Fix modeling of bool based types
ClosedPublic

Authored by alexshap on Jul 5 2017, 9:49 PM.

Details

Summary

This is a follow up for one of the previous diffs https://reviews.llvm.org/D32328.
getTypeSize and with getIntWidth are not equivalent for bool
(see https://clang.llvm.org/doxygen/ASTContext_8cpp_source.html#l08444),
this causes a number of issues
(for instance, if APint X representing a bool is created with the wrong bit width then X is not comparable against Min/Max
(because of the different bit width), that results in crashes (triggered asserts) inside assume* methods) (for example see the test case).
P.S. I have found some other suspicious places where getIntWidth probably should be used instead of getTypeSize,
however fixing all of them in one go is not trivial
(a couple of tests have failed on the larger diff (which might indicate some issues inside those checkers), but I have not investigated it yet,
so decided to break up my changes into smaller chunks and keep all the tests green.

Test plan:
make check-all

Diff Detail

Repository
rL LLVM

Event Timeline

alexshap created this revision.Jul 5 2017, 9:49 PM
alexshap retitled this revision from [analyzer] Fix modeling bool-based types to [analyzer] Fix modeling of bool based types.Jul 5 2017, 9:52 PM
NoQ edited edge metadata.Jul 6 2017, 1:10 AM

Thanks, these are very helpful.

Sending changes in smaller chunks is great even if you already have one large chunk perfectly working :)

The test covers only the BasicValueFactory-related part of your patch; the Loc-related changes are still to be tested. I suspect the following may work:

  1. For evalCastFromNonLoc: casting a symbolic pointer to an integer, then casting this integer to the enum class.
  2. For evalCastFromLoc: Casting a concrete pointer (eg. null pointer) to the enum class (not sure if you can trick the analyzer into such cast directly while following the c++ standard).

thanks,

  1. evalCastFromNonLoc is actually called there (and that's how i ran into this issue) because of (a bit unexpected to me) ImplicitCastExpr 'LValueToRValue' inside switch:

(to be honest i don't know why Clang inserts this cast into AST here, but i have checked several versions of clang - the behavior is the same)

  1. evalCastFromLoc - that's hard to me - so far i have not found a valid compilable c++ code to cast a pointer to bool enum.
NoQ added a comment.Jul 6 2017, 11:25 AM

evalCastFromNonLoc is actually called there (and that's how i ran into this issue) because of (a bit unexpected to me) ImplicitCastExpr 'LValueToRValue' inside switch

Yeah, but the NonLoc that's being casted is definitely not nonloc::LocAsInteger.
And i also tried to run your test with only BasicValueFactory fix and it passed.

evalCastFromLoc - that's hard to me - so far i have not found a valid compilable c++ code to cast a pointer to bool enum.

I didn't see if that's exactly what we're looking for, but the following compiles and crashes:

EnumBool E = static_cast<EnumBool>((intptr_t)nullptr);
switch (E) {
case EnumBool::F:
  return false;
}
return true;

In D35041#801073, @alexshap wrote:
evalCastFromNonLoc is actually called there (and that's how i ran into this issue) because of (a bit unexpected to me) ImplicitCastExpr 'LValueToRValue' inside switch

Yeah, but the NonLoc that's being casted is definitely not nonloc::LocAsInteger.

yea, you are right, sorry about the confusion.

alexshap updated this revision to Diff 105524.Jul 6 2017, 1:41 PM

Add a test where evalCastFromLoc is called
and the line unsigned BitWidth = Context.getIntWidth(castTy);
is hit.
I am planning to update this patch once again with a proper test where
the path inside evalCastFromNonLoc is executed (need a bit more time)

alexshap updated this revision to Diff 105562.Jul 6 2017, 4:42 PM

Add a test where SimpleSValBuilder::evalCastFromNonLoc is called and we hit the line unsigned castSize = Context.getIntWidth(castTy);

alexshap updated this revision to Diff 105597.Jul 7 2017, 2:05 AM
NoQ accepted this revision.Jul 10 2017, 12:02 PM
NoQ added inline comments.
test/Analysis/enum.cpp
50

Nice! Will use in my tests.

This revision is now accepted and ready to land.Jul 10 2017, 12:02 PM
This revision was automatically updated to reflect the committed changes.