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

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx marked 3 inline comments as done.
chrish_ericsson_atx 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;
+
----------------
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?


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


================
Comment at: clang/test/Analysis/enum-cast-out-of-range.c:27-34
+enum unscoped_unspecified_t unused;
+void unusedExpr() {
+    // following line is not something that EnumCastOutOfRangeChecker should evaluate.  checker should either ignore this line
+    // or process it without producing any warnings.  However, compilation will (and should) still generate a warning having 
+    // nothing to do with this checker.
+    unused; // expected-warning {{expression result unused}}
+}
----------------
NoQ wrote:
> I guess this covers D33672#1537287!
> 
> That said, there's one thing about this test that i don't understand. Why does this AST contain an implicit lvalue-to-rvalue cast at all? This looks like a (most likely otherwise harmless) bug in the AST; if i understand correctly, this should be a freestanding `DeclRefExpr`.
It sure looks like D33672 is the same bug, so yes, I bet it will fix that one.   This fix also addresses https://bugs.llvm.org/show_bug.cgi?id=41388.  (Forgive me if there's a way to directly link to bugs.llvm.org with this markup.)

Not sure about whether the AST should have represented this node as a DeclRefExpr-- that certainly makes some sense, but the LValueToRValue cast isn't really wrong either.   At the end of the day, this statement is useless, so I'm not sure it matters (much) how the AST represents it.  Representing it as LValueToRValue cast wouldn't cause side effects, would it? (E.g.,  cause IR to contain explicit dereferencing code?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014