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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+    return;
+
----------------
chrish_ericsson_atx wrote:
> NoQ wrote:
> > If you look at the list of cast kinds, you'll be shocked to see ridiculously weird cornercases. Even though lvalue-to-rvalue cast definitely stands out (because it's the only one that has very little to do with actual casting), i'd still be much more comfortable if we *whitelist* the casts that we check, rather than blacklist them.
> > 
> > Can you take a look at the list and find which casts are we actually talking about and hardcode them here?
> I'm happy to cross-check a list; however, I'm not aware of the list you are referring to.  Would you mind providing a bit more detail?
See **[[ https://github.com/llvm-mirror/clang/blob/release_90/include/clang/AST/OperationKinds.def | include/clang/AST/OperationKinds.def ]]**.


================
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));
 
----------------
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014