[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts
Chris Hamilton via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 12 14:25:53 PDT 2019
chrish_ericsson_atx marked an inline comment as done.
chrish_ericsson_atx added a comment.
In D66014#1625733 <https://reviews.llvm.org/D66014#1625733>, @Szelethus wrote:
> Oh, btw, thank you for working on this!
Glad to help!
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)
> 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 ]]**.
Ah. Okay -- That is a list I've seen before, of course... :)
Hmm... regarding whitelisting versus blacklisting... In this case, it seems to me that switching to whitelisting casts that we know we should be checking increases the risk that we would introduce a new bug -- specifically that we'd accidentally leave out cast type that should have been included, which would disable a legitimate check (that some users might already be relying on). By contrast, blacklisting the one cast type we know should *not* be checked adds zero new risk and still fixes this bug.
The sense of risk for me comes partly from being fairly new to working on clang AST code -- this is my first change. It strikes me that if I were to feel confident about having the "correct" whitelist, I would have to understand every cast type fairly deeply. Given that I see several cast kinds in that list that I know nothing about, I'm concerned with the level of effort required, and that I still won't be able to fully mitigate the risk.
Can you help me understand your rationale for preferring a whitelist in this case? Or, do you feel I've overstated the risk here? I'm not emotionally invested in being proven correct-- just want to learn! :)
CHANGES SINCE LAST ACTION
More information about the cfe-commits