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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
