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