[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Oh, btw, thank you for working on this!



================
Comment at: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:121-122
   // Check if any of the enum values possibly match.
   bool PossibleValueMatch = llvm::any_of(
       DeclValues, ConstraintBasedEQEvaluator(C, *ValueToCast));
 
----------------
NoQ wrote:
> chrish_ericsson_atx wrote:
> > Szelethus wrote:
> > > So this is where the assertion comes from, and will eventually lead to `SValBuilder::evalEQ`, which calls `SValBuilder::evalBinOp`, where this will fire on line 427:
> > > ```
> > > assert(op == BO_Add);
> > > ```
> > > Seems like this happens because `unused`'s value in your testcase will be retrieved as a `Loc`, while the values in the enum are (correctly) `NonLoc`, and `SValBuilder::evalBinOp` thinks this is some sort of pointer arithmetic (`5 + ptr` etc).
> > > 
> > > How about instead of checking for LValueToRValue cast, we check whether `ValueToCast` is `Loc`, and bail out if so? That sounds like a more general solution, but I didn't sit atop of this for hours.
> > Is this the only case where ValueToCast will be Loc?   I was unsure about the implications of Loc vs. NonLoc in this code, and I didn't want to risk breaking a check that should have been valid.  That's why I opted for bailing on an LValueToRValue cast at a higher level-- that seemed much safer to me.  I certainly might be missing something, but I couldn't find any evidence that an LValueToRValue cast should ever need to be checked for enum range errors.   I will certainly defer to your judgement, but I'd like to have a better understanding of why looking for ValueToCast == Loc would actually be more correct.
> > How about instead of checking for `LValueToRValue` cast, we check whether `ValueToCast` is `Loc`, and bail out if so?
> 
> In this case it probably doesn't matter too much, but generally i always recommend against this sort of stuff: if possible, consult the AST. A `Loc` may represent either a glvalue or a pointer-typed prvalue, and there's no way to tell the two apart by only looking at that `Loc`. In particular, both unary operators `*` and `&` are modeled as //no-op//. I've been a lot of incorrect code that tried to dereference a `Loc` too many or too few times because the code had misjudged whether it has already obtained the prvalue it was looking for. But it's easy to discriminate between the two when you look at the AST.
> 
> The Static Analyzer is a converter from ASTs to ExplodedGraphs. When in doubt, consult the AST.
Notes taken, thanks! :)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66014/new/

https://reviews.llvm.org/D66014