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

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)
+    return;
+
----------------
NoQ wrote:
> 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!  :)

Repository:
  rC Clang

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

https://reviews.llvm.org/D66014